scripts: refactored check-for-changes.sh#2498
Conversation
I refactored `check-for-changes.sh` and used the new log. `_notify` can therefore be deleted as it is used no more. The `_get_date` function was moved to `utils.sh`.
Let'sEncrypt and SSL manual tests were adjusted to the new log. Moreover, ``` assert_output --partial 'Change detected' ``` was dropped because it appears first in the logs, and due to the now more verbose log, we'd need to increase the lenght of the `tail`ed output, but this an unreliable method. I therefore simply dropped this message, and I think this is fine, because the other messages are still fine for testing whether all is well with the script and its functionality.
georglauterbach
added
area/scripts
kind/improvement
|
Feel free to have a look. I'm waiting on #2499 to get merged because this PR holds the |
So here is what happens:
1. The test starts and the mail server logs the message about the
certificate being extracted during startup in the normal logs (all is
well)
2. The `_acme_rsa` runs and extracts (FOR THE SAME DOMAIN) (all is well)
3. In the end, `_acme_wildcard` run, (FOR ANOTHER DOMAIN mind you). This
results in another entry being created under `/etc/letsencrypt/live`
which causes the changedetector to run twice, not once (which is
correct). Now, all is not well. Due to the (in total) third run, the
log does not contain the `'/etc/letsencrypt/acme.json' has changed...`
message anymore when `tail`ing normally (because the `acme.json`
hasn't changed again).
I therefore
1. Added `-3000` to tail more log. This is fine because with the
first test (`_acme_rsa`), we cannot get false positives as it is
the first test; with `_acme_wildcard` I carefully measured the
logs to not have a false positive (now, this test WAS and STILL IS
very sensible to log adjustments - I'm not changing this in this
PR).
2. Added more `sleep` duration for two changedetector runs to
complete without problems. This prevents timing issues in `_acme_wildcard`.
3. Obviously, the restart count needed to be adjusted as well (as
the changedetector now restards thrice and not twice as
expected). Looking at the whole file is an improvement because
now we will _always_ have the total amount of restarts available.
I guess this did not come up due to how the logs were written before and
because of "timing issues". This could and should be resolved in the
future.
…/check-for-changes
|
@polarathene I added a longer commit message for |
|
From commit message:
The third change event shouldn't be occurring? We identified this was triggered by the wildcard cert files, yet the rsa cert files did not trigger a change (as they shouldn't) despite different contents to the ecdsa cert files. The wildcard files should not be triggering a change event either... I prefer we resolve that bug tbh. I detailed all of this with full flow and likely lines that contain the bug in the original PR discussion about this. Getting restart counts from the full log is great, but adjusting the expected restart count in the test to accommodate the bug is not - when we should be able to fix it instead of ignore it. |
So, apparently 1. `check-for-changes.sh` needs HOSTNAME and DOMAINNAME to to be set properly (this is now fixed again, I removed it earlier) 2. POSTMASTER_ADDRESS was not properly set to begin with, so this PR actually also fixes a bug :D The regression should be fixed now and I reverted parts of the test back to their original state.
|
@polarathene I found the regression, and |
Referencing the commit message:
HOSTNAME and DOMAINNAME are needed for the |
I guess they are needed by Keeping them in sync is a whole other task 😉 There is a maintenance concern, but there is currently no need to worry because we do not rely on this and we do not expect these variables to change during the lifetime of the container. |
…/check-for-changes
Moreover, I opted to source `/etc/dms-settings` as a whole to future-proof the script. When the DNS adjustments PRs (that do not exist by now but will exit in the future) are done, we can then remove `_obtain_hostname_and_domainname` because we're already writing the variables to `/etc/dms-settings`. I left instructions in the script in the form of TODO comments.
|
All important changes are included now. This PR is officially ready for review @casperklein @polarathene :) |
Because we now log the date for all messages of the changedetector, we need to `tail` a bit more log than before.
3 participants
Description
Slightly altered
check-for-changes.shand adjusted it to the new log. Removed_notifyas it is now not used anywhere else. I will removeDMS_DEBUGwith a separate PR.Type of change
Checklist:
docs/)