When review comments have been resolved, on the Web UI it seems notifications don't properly take you to the discussion in my experience, so I'm instead linking to those and quoting them instead since wading through the history to find the actual updated sub-thread can be annoying 😅

I could probably have marked them unresolved instead, but the actual feedback was addressed.

Review feedback comment: #2716 (comment)

There is LDAP docs that advise using rimap to authenticate against Dovecot instead (although I don't know if there is any benefit to that vs using our default Dovecot SASL instead of the Cyrus SASL (saslauthd)).

Regardless, I don't see any documentation or scripts that are intended to support this pam default. It may be legacy / redundant from the original PR introducing LDAP auth (where the contributor wanted to authenticate with Kopano instead of Dovecot).

Whether or not we should use rimap should be decided elsewhere though, I think :)

By "LDAP docs", I meant our own doc pages for LDAP.

Earlier in the comment I was pointing out that we don't have any test, documentation, or scripts usage beyond the ENV fallback (which was originally in the Dockerfile) for pam. Only rimap is used in a test and mentioned in our docs in LDAP pages.

So we already use rimap in that sense. I wasn't suggesting any change be made regarding that, just adding information for us to be aware of 😅

Review feedback comment: #2716 (comment)

This is appreciated, thank you :)

If it wasn't clear my appreciation was for the referenced/highlighted change:

  if [[ ${ENABLE_SASLAUTHD} -eq 1 ]]
  then
    _environment_variables_saslauthd
    _register_setup_function '_setup_saslauthd'
  fi

as previously the ENV portion was not guarded with ENABLE_SASALUTHD=1 check, and I liked that you had more cleanly grouped/separated the relevant feature... but then you blended it into LDAP 😂 (still better handled with the guard though at least!)

There's also a portion of the _setup_saslauthd() method that is specific / dependent upon the SASLAUTHD_LDAP _* ENV for a fallback config generation. No change needs to be done for that in this PR, but might be worth addressing in a follow-up PR

Addressing _setup_saslauthd should be done in another PR, right :)

I wasn't sure if you were asking a question about doing this in another PR, or just acknowledging that it should be.

A follow-up PR would be good if you have the time for it 👍

I have not resolved the comments this time in case there is more to them. I added the _environment_variables_saslauthd function back to the code. Moreover, I did a more fine grained distinction within the general ENV setup function - I hope it's not too fine grained.

I also added a stub (not enabled by now) for a future DNS-related-code refactoring. The DMS_FQDN variable was introduced. All of this is commented though, so no interference with the current code.

LGTM 👍 The only thing I am personally not completely confident with is the var handling in variables.sh.

While I understand your intention, I think it's too fine grained and adds more complexity than benefit.

It seems also not completely consistent:

You grouped the var assignment by topic. ENABLE_SPAMASSASSIN is handled in _handle_variables_anti_spam which makes sense when you want all SPAM related var assignments in one place. On the other hand, there is a separate _handle_variables_enable function, which handles ENABLE_* vars outside of their respective functions, e.g. ENABLE_FETCHMAIL. Other Fetchmail related vars are handled in _handle_variables_miscellaneous.

A much simpler approach would be going back to one big list, but change some ENV namings to have them easily in order:

Old:

...
VARS[ENABLE_FETCHMAIL]="${ENABLE_FETCHMAIL:=0}"
...
...
...
VARS[FETCHMAIL_PARALLEL]="${FETCHMAIL_PARALLEL:=0}"
VARS[FETCHMAIL_POLL]="${FETCHMAIL_POLL:=300}"
...

New:

...
VARS[FETCHMAIL_ENABLE]="${FETCHMAIL_ENABLE:=0}"
VARS[FETCHMAIL_PARALLEL]="${FETCHMAIL_PARALLEL:=0}"
VARS[FETCHMAIL_POLL]="${FETCHMAIL_POLL:=300}"
...

You see at the first look, all related fetchmail vars, instead of checking different functions.

The "big list" could also be grouped by adding empty lines and/or by adding comments.

The concern may be valid, but don't go changing ENV for now as this is for 11.2 and would be a far larger change to make and review the renaming effort.

I'm mostly interested in the other methods separating out the anti-spam, network, ldap and sasl. I don't see any issue with function wrapping for the grouping.

If you're concerned about sorting order you can looking with CTRL + F whatever you're searching for, or if the ENV set in /etc/dms-settings which is already sorted. I personally did not find the current list which was preferring sorted ENV any better to reason with and appreciate the more clearly defined scopes.

The concern may be valid, but don't go changing ENV for now as this is for 11.2 and would be a far larger change to make and review the renaming effort.

Yeah that was just a general proposal / mind game and if chosen, something for the next major release.

I personally did not find the current list which was preferring sorted ENV any better

I do. More information on less space. Creating functions to call them only once makes no sense in that case. Especially in comparison to any multi purpose script file, we have a dedicated file here just for variable assignments. So using functions just for grouping is not necessary. Just go with something like this (KISS --> Keep it simple, stupid):

# LDAP stuff
VARS[LDAP_foo]
VARS[LDAP_bar]
VARS[LDAP_baz]

# Antispam stuff
VARS[Antispam_foo]
VARS[Antispam_bar]
VARS[Antispam_baz]

If I am overruled here, at least improve the thing in regards of consistency (see example with fetchmail).

Also Antivirus/Antispam/Security is currently completely mixed in _handle_variables_anti_spam and should have their own functions instead, e.g.

• ClamAV/Amavis --> Antivirus or _handle_variables_miscellaneous
• Fail2ban and others to --> Security or _handle_variables_miscellaneous
• Logrotate/Logwatch//PFLogsum/SUPERVISOR_LOGLEVEL --> Logging

Yeah that was just a general proposal / mind game and if chosen, something for the next major release.

We could definitely look into a breaking change (probably with v13 where we might tackle the HOSTNAME/DOMAINNAME ENVs to DMS_FQDN) to get more consistent naming. Many of the ENV is from different contributors/maintainers over the years and not necessarily the good naming 😅

I'd also like to find time for addressing the SSL ENV support, but I may be too busy to invest time into that until early next year.

I do. More information on less space. Creating functions to call them only once makes no sense in that case.

I can understand that, but the added verbosity is a little helpful IMO.

I see that we've never really had decent separation here in the past 6 years it's just been a sorted list of ENV that accumulated.

When you migrated the SASLAUTHD_ ENV and introduced the LDAP ones, you disregarded my concern about omitting some information for the dependency/relationship to be more apparent which has shown to cause confusion with maintainers already.

Splitting these off into their own function scope instead of a comment allows for this:

  if [[ ${ENABLE_SASLAUTHD} -eq 1 ]]
  then
    _environment_variables_saslauthd
    _register_setup_function '_setup_saslauthd'
  fi

You don't have to scan through the list of ENV among the rest to know they're not default ENV but feature gated, and it's much more clearer where these are going to be used next.

It's now very clear that the bulk of SASLAUTHD_ is only relevant when both LDAP and SASLAUTHD features are active. And that while SASLAUTHD_ may reference LDAP_ ENV when available as fallbacks, that those LDAP ENV are not specifically related to the SASLAUTHD_ feature (or as was misunderstood by two maintainers, SASL in general).

These nested scopes come with additional inline documentation related to that scope, a debug log message and can be only added to VARS when relevant (no unhelpful noise in /etc/dms-settings for the majority of users not using such a feature). All big wins for maintainers I think?

I know you were in favor of the flat list (as the current logic does) and having all ENV state be stored into /etc/dms-settings regardless of if it's used or not. This was mostly a workaround introduced due to a test for quotas you added that of several solutions available you decided was most appropriate. With the var deriving scripts split out, you probably could call that instead of relying on /etc/dms-settings now, so having that file only store relevant ENV configured is likely for the best going forward.

If we moved the ENABLE_ prefix to _ENABLE suffix as you suggested, that would may sometimes wind up interleaved in an alphabetical sort? Personally I prefer the ENABLE_ prefix, or at least avoiding alphabetical sort so that it's at the top of the related ENV.

If an issue reporter shares /etc/dms-settings it may also be preferable to glance over all ENABLE_ related values in one area due to the sorting, than to wade through the whole list to see what is enabled/disabled?

If I am overruled here, at least improve the thing in regards of consistency (see example with fetchmail).

I'm all for consistency 👍

Also Antivirus/Antispam/Security is currently completely mixed in _handle_variables_anti_spam and should have their own functions instead

We could definitely improve this further, although I'm fine with that being tackled in a follow-up PR, but as we've both reviewed now, additional commits that address that in this PR is ok too.

This just seemed to be moving in a direction that was more desirable for me as a maintainer. If we want new contributors, they should be able to ideally figure out what ENV relates to a feature / topic vs the alphabetic sorting we have presently.

Personally as part of tackling the multi-stage Dockerfile build, I'd like it more if we can better isolate features further beyond just ENV. That'd serve as improved documentation in itself (for new contributors that could hopefully become maintainers, it's important that friction is reduced to grok how everything fits and flows together).

I'd like to look into ClamAV being it's own stage in the Dockerfile, that pulls from the official image and gets the DB from that. And if possible, scope that stage that it's very clear what belongs to supporting that feature, that the stage itself could be dropped to remove the feature entirely if desired.

I want that mostly for Amavis and SpamAssassin, which may one day get swapped for rspamd instead for example. And to simplify what packages are installed for what (currently that's another mess with alphabetical sorting + fail2ban install), which will make it easier to be more flexible on base image choice and using tools like dive or optimizing builds.

Related to the discussion of future breaking changes with ENV rework, I want to reference this comment from another issue I made.

Summary:

Personally all the configomat ENV and the SASLAUTHD_LDAP ENV used to generate/modify config files doesn't belong in the project and could be deferred to a separate setup step.

If I find time to polish up the Rust port, it could be further adapted to accommodate our related shell scripts as a single CLI binary, or bundled with those shell scripts.

We can provide/maintain it as a separate project with docs pointing users to that, or include it in the DMS image with a setup sub-command to support it (or docs for use with user-patches.sh).

Will take care of this when I'm back from vacation. In the meantime, I'd go with a middle-way, and remove the function-in-function calls while staying true to the sorting (mostly). Definite want to keep the functions for LDAP and SASLAUTHD though. I believe this to be a good compromise between readability and KISS.

when I'm back from vacation.

Do you have an ETA on that? I have a PR that introduces ENV but if you're going to be away for more than a week, perhaps I should submit the PR and have this one resolve the conflict if it's merged prior?

when I'm back from vacation.

Do you have an ETA on that? I have a PR that introduces ENV but if you're going to be away for more than a week, perhaps I should submit the PR and have this one resolve the conflict if it's merged prior?

I will be able to work on this maybe on Sunday. Feel free to submit your PR, I don't mind resolving conflicts:)

Documentation preview for this PR is ready! 🎉

Built with commit: d459597