scripts: cover SASLAUTHD_* variables in start-mailserver.sh
#2562
Conversation
casperklein
added
area/scripts
kind/improvement
There was a problem hiding this comment.
This doesn't seem like the right approach?
The only purpose these ENV appear to serve is to generate a saslauthd config based on container ENV and fallbacks which is only when SASLAUTHD_MECHANISMS has the value ldap:
Config is generated in two approaches, assigning the ENV (or fallback directly), presumably for required config values, and then lines only referencing the ENV that also set the values key if set, otherwise resulting in empty lines:
docker-mailserver/target/scripts/startup/setup-stack.sh
Lines 587 to 610 in b4c49d2
docker-configomat is used presently to create similar files for DOVECOT_* and LDAP_* container ENV settings. This magic under the hood was partially documented in late 2020 but to a maintainer unfamiliar, it's not clear that those ENV were being processed that way. LDAP tests do cover them, and I was only aware of the configomat relation since September 2021.
Apart from some ENV to key mapping mismatches and slight syntax difference, that approach might work, or other approaches that have been used elsewhere to just append/sed update a config file.. Personally I'd rather we drop the configomat submodule at some point.
I'm happy to go forward with the PR regardless if you like, but I think it might be worth adding a comment that makes the maintainer reading aware that this block of ENV isn't serving any purpose in /etc/dms-settings and is just for generating a /etc/saslauthd.conf file for LDAP setups which seem to require SASL auth? (I haven't looked into setting up LDAP)
| @@ -111,6 +111,51 @@ VARS[TZ]="${TZ:=}" | |||
| VARS[UPDATE_CHECK_INTERVAL]="${UPDATE_CHECK_INTERVAL:=1d}" | |||
| VARS[VIRUSMAILS_DELETE_DELAY]="${VIRUSMAILS_DELETE_DELAY:=7}" | |||
|
|
|||
| # SASL specific variables | |||
| VARS[SASLAUTHD_MECHANISMS]="${SASLAUTHD_MECHANISMS:=pam}" | |||
| VARS[SASLAUTHD_LDAP_SERVER]="${SASLAUTHD_LDAP_SERVER:=${LDAP_SERVER_HOST}}" | |||
There was a problem hiding this comment.
SASLAUTHD_LDAP_SERVER is only used to append itself as a value for the ldap_servers: key as one of the lines for a default /etc/saslauthd.conf fallback:
docker-mailserver/target/scripts/startup/setup-stack.sh
Lines 587 to 591 in b4c49d2
The config file, despite it's name is only relevant / used by LDAP configs it seems:
LDAP_SERVER_HOST is not included in VARS or defined anywhere in our scripts.
It's available in:
- Env:
mailserver.envtest/mail_with_ldap.bats(used as container ENV arg)
- Docs (set with example placeholders or referenced as a fallback for another ENV):
README.mddocs/content/config/environment.mddocs/content/config/advanced/auth-ldap.mddocs/content/examples/use-cases/forward-only-mailserver-with-ldap-authentication.md
- Scripts
target/scripts/startup/setup-stack.sh- in_setup_ldap(as a third fallback forDOVECOT_LDAP_MAPPING) and_setup_saslauthd(handled in this PR)
Doesn't seem to be a problem then since it's expected to be a container set ENV?
There was a problem hiding this comment.
Also not covered but seem to otherwise be container ENV that our scripts only use for SASLAUTHD_* ENV fallbacks:
LDAP_BIND_DN-mailserver.envand ENV docs both referenceSASL_LDAP_BIND_DN, but are meant to be referencingSASLAUTHD_LDAP_BIND_DN.LDAP_BIND_PWLDAP_SEARCH_BASE
Other SASLAUTHD_* ENV with the exception of SASLAUTHD_MECHANISMS are only being used to generate config.
Only DOVECOT_* and LDAP_* ENV is processed by configomat.sh here in setup-stack.sh:_setup_ldap:
docker-mailserver/target/scripts/startup/setup-stack.sh
Lines 340 to 374 in b4c49d2
Should we really migrate this logic for storing in /etc/dms-settings? One might argue that ENV config is the wrong approach vs just overwriting an example config and mounting that with other config files?
| @@ -111,6 +111,51 @@ VARS[TZ]="${TZ:=}" | |||
| VARS[UPDATE_CHECK_INTERVAL]="${UPDATE_CHECK_INTERVAL:=1d}" | |||
| VARS[VIRUSMAILS_DELETE_DELAY]="${VIRUSMAILS_DELETE_DELAY:=7}" | |||
|
|
|||
| # SASL specific variables | |||
| VARS[SASLAUTHD_MECHANISMS]="${SASLAUTHD_MECHANISMS:=pam}" | |||
There was a problem hiding this comment.
This is fine, the ENV is specified here clearly as a default instead of baked in as a default via docker image ENV.
It is used later in this script by calling:
which uses the ENV:
docker-mailserver/target/scripts/startup/daemons-stack.sh
Lines 39 to 42 in b4c49d2
Technically the fallback could exist within that function unless we have anything else that would justify storing this information into /etc/dms-settings.
By keeping the ENV in the Dockerfile like it was, the fallback would be implicit and always available, without being dependent upon this script that strives to ensure any ENV fallbacks are accessible to other processes through /etc/dms-settings.
This distinction might be worth having (with documentation for maintainers). ENV usage has become vague as we see in this PR, where we're moving ENV to a common ENV setup and to persist but ignoring the intention?
Dropping this line in favor of restoring ENV SASLAUTHD_MECHANISMS=pam in the Dockerfile? (with some context comment to avoid repeating the misunderstanding?).
|
I am very low on time for the next weeks. I will look into this by the end of may. |
georglauterbach
changed the title
SASLAUTHD_* variables in start-mailserver.sh
My aim with this PR is to streamline our logic. At the moment, not all user configurable ENVs are present in our
But this is also true for most of the other ENVs in that file? Or did I misunderstand you? The file serves two purposes IMO:
Yes. But it can be configured by the user, so it should be in
Not a fan of this. I like the approach from a maintenance point of view, to see all (available) used VARS in one place:
I guess, we looked with different expectations on this PR 😆 My main intend was to streamline our code/logic. Your general concerns (if ENVs are the best way to go to alter config files) are also valid, but from my POV not directly related to this PR. |
Why aren't you including Do you intend to do a follow-up PR covering the Likewise, I noted several other At the very least, perhaps
Yes I noticed it was not, not if it should be. My opinion is that the feature is niche in support (we already have a lack of maintainer support for LDAP itself), and no one has requested these ENV be persisted to The actual config file could be parsed to get the same information and is probably a better source for such information if any other manipulation of the config was to occur. Especially since we've got a conditional to check for existence of the config file, users may be explicitly mounting their own config where those ENV would be unrelated, and any fallbacks assigned misleading, vs the actual information. Restoring the proper guards / dependencies for the ENV being managed would at least help avoid that problem.
I recall That's valid. I don't see it as valid in this case where the sole purpose is use the ENV as values into a config file being generated. Nothing else depends or cares about those ENV, fallbacks are only relevant during the logic to generate config if one does not already exist. Perhaps I'm not thinking of a situation where this would actually be useful. For us, it's expected that any issues provide the ENV used, and if anyone did troubleshoot an issue regarding the feature these ENV support, we'd likely ask them to share I don't believe all ENV we support should live in that file. Personally, if I had the time I would look back through the history of where and why the ENV was contributed, and consider a breaking change PR that drops support for it. It seems more suited to be it's own helper script/command for When/if I find time to go through LDAP support, my intent would be to try isolate the feature support into it's own layer, as right now that support has been rotting and very reliant upon community contributions (we generally only get issues and very few fixes these days for LDAP). You may want to move these lines closer to each other, the first TL;DRI am of the opinion that these ENV do not belong in I will provide approval, on the basis that:
|
Yes. But that's like it was ever since - so no regression: This applies for any ENVs that depends on other ENVs like
If that behaviour should generally be changed, this should be discussed/changed in a separate issue/PR.
Because I overlooked it at first. I am going to add it as well 👍
You are right, they should also be covered. But I don't have immediate plans to do so. Maybe in the long term, I've other ideas that I want to contribute first. This PR already took me more afford than initially planed 😉
Yep, I will add
I think, we just have different expectations about the VARS array and therefore the content of I am the opinion, that having all ENVs in
Why you think it's unnecessary? Without, the value of tl;dr:
We have three options now I guess:
I am clearly for the third option. I also understand your point of view, but I have a different opinion ✌️ |
|
EDIT: Feel free to ignore the comment below and merge. I'll raise a follow-up PR. Original Response
I was pointing out that you're changing the current behaviour by dropping all those dependencies. Technically there is no impact since it's just processing ENV that was otherwise only handled when relevant. Actual logic to use them remains behind the dependencies. Still I'm not fond of unconditionally running the ENV handling.
Fair, but that doesn't mean it should be that way. Just because it's not properly wrapped elsewhere does not justify ignoring it with this PR imo.
I could see it being useful for users to include the output of with reports. Usually they dump the
That's fine. I'm not requesting this PR address that issue in full. Just that since
I meant the ENV did not need to be part of At first I had a double-take when I saw the double -VARS[SASLAUTHD_LDAP_SERVER]="${SASLAUTHD_LDAP_SERVER:=${LDAP_SERVER_HOST}}"
# ...
+SASLAUTHD_LDAP_SERVER="${SASLAUTHD_LDAP_SERVER:=${LDAP_SERVER_HOST}}"
[[ ${SASLAUTHD_LDAP_SERVER} != *'://'* ]] && SASLAUTHD_LDAP_SERVER="ldap://${SASLAUTHD_LDAP_SERVER}"
VARS[SASLAUTHD_LDAP_SERVER]="${SASLAUTHD_LDAP_SERVER}"
I'd rather you don't if you're not covering all of You've said you'd like to tackle that in future, but unsure when. Might as well leave that support blank rather than partial? Since those also need their outdated documented defaults corrected (unrelated to
I think the concern regarding I would still appreciate some inline documentation regarding the ENV. Now that they've been pulled out of the only function that uses them, maintainers may not be aware that is the only relevant location and need to consider the scope of these ENV in future maintenance work? (grep isn't really sufficient for confidence, eg with ENV dependent upon Prior to this PR, it was not obvious to me what the ENV were dependent upon for relevance. As our LDAP support in future remains questionable, I'd prefer that we keep it obvious. Without a conditional wrapping them, I'm also wary of ENV being shuffled around (less of an issue with the Could we please at least go from the vague # Only applies when ENABLE_SASLAUTHD=1 and SASLAUTHD_MECHANISMS=ldap
# SASLAUTHD ENV to generate config when /etc/postfix/saslauthd.conf does not existThat's not much more to add, but vastly improves the communication of purpose for these ENV to maintainers. SASL is used elsewhere beyond the purpose covered here and differs in relevancy. As a maintainer I want to think less and ideally not need to trawl the project or
Or the fourth that I requested, wrap these ENV in a conditional, retaining the current chain of dependencies for their relevance, rather than further ambiguating that.. I'm willing to go with your third option (this PR), with a compromise of better inline documentation (the two line comment suggestion above).
EDIT: Since two other maintainers have approved this and not joined in my review concerns, I'm not going to fight this further. Merge it and I'll raise a separate PR with the documentation concerns I have. |
|
WTF. I wanted to update the branch, not merge it 😱 Edit: I'll provide a follow up PR with the promised changes. |
|
Yeah sorry @casperklein I thought it was good to merge (I couldn't update the branch though, preventing that) and so I had prepared the squash commit for merging 😅 There's a "Revert" button I can click, I assume it will undo the merge? |
No Problem. I already did the follow up PR. |
Description
This PR moves the
SASLAUTHD_variables declaration fromsetup-stack.shtostart-mailserver.shand adds them to theVARSarray.Fixes: #2541 (comment)
Type of change
Checklist:
docs/)