scripts: add wrapper to update Postfix configuration safely#3484
scripts: add wrapper to update Postfix configuration safely#3484georglauterbach merged 6 commits intomasterfrom
Conversation
georglauterbach
added
area/scripts
area/configuration (file)
kind/bug/fix
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
I think I suggested somewhere using postconf instead of sed to set / append values for main.cf, but can't recall if there were any issues with that (beyond the 2 sec delay to read a setting when not using the mtime workaround),
Regardless this helper is awesome (and well documented!), great work 😁
| -e '/\$dkim_milter/! s|^(smtpd_milters =.*)|\1 \$dkim_milter|g' \ | ||
| -e '/\$dkim_milter/! s|^(non_smtpd_milters =.*)|\1 \$dkim_milter|g' \ | ||
| /etc/postfix/main.cf | ||
| _append_to_option_in_postfix_main 'smtpd_milters' "\$dkim_milter" |
There was a problem hiding this comment.
| _append_to_option_in_postfix_main 'smtpd_milters' "\$dkim_milter" | |
| _append_to_option_in_postfix_main 'smtpd_milters' '$dkim_milter' |
There was a problem hiding this comment.
I think the shellcheck lint will complain (or did with grep unless I did "\$value" instead of '$value'), and may affect sed? I recall some similar problem when passing strings like this to a function that'd use sed with a variable reference...
@georglauterbach has checked that this is working as intended with repeat runs? I don't think we have any specific test for it. My earlier advice on the original approach to this was to just ensure we reset the config files via a copy at startup, avoiding the issue (so long as the change is not applied again via a setup command or triggered by check-for-changes.sh).
There was a problem hiding this comment.
@casperklein The quoting is done in order to not use # shellcheck disable=SC... above.
@polarathene I can double-check, but it may also be a good idea to ask @wligtenberg whether #3380 was properly solved for him. If so, this would indicate success. I would check nevertheless, but don't know when exactly.
The new function can 1. update/append 2. update/prepend 3. initialize if non-existent options in `/etc/postfix/main.cf` in a safe and secure manner. When the container is improperly restarted, the option is not applied twice.
Co-authored-by: Casper <[email protected]>
georglauterbach
force-pushed
the
scripts/postfix-helper-append
branch
from
3272028 to
a4dd2b7
Compare
|
It turned out there were problem with my method indeed, because the escaping was lost on the way. I have revised the functionality again, and checked whether the fix works. While I was at it, it compacted the code and removed superflous functions again. I rebased on master, please provide a dedicated review again. |
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
LGTM 👍
- 1 suggestion to consider (optional, but seems like an improvement)
- References to
db.shshared as similar functionality exists.- Although my stance is to avoid adapting
db.shto supportmain.cfif it'd complicatedb.shfurther; the PR version here is simpler to grok and maintain in the context of managingmain.cf. - I caution changing
db.shusage of it'ssedhelper, but it may not be equivalent as yours? 🤷♂️
- Although my stance is to avoid adapting
|
@polarathene I applied a patch that I derived after looking at your suggestion. This should include the error handling, but keep the whole function as simple as possible. We also use |
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
I personally think the suggestion would be more maintainer friendly (wishful thinking of the project getting additional maintainers some day..).
If you disagree, @casperklein could weigh in with their opinion, if not I won't fight it and will approve (requested changes instead of approved was only to provide a bit of time to consider the suggestion).
Co-authored-by: Brennan Kinney <[email protected]>
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
Thank you, much appreciated! ❤️
|
Merged too fast, couldn't review. Therefore, see my suggestions in the follow up PR #3503. |
3 participants
Description
Add functionality to safely and securely append to, prepend to or init options in
/etc/postfix/main.cf.Fixes #3482
Open Tasks
docker restartdoes not append the string twice.Type of change
Checklist:
docs/)