check-for-changes: performance improvements + wait for settle

[NorseGaud]

Description

See: #2098

This PR will:

• Have check-for-changes.sh wait for changes to "settle" (checksum comparison has to not have changed over two different checks with 5 second gaps between) before modifying and restarting anything
• Added backgrounding of processes and using wait throughout instead of sleeps for more efficiency
• Removed the while loop from addmailuser due to check-for-changes.sh logic changes

This is, in my opinion, critically needed for an API to be performing changes -- and lots of them all at once! See docker-mailserver/docker-mailserver-admin#3 for more discussion about this.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (non-breaking change that does improve existing functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• [] I have made corresponding changes to the documentation (README.md or the documentation under docs/)
• If necessary I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[NorseGaud]

Forgot to draft this... again... Sorry... It's still a WIP

[NorseGaud]

It looks like both the CI and my local machine are getting the following (running with new verbose flag):

✗ begin 186 checking tls: cipher list - rsa intermediate
   (from function `assert_success' in file test/test_helper/bats-assert/src/assert.bash, line 114,
    from function `collect_cipherlist_data' in file test/security_tls_cipherlists.bats, line 154,
    from function `check_ports' in file test/security_tls_cipherlists.bats, line 86,
    in test file test/security_tls_cipherlists.bats, line 47)
     `check_ports 'rsa' 'intermediate'' failed
   $ docker run -d --name tls_test_cipherlists --volume /Users/norsegaud/docker-mailserver/test/duplicate_configs/security_tls_cipherlists.bats/:/tmp/docker-mailserver/ --volume /Users/norsegaud/docker-mailserver/test/test-files/ssl/example.test/:/config/ssl/:ro --env DMS_DEBUG=0 --env ENABLE_POP3=1 --env SSL_TYPE=manual --env SSL_CERT_PATH=/config/ssl/cert.rsa.pem --env SSL_KEY_PATH=/config/ssl/key.rsa.pem --env TLS_LEVEL=intermediate --network test-network --network-alias example.test --hostname mail.example.test --tty mailserver-testing:ci
     f7d78f74fc3eedaa1277f63d9b7a2d06f22fbae7705f75b842b56bed71fce40b
   [ TASKLOG ]  mail.example.test is up and running
   $ docker run --rm --user 501:20 --network test-network --volume /Users/norsegaud/docker-mailserver/test/test-files/ssl/example.test/:/config/ssl/:ro --volume /var/folders/vb/pjyhgj4s491_k0t025_j56y00000gn/T//results/rsa/intermediate/:/output --workdir /output drwetter/testssl.sh:3.1dev --quiet --file /config/ssl/testssl.txt --mode parallel --overwrite --preference
     standard_init_linux.go:228: exec user process caused: permission denied
   
   -- command failed --
   status : 1
   output : standard_init_linux.go:228: exec user process caused: permission denied
   --

It seems like --user 501:20 is the issue as it runs without it. The comment says

--user "<uid>:<gid>" is a workaround: Avoids permission denied write errors for json output, uses id to match user uid & gid.

@polarathene, I'm going to see if anything else fails after removing it but I wanted to see if it's ok if it's removed (as long as everything else passes).

[polarathene]

--user 501:20 --network test-network --volume /Users/norsegaud/

That is odd looking uid and gid values, and with that volume is this Windows/WSL? That context seems to be missing in the CI tests (due to lack of verbose flag I guess?)

But this other volume looks more of an issue:

--volume /var/folders/vb/pjyhgj4s491_k0t025_j56y00000gn/T//results/rsa/intermediate/:/output

I'm not familiar with this mount, but that T//results looks a bit broken path wise? I don't think it should have two / in a row like that?

Chances are the --user may be misleading here as the cause, and rather the path to write to is invalid due to some change in this PR? I believe that is meant to be temporary output for the testssl container to write JSON to.

I think my test was one of the only ones to write to such a location which might be why it's failing in particular, if so it'd be better to fix whatever change has introduced that breakage, and not quick fix it by modifying the temporary files location used in the test.

[polarathene]

I believe whatever changes have been introduced have broken the functionality of ${BATS_TMPDIR} providing a valid path prefix here:

docker-mailserver/test/security_tls_cipherlists.bats

Lines 22 to 23 in f333740

	# `${BATS_TMPDIR}` maps to `/tmp`
	export TLS_RESULTS_DIR="${BATS_TMPDIR}/results"

EDIT: If you are noticing any issues with Windows/WSL running the tests that are inconsistent with the CI, it might be another reason in favor of running bats from docker? I was looking into that at some point, not entirely sure if it'd resolve any issues with volume mounts though (a throwaway data volume that is a container instead of bind mount on the host filesystem might also work?).

[NorseGaud]

Thanks @polarathene , appreciate your insight. So, I rolled bats back to 1.3.0 and it works. It looks like 1.4.0 introduced a change to BATS_TMPDIR: bats-core/bats-core#410 (comment)

How the hell this didn't fail in #2095 is beyond me... Might be good to have @casperklein and @georglauterbach take a look if they get a few minutes. I'm unsure about how the CI is all set up TBH. IMO this shouldn't have passed...

[NorseGaud]

Please ignore the bats submodule for now. I'm using that to gain visibility into the tests and will revert it soon.

[georglauterbach]

LGTM 👍

@casperklein or @wernerfred feel free to merge this PR if you approve it :)

[georglauterbach]

I will go ahead and merge this :)

[casperklein]

I hadn't time to review this yet. Otherwise I had approved it..

[wernerfred]

Dito ⌛

[casperklein]

I think the sleep here was, because service postfix start exit (0) successfully pretty fast, however postfix needs some time to come up completely.

service postfix start &
wait

is equal to just service postfix start ?

I am not sure if it's related, but I read an issue/PR this week, about our CI behaving "flacky" again. Maybe this is the reason?

[NorseGaud]

Good points -- I'll play around with it and see what I find.

[casperklein]

I know it was there before, but do some know what the -- are for? I couldn't find something in the man page.

[NorseGaud]

A double-dash in a shell command signals the end of options and disables further option processing.

[georglauterbach]

Commonly used for example when you're trying to grep something that starts with a double dash, you'd write

grep -E -- "--to-grep" $FILE

[casperklein]

I knew that. I was wondering, if in that case, it has another purpose. Because it seems to me unnecessary here.

PS:

A double-dash in a shell command signals the end of options and disables further option processing.

I wouldn't rely on that, as not every command implements that logic 😞

[NorseGaud]

I never use it personally :)

[casperklein]

Inspired by your other changes, I wonder if this one liner would work too:

wait $(</var/spool/postfix/pid/master.pid)

[casperklein]

Unfortunately, wait only works on child processes of the current shell:

# wait $(</var/spool/postfix/pid/master.pid)
bash: wait: pid 3108 is not a child of this shell 

However https://stackoverflow.com/a/41613532/568737 suggests tail instead (which works):

tail -f --pid="$(</var/spool/postfix/pid/master.pid)" /dev/null

We could replace the while loop with that.

[NorseGaud]

Sounds good -- including in #2134

[casperklein]

I am unsure. This could lead to problems on MacOS? I don't have a mac to test, but the stackoverflow posts suggest this only for linux.
On the other hand, we don't support MacOS officially. What do you think @docker-mailserver/maintainers?

[NorseGaud]

The code doesn't run on mac, it'll run inside of the linux contianer. Shouldn't be an issue.

[casperklein]

You are absolutely right.