The sed code needs some more work.
It does not work when the variables are not already there...
I have tried some more, but I either keep adding the variables, or it does not work when they are still empty...

In my case they became long enough that mail delivery stopped.

Can you share why this was happening that many times? What was restarting the container so often?

You should use docker compose up -d --force-recreate or docker compose down && docker compose up -d, this will clear the internal state changes that aren't meant to persist, and apply config changes correctly.

If the container is being restarted instead of recreated, it would be helpful to identify why that is happening. Are you restarting the docker host, or some kind of automated deployment/update tool?

This probably won't be the only config that isn't resilient to container restarts unfortunately, it's just one that more visible due to the overhead of repeating the milters excessively.

An easy way to fix this should be to explicitly set the default provided in main.cf, then let the setup scripts adjust it after that as they presently do. As you can see the default is unset:

So you might want to add a new function in this file at the top, that is called before the others, which resets these two settings?:

# Prevent appending repeated values when container is restarted:
function _reset_milters() {
  postconf 'smtpd_milters ='
  postconf 'non_smtpd_milters ='
}

I'm not 100% sure if that is the exact command to clear/unset the value like we have in main.cf, but I think it is 😅

I have installed/configured docker-mailserver using this ansible role: https://github.com/hmlkao/ansible-docker-mailserver

I don't think that has anything to do with the restarts, but it does result in me not being able to use docker compose (I think).

I use watchtower to update my docker images when there are new releases. I guess that is responsible for restarting the container that often. Sure, the host and docker service have been restarted a couple of times, but that should have been limited to about 10 times or so. As I have only started using this about 3 months ago.
So I guess watchtower is to blame.

That being said, it would be nice/better if the scripts that change the config are idempotent. And what better place to start, than in a place where it will result in issues. (and even worse, issues that give relatively obscure errors... :) )
This specific example is relatively easy. If there is a list of scripts that change config files, I might be able to help out making them idempotent. It might be even a good idea to have a test for that. :)

We do have a lot of reports about scripts not being idempotent, and I agree: they should be. When inspecting your DMS main logs, you should probably be able to see something like

This container was (likely) improperly restarted which can result in undefined behavior
Please destroy the container properly and then start DMS again

@polarathene @casperklein In

what if we just run

? The only important thing would be to properly exit afterwards and to ask whether any configuration breaks. This would in turn save us from having to insert checks in all places where we adjust files during startup.

what if we just run

Sorry, I don't quite follow. Do you propose removing the _check_improper_restart() log lines advice with the tail?

One solution is to place our service config files in a DMS location like you'll find packages do with example config, or we could use /etc. Then at startup we copy them over to the proper service config locations.

There is a risk there for users that directly mount config overrides themselves at these locations. When we changed the SASL socket location, we have multiple users experience breakage because they had mounted their own Dovecot config which didn't have the new SASL socket path configured while Postfix did.

If the user isn't careful with backups, they'd lose that custom config by updating and not paying attention to the changelog. That hiccup aside, it would probably ensure a predictable config state for most services when a container is restarted.

If there is a list of scripts that change config files, I might be able to help out making them idempotent. It might be even a good idea to have a test for that. :)

This whole project is mostly glue in bash scripts that modifies those configs 😅

We have plenty of tests, but it's not practical for us to test every combination in a manner that reproduces these kind of bugs. In this case it's having DKIM/DMARC enabled and restarting the container with the expectation that the config wouldn't differ from the first generation.

We could probably have a test with several common configurations that starts the container, copies the modified configs and restarts to compare with a diff/checksum if they've diverged. We would appreciate that, but my earlier suggestion of resetting the config during container startup might also work better 🤔

what if we just run

Sorry, I don't quite follow. Do you propose removing the _check_improper_restart() log lines advice with the tail?

Not quite. Instead of

function _check_improper_restart() { 
  _log 'debug' 'Checking for improper restart' 
  
  if [[ -f /CONTAINER_START ]]; then 
    _log 'warn' 'This container was (likely) improperly restarted which can result in undefined behavior' 
    _log 'warn' 'Please destroy the container properly and then start DMS again' 
  fi
}

just run

function _check_improper_restart() { 
  _log 'debug' 'Checking for improper restart' 
  
  if [[ -f /CONTAINER_START ]]; then 
    _log 'info' 'This container was (likely) restarted -- not configuring again' 
    tail -Fn 0 /var/log/mail/mail.log 
    exit 0 
  fi
}

I don't understand your intension here. BTW: unless tail crashes or something, exit 0 will never be executed.

Probably not a good idea after all.. forget about it. This issue is marked for v13.0.0. If we want this to get fixed for v13.0.0, I would use a simple check before running sed to see if the milters are already present.

just run

I'm not sure if that'd work, and may cause more bug reports despite the log line when config changes are made.

If we want this to get fixed for v13.0.0

We can avoid the check and just reset / unset the values. They're DMS specific ones rather than official postfix parameters IIRC?

I would use a simple check before running sed to see if the milters are already present.

That is exactly what prepending the sed command with /\$dkim_milter/! does.
It will only apply the substitution when $dkim_milter is not present on that line.

The sed code needs some more work.

It does not work when the variables are not already there...

I have tried some more, but I either keep adding the variables, or it does not work when they are still empty...

This seems to still apply I guess? Guarding with a regex check (=~ <REGEXP>) should probably be easier than using the sed syntax 🤔 not sure though. In either case, this needs to be done.

In either case, this needs to be done.

What is wrong with the suggestion to just reset the two settings?

It's two commands and would be consistent regardless of container starting fresh or restarted. No need for conditional logic with regex then.

This seems to still apply I guess? Guarding with a regex check (=~ <REGEXP>) should probably be easier than using the sed syntax thinking not sure though. In either case, this needs to be done.

Nope, the second commit to the branch fixed that issue. I think the sed syntax is easy enough to do it in sed, but that is a personal preference I guess.

What is wrong with the suggestion to just reset the two settings?

Nothing I guess, since they start out empty. Only when people use their own config files that might not be the case, maybe because of that having an idempotent sed command might be preferable?

Only when people use their own config files that might not be the case

In this case they'd be expected to override the config with postfix-main.cf, not mounting a /etc/postfix/main.cf directly.

I like the sed guard, but to be consistent with the rest of our scripts, we should employ @polarathene's proposal.