chore: LDAP config improvements #3522
Conversation
- `variables-stack.sh` does not need to manage all these extra ENV or store them. They're not used anywhere else. - `saslauthd.sh` is the only consumer of these ENV which are effectively direct key/value mappings, with some defaults provided / inherited. Instead of trying to conditionally support key/value pairs when ENV is set, we could instead use `sed` to delete lines with empty values.
- Drop deprecated support: - `DOVECOT_HOSTS` is an ENV deprecated since v10. - Fallback for missing URI scheme introduced for Dovecot and SASLAuthd in v10. - Adding error log message when no LDAP URI scheme is detected for the supported ENV (when set). - Docs updated for ENV to reflect the mandatory requirement. `mailserver.env` partially synced equivalent sections. - Provided base LDAP configs (for overriding) likewise updated from `domain.com` to `example.com`. - LDAP test updated for required `ldap://` URI scheme. Common ENV shared across LDAP configs hoisted out of the Postfix group.
polarathene
added
area/scripts
service/ldap
area/configuration (file)
kind/update
| # NOTE: It's unlikely this file would already exist, | ||
| # Unlike Dovecot/Postfix LDAP support, this file has no ENV replacement | ||
| # nor does it copy from the DMS config volume to this internal location. | ||
| if [[ ${ACCOUNT_PROVISIONER} == 'LDAP' ]] \ | ||
| && [[ ! -f /etc/saslauthd.conf ]]; then |
There was a problem hiding this comment.
Technically instead of ACCOUNT_PROVISIONER=LDAP, this should be for SASLAUTHD_MECHANISMS=ldap, as the two features serve a separate purpose? 🤷♂️ (although I'm not sure when you'd only have Postfix auth against LDAP while using a different ACCOUNT_PROVISIONER).
There was a problem hiding this comment.
Should we take action here immediately?
There was a problem hiding this comment.
Nah, this is getting reworked by a follow-up and can be addressed by that 👍
|
|
||
| # Create a config based on ENV | ||
| sed '/^.*: $/d'> /etc/saslauthd.conf << EOF |
There was a problem hiding this comment.
What does this sed actually do?
There was a problem hiding this comment.
key: is matched (space could probably be \s*), so that if no value was assigned via the HereDoc input, the line is removed from the output.
Before we had an awkward workaround to avoid keys with empty values, which resulted in multiple blank lines if unset.
There was a problem hiding this comment.
Only adding lines without empty vars.
There was a problem hiding this comment.
I just noticed, for empty vars, ${foo:-} should have been used. ATM that's not a problem. But a good practice, if we want to introduce "set -u" in the future.
| # NOTE: It's unlikely this file would already exist, | ||
| # Unlike Dovecot/Postfix LDAP support, this file has no ENV replacement | ||
| # nor does it copy from the DMS config volume to this internal location. | ||
| if [[ ${ACCOUNT_PROVISIONER} == 'LDAP' ]] \ | ||
| && [[ ! -f /etc/saslauthd.conf ]]; then |
There was a problem hiding this comment.
Should we take action here immediately?
|
Documentation preview for this PR is ready! 🎉 Built with commit: dc7159d |
Description
ldap://when no URI scheme is provided. v13 will better communicate this via error message in the deprecation checks as a precaution. Technically not a breaking change.SASLAUTHD_*ENV are only used to generate a config file. I have removed logic for this invariables-stack.sh, so that onlysaslauthd.shis handling it (and only when using LDAP support). The current defaults / inheritance is preserved (with the exception ofldap://URI scheme fallback)._replace_by_env_in_file()helper method) is presently not compatible with/etc/saslauthd.conf(which we generate during startup), due to different key/value delimiter format. I intend to refactor the approach for these configs and the related ENV support after this PR.ldap://URI scheme and to relocate some LDAP ENV config that is generic, not just specific to Postfix.By using
sedto remove the unset settings, we simplify handling for a portion of this config that relied on updating an ENV variable to include the key (and when unset, just created multiple empty lines).I believe a good approach going forward is to provide all settings in the supported configs upfront, with the
sedremoval approach.I have the configs already defined for this and would like to follow-up with another PR that implements such a change (as detailed here).
Defaults that are non-standard will be dropped in a future PR. With consideration for sane defaults from DMS.
Type of change
Checklist:
docs/)