refactor: acme.json extraction#2274
Conversation
The extraction python code is effectively the same, except for what is now `args.requested` var. Extracted out into a python script that can be treated as a utility in the `$PATH` like other helper scripts. It can now be used and optionally tested directly instead of via `helper-functions.sh`. Added an arg parser that provides better experience with help output and error/parsing checks out of the box. Should be easier to maintain now. Updated to be compatible with Python 3, required for upgrade to Debian 11 Bullseye.
Refactored to be less heavy in width, especially with the final nested conditional check, which was extracted out to a separate function call. Added basic error handling when a key lookup fails, which was more useful when removing the nested if statements.
- Only use the conditional for handling the failure if key or cert value is missing from extraction. - Emit an actual warning or success debug log. - Use the FQDN arg `$CERT_DOMAIN` here in place of `$HOSTNAME` and `$1`, since that's what we matched via the python script.
`-h` / `--help` output was accidentally disabled, I forgot to remove this option.
Co-authored-by: Frederic Werner <[email protected]>
This comment has been minimized.
This comment has been minimized.
The test was invalid previously..
…efactor/acme-extraction
The added logs and timing helps debug changedetector service behaviour, especially after multiple changes. The timing from the start and end of a detected change event helps establish how long it took to process which is quite helpful in tests that need to wait for changes.
While fairly verbose, the added comments should assist maintainers to more quickly be informed on the functionality. Added more logs help with debugging.
Without quote wrapping the variables in the for declaration, `*.example.test` did not seem to be attempted. `mail.example.test` would be tried followed by `example.test` if no match was found. I'm a little surprised that shellcheck lint did not pick up on that. I have opted to use an array instead where the lint will actually advise to quote wrap vars.
This comment has been minimized.
This comment has been minimized.
|
If there's anything else I can do to make this PR easier to review, please let me know.
I know the python might be uncomfortable to maintainers here (I'm not a fan of Python myself, but that argparser is much nicer than the shell equivalent I think 😅 ), If you check the commit history it originally is similar to the python embedded in the shell scripts currently (which is the same code repeated twice with a minor tweak for which JSON field to return), I just reworked it to be DRY.
|
| fi | ||
|
|
||
| mkdir -p "/etc/letsencrypt/live/${CERT_DOMAIN}/" | ||
| echo "${KEY}" | base64 -d > "/etc/letsencrypt/live/${CERT_DOMAIN}/key.pem" || exit 1 |
There was a problem hiding this comment.
Just a question, if that is the intended behaviour: When this command fails, then check-for-changes should exit with errorcode 1. Is that correct?
Supervisor will then start check-for-changes again.
There was a problem hiding this comment.
I didn't author this line originally, but I guess that's what happens? 😅
I'm not quite sure how it'd fail if the directory was created and the value is not empty, other than maybe not being valid base64 encoded data for some reason?
It would end up in a bit of a loop then (startup script also calls this method in setup-stack.sh), without an error logged? (or that comes from the base64 decode error prior to exit?
There was a problem hiding this comment.
I'm not quite sure how it'd fail
Thought the same. But was wondering why there is this explicit || exit 1
There was a problem hiding this comment.
without an error logged?
When supervisor restarts a service, you'll see something similar to this at STDOUT:
2021-11-01 23:13:08,348 INFO exited: update-check (terminated by SIGTERM; not expected)
2021-11-01 23:13:08,350 INFO spawned: 'update-check' with pid 3534
2021-11-01 23:13:08,436 INFO success: update-check entered RUNNING state, process has stayed up for > than 0 seconds (startsecs)
There was a problem hiding this comment.
But was wondering why there is this explicit
|| exit 1
It was introduced in July 2020 via this PR, there is no discussion about it or mention in the relevant commit message.
Co-authored-by: Casper <[email protected]>
This comment has been minimized.
This comment has been minimized.
polarathene
added
area/scripts
kind/bug
kind/improvement
3 participants
Description
This has been extracted from #2245 as it does not need to be part of that PR.
It has additionally extracted the python logic to an actual python file that I've refactored and made compatible with Python 3, as Python 2 is EOL and no longer in newer versions of Debian. Required for #2116 to pass tests.
Split into scoped commits with messages if it helps with review :)
Commit Summary:
check-for-changes.shSSL_DOMAINsilently skipping when value has wildcard prefix*.(at least this was known as a bugfix when originally committed in linked PR).helper-functions.sh:_extract_certs_from_acme:$CERT_DOMAIN, aka the FQDN) provided for extraction is empty.$CERT_DOMAINin place of$HOSTNAMEand$1for a consistent value (previously could mismatch, eg withSSL_DOMAINdefined).SSL_DOMAINwith wildcard value for themkdirletsencrypt directory name (wildcard prefix*.is first stripped instead).acme_extract(new python utility foracme.jsonhandling):$PATHlike other helper scripts. It can now be used and optionally tested directly instead of viahelper-functions.sh.Interacting with the python script has following output:
--keyor--certis actually required, so minor mistake by theargparserPython module with it's generated help text above:Type of change
Checklist: