feat: start implementing replication - Add DOVECOT_ITERATE_ATTRS/FILTER ENVs#3426
feat: start implementing replication - Add DOVECOT_ITERATE_ATTRS/FILTER ENVs#3426williamdes wants to merge 3 commits intodocker-mailserver:masterfrom williamdes:ldap-2048-iterate-users
Conversation
williamdes
changed the title
|
I just did a quick look and wondered, if the two newly added ENVs don't need to be added to Does changing those ENVs actually works? |
Changing the ENVs does work because the tests say so and my production ENV too since some years now ;p But most probably it's needed to list users to replicate. |
|
You're right. I found the snipped that handles the ENV change: |
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
This looks fine, except for the test which is not acceptable. Not your fault as I don't expect you to have known why 😅
Most tests have been ported over to our newer conventions which makes it easier to write / maintain tests. LDAP is one of the few that I didn't get around to tackling with my available volunteer time for DMS.
Here you've brought in the same common helper script but from the revised version. You then leverage X_EXTRA_ARGS in a way that it's not meant to be used 😅 (you probably wanted to add the setup helper and call our dedicated method for that instead)
Here's an example of the newer version of tests for setup_file():
docker-mailserver/test/tests/serial/mail_with_imap.bats
Lines 1 to 21 in 7d5c273
LDAP has a little bit extra going on with a custom network and LDAP container also being setup.
I can also see that your new test cases are calling setup_file() directly with additional ENV. setup_file() is only meant to be run once for the whole file, test cases aren't meant to call it again.
setup_file() will do any common setup that's relevant for all test cases before they're run, when you need to run containers with different ENV, you can likewise setup the container in test cases instead of in setup_file():
Then the teardown will happen automatically if our test file has:
And you've set the container name as shown above.
|
Thanks for the feedback @polarathene Since I mostly did follow the actual testing style (of the file) could we merge this PR and cleanup this in another one? |
|
Fine we me, just sayin :) |
Are you actually going to follow-up with it? (in a timely manner) It's not about testing style. I pointed out issues with how you modified the test. You are using the test suite methods itself incorrectly, not just our helpers / conventions. I don't think we can trust the tests are reliable with the current concerns raised. You can wait until end of the month, I may have time to address this myself. Otherwise if someone does merge this PR ignoring my concerns:
|
github-actions
Bot
added
the
meta/stale
polarathene
removed
the
meta/stale
|
Oh gosh I forgot about this |
This is probably not going to happen due to a simple lack of time.. |
Nevermind, still on my todo list |
|
I've been working on the LDAP test today, it'll be refactored and should be reviewed / merged this week sometime. Your PR here can then adapt to the new test. |
Thank you so much |
|
@williamdes while we wait on #3483 to get merged, are you familiar with any other openldap docker images that you can easily automate setup with? The one we're using in our tests is quite outdated (version pinned), and upstream is no longer maintained for 2 years. I have come across a bitnami / vmware image, but it doesn't support |
Yeah of course, I built one for the same reasons Please file issues with anything you want to have for it |
|
The LDAP test refactor PR is merged, I'll revise the portion of your test changes in this PR by the weekend 👍 I managed to resolve the issue I was facing today. I gave your image a look but didn't feel it was a better choice for DMS maintainers to rely on. Detailed notes below.
Doesn't seem that way. I looked through the reference links you provided but stopped by this point: docker run --rm --name ldap-test \
--env LDAP_ADMIN_PASSWORD=admin --env LDAP_CONFIG_PASSWORD=admin --env LDAP_MONITOR_PASSWORD=admin \
--env LDAP_BASE_DN='dc=example,dc=test' \
--env LDAP_AUTH_BASE_DN='ou=people,dc=example,dc=test' \
--volume '/tmp/ldifs/:/ldifs/:ro' \
--hostname 'ldap.example.test' \
botsudo/docker-openldapIt wanted me to configure more ENV and refused to exit with The image itself seems to have a lot more going on package / config wise, I would have liked to get the Alternative (most popular openldap image actively maintained):
|
|
Can you open an issue on my image? If I understand you are missing loading ldiff files at startup? You "should not need" to load more of them since they are pre loaded by myself, the postfix schema and all. |
| iterate_filter = (objectClass=PostfixBookMailAccount) | ||
| iterate_attrs = mail=user |
There was a problem hiding this comment.
On production I had removed the user patches lines to insert the following lines
I am starting to think that only adding ENVs like DOVECOT_ITERATE_ATTRS may not be enougth to have the magic work. Maybe default values in this file are an essential part too
There was a problem hiding this comment.
What is not working for you?
You just want the test-cases you added to pass with the expected output correct? I'll ensure that happens 👍
mail=user means that the identifier in the LDAP object is "mail" attr. This is also according to other uses: LDAP_QUERY_FILTER_USER And with the reality of a production ENV
On it |
|
Documentation preview for this PR is ready! 🎉 Built with commit: 80818b3 |
| local CUSTOM_SETUP_ARGUMENTS=( | ||
| --env DOVECOT_ITERATE_ATTRS='uniqueIdentifier=user' | ||
| ) |
There was a problem hiding this comment.
for reasons that I ignore, this does not seem to give the options to the container creation process
Any help would be very welcome 🙏🏻
There was a problem hiding this comment.
You will need to provide the array CUSTOM_SETUP_ARGUMENTS on container startup to the function that creates the container. Functions like _run_in_container do not take it into account.
If you want to have it added to the ENV of the container, you'll need to do so at the top of the file where the container is created I guess (or wherever it is created). Probably
and
There was a problem hiding this comment.
Yeah, we need to create a new container instance when we want to use different ENV like your test-cases do.
I've converted the test file into the new format we use, but it is the kind that sets all containers up in the main setup() method, whereas you'll see some other tests elsewhere like hostname.bats needs to run different variations for different test cases so each test-case is making it's own container instance, and calling many functions as "test-cases" (a workaround I opted for).
We could technically have multiple .bats files instead, but for such small test-cases like yours that seems unnecessary. I will adapt this LDAP test to support both approaches, but the main annoyance I had was with the ENV setup / sharing.
Technically you could right now define a common base ENV and combine like done here:
Then you can create a container with different ENV like this small test-case:
You'd just expand an array like in the prior snippet I showed for sharing the base ENV each container is needing. Do note that you don't have to call it CUSTOM_SETUP_ARGUMENTS (although you should when you can), we just have to match the string value of the variable name that is passed to _common_container_setup().
Plan is that after converting all tests to the new format, to do another pass that moves to compose.yaml, which may instead rely on ENV files or a different way to modularize these config variants.
There was a problem hiding this comment.
Also note that a PR is about to get merged which changes the test uniqueIdentifier to userID (uid).
Although my goal is iterating towards mail attribute, at least as the login name but presently the test and DMS are not properly configured for this, hence why your PR is delayed. I'd like to get that transition for our LDAP tests and docs addressed first, but am happy to go ahead with this Dovecot support for v13 after I can be more confident in our LDAP support and test coverage :)
Yeah sorry, I've had some other events need my attention and within DMS I've been trying to queue some additional LDAP related changes before adjusting the test a bit more before meeting your requirements needed here. Almost there 😅 |
polarathene
left a comment
polarathene
left a comment
There was a problem hiding this comment.
You can probably use this review feedback to get the tests working/passing, but like I've said I'm combing through LDAP support and history within DMS and may introduce some breaking changes before returning to this PR.
I don't feel there is a rush to merge this, unless you have a follow-up you want to push through for v13 as well? (if so, take my feedback and update the LDAP tests for me and we'll look into merging it sooner)
| iterate_filter = (objectClass=PostfixBookMailAccount) | ||
| iterate_attrs = mail=user |
There was a problem hiding this comment.
What is not working for you?
You just want the test-cases you added to pass with the expected output correct? I'll ensure that happens 👍
| local CUSTOM_SETUP_ARGUMENTS=( | ||
| --env DOVECOT_ITERATE_ATTRS='uniqueIdentifier=user' | ||
| ) |
There was a problem hiding this comment.
Yeah, we need to create a new container instance when we want to use different ENV like your test-cases do.
I've converted the test file into the new format we use, but it is the kind that sets all containers up in the main setup() method, whereas you'll see some other tests elsewhere like hostname.bats needs to run different variations for different test cases so each test-case is making it's own container instance, and calling many functions as "test-cases" (a workaround I opted for).
We could technically have multiple .bats files instead, but for such small test-cases like yours that seems unnecessary. I will adapt this LDAP test to support both approaches, but the main annoyance I had was with the ENV setup / sharing.
Technically you could right now define a common base ENV and combine like done here:
Then you can create a container with different ENV like this small test-case:
You'd just expand an array like in the prior snippet I showed for sharing the base ENV each container is needing. Do note that you don't have to call it CUSTOM_SETUP_ARGUMENTS (although you should when you can), we just have to match the string value of the variable name that is passed to _common_container_setup().
Plan is that after converting all tests to the new format, to do another pass that moves to compose.yaml, which may instead rely on ENV files or a different way to modularize these config variants.
| local CUSTOM_SETUP_ARGUMENTS=( | ||
| --env DOVECOT_ITERATE_ATTRS='uniqueIdentifier=user' | ||
| ) |
There was a problem hiding this comment.
Also note that a PR is about to get merged which changes the test uniqueIdentifier to userID (uid).
Although my goal is iterating towards mail attribute, at least as the login name but presently the test and DMS are not properly configured for this, hence why your PR is delayed. I'd like to get that transition for our LDAP tests and docs addressed first, but am happy to go ahead with this Dovecot support for v13 after I can be more confident in our LDAP support and test coverage :)
| @test "dovecot: (ENV DOVECOT_ITERATE_FILTER=(objectClass=PostfixBookMailAccount)) ldap email listing works" { | ||
| local CUSTOM_SETUP_ARGUMENTS=( | ||
| --env DOVECOT_ITERATE_FILTER='(objectClass=PostfixBookMailAccount)' | ||
| ) | ||
|
|
||
| _run_in_container doveadm user '*' | ||
| assert_success | ||
| assert_line --index 0 '[email protected]' | ||
| assert_line --index 1 '[email protected]' | ||
| assert_line --index 2 '[email protected]' | ||
| _should_output_number_of_lines 3 | ||
| } |
There was a problem hiding this comment.
Here's an example of what this would look like from the advice given in my review feedback:
@test "dovecot: (ENV DOVECOT_ITERATE_FILTER=(objectClass=PostfixBookMailAccount)) ldap email listing works" {
# Container setup:
export CONTAINER_NAME='dms-test_ldap_dovecot-iter3'
local CONTAINER_ARGS_ENV_CUSTOM=(
"${CONFIG_CONTAINER_BASE[@]}"
--env DOVECOT_ITERATE_FILTER='(objectClass=PostfixBookMailAccount)'
)
_init_with_defaults
_common_container_setup 'CONTAINER_ARGS_ENV_CUSTOM'
# Actual test-case:
_run_in_container doveadm user '*'
assert_success
assert_line --index 0 '[email protected]'
assert_line --index 1 '[email protected]'
assert_line --index 2 '[email protected]'
_should_output_number_of_lines 3
# Cleanup, removes test-case container:
_default_teardown
}The cleanup is just force removing the container:
docker-mailserver/test/helper/setup.bash
Lines 198 to 204 in e9f04cf
Setup is like we do in setup(), except we need to export the custom args as a common base var to expand / import here. It's important to give each container a unique name to identify it, I chose to use the name variant dovecot-iter3 as it's the 3rd test-case container for testing against that ENV (or 2nd if we don't create a separate container for the default test).
Setting the name will ensure the helper methods used will know which container to use implicitly.
|
Once this PR is merged, adding your LDAP test cases should be pretty straight-forward 😁
Alternatively I'll take care of it once that PR is merged. |
|
I am considering a different approach that should meet your needs for these two settings, and any other users future needs for missing settings that these configs are meant to support. Our current approach is a bit awkward and could be improved both for the user and maintainers. @williamdes how important is the ENV support to set these two settings vs including a file with them? I understand your request for this was to avoid the Only Postfix LDAP config files are supported in UPDATE:: For ENV replacement at least with the new template approach I'm going to open a PR for, it'd be handled by this little dance: ENV_PREFIX='DOVECOT_'
ENV_TEMPLATE=/dms/ldap/dovecot.tmpl
# Ensures that envsubst is only run with ENV from the target prefix.
# Those ENV were provided as a generated `.env` file (with prefix dropped by sed)
# to zenv which runs the envsubst command within that constrained environment.
env --ignore-environment \
./zenv --file <(env | grep "^${ENV_PREFIX}" | sed "s/^${ENV_PREFIX}//") \
envsubst < "${ENV_TEMPLATE}"
# stdout now provides the transformed template to be layered with any potential user supplied file config.That's viable I guess 😅 |
|
This pull request has become stale because it has been open for 20 days without activity.
|
github-actions
Bot
added
the
meta/stale
williamdes
added
stale-bot/ignore
georglauterbach
added
the
meta/feature freeze
georglauterbach
removed
the
meta/feature freeze
|
Closing this one. My brain forgot what was left todo. Feel free to pick stuff out if needed |
|
Thank you for your work on LDAP support @polarathene ! Please do mention me directly on PRs so I can review them if you want. Having an easier way to define ENVs to create the dovecot config keys
The only goal was to remove more lines from my patch file :) |
|
Thank you for closing this as it has become stale! This saves me the maintenance of stale PRs :) |
#3524 is the improved approach to what this PR was tackling. It enables support more broadly. Feel free to review it or suggest feedback for some concerns I've noted. It's only really blocked by my rewrite of the docs for the feature which I've kept on-hold due to very low time right now. Hoping to have a week or so for dedicating to DMS later this month. |
4 participants
Description
I added two ENVs needed to list users: #2048 (comment)
Ref #2048
Type of change
Checklist:
docs/)