tests(feat): Complete rewrite of letsencrypt tests #2286
Conversation
polarathene
added
area/security
area/tests
kind/improvement
Easier to grok what is different between configurations.
- Container name usage replaced with variable
- Volumes defined earlier and redeclared when relevant (only real difference is `VOLUME_LETSENCRYPT`)
- Contextual comment about the `acme.json` copy.
- Quoting `SSL_TYPE`, `SSL_DOMAIN` and `-h` values for syntax highlighting.
- Moved `-t` and `${NAME}` to separate line.
- Consistent indentation.
Extracts out repeated test logic into methods
- Preparation step for shifting out the container configs to their own scoped test cases. Split into multiple commits to ease reviewing by diffs for this change. - Re-arrange the hostname and domain configs to match the expected order of the new test cases. - Shuffle the hostname and domainname grouped tests into tests per container config scope. - Collapse the `acme.json` test cases into single test case.
- Shifts the hostname and domainname container configs into their respective scoped test cases.
- Moving the `acme.json` container config produces a less favorable diff, so is deferred to a follow-up commit.
- Test cases updated to refer to their `${CONTAINER_NAME}` var instead of the hard-coded string name.
Final commit to shift out the container configs. - Common vars are exported in `setup_file()` for the test cases to use without needing to repeat the declaration in each test case. - `teardown_file()` shifts container removal at end of scoped test case.
- `CONTAINER_NAME` becomes `TEST_NAME` (`common.bash` helper via `init_with_defaults`). - `docker run ...` and related configuration is now outsourced to the `common.bash` helper, only extra args that the default template does not cover are defined in the test case. - `TARGET_DOMAIN`establishes the domain folder name for `/etc/letsencrypt/live`. - `_should*` methods no longer manage a `CONTAINER_NAME` arg, instead using the `TEST_NAME` global that should be valid as test is run as a sequence of test cases. - `PRIVATE_CONFIG` and the `private_config_path ...` are now using the global `TEST_TMP_CONFIG` initialized at the start of each test case, slightly different as not locally defined/scoped like `PRIVATE_CONFIG` would be within the test case, hence the explicit choice of a different name for context.
- Test case comment descriptions. - DRY: `docker rm -f` lines moved to `teardown()` - Use `wait_for_service` helper instead of checking the `changedetector` script itself is running. - There is a startup delay before the `changedetector` begins monitoring, wait until it ready event is logged. - Added a helper to query logs for a service (useful later). - `/bin/sh` commands reduced to `sh`. - Change the config check to match and compare output, not number of lines returned. Provides better failure output by bats to debug against.
This just extracts out existing logic from the test case to functions to make the test case itself more readable/terse.
No changes, just moving logic around and grouping into inline functions, with some added comments.
This also required copying the source files to match the expected letsencrypt file structure expected in the test/container usage.
No longer necessary, using the `example.test/` certs instead. These letsencrypt certs weren't for the domains they were used for, and of course long expired.
Add more maintainer comments, rename some functions.
Finally able to add more test coverage! :) - Two new methods to validate expected success/failure of extraction for a given FQDN. - Added an RSA test prior to the wildcard to test a renewal simulation (just with different cert type). - Added extra method to make sure we're detecting multiple successful change events, not just a previous logged success (false positive).
Covers all ports (except POP) and correctly tests against expected verification status with new `example.test` certs. The `FQDN` var will be put to use in a follow-up commit.
Can be useful for other TLS tests to utilize.
polarathene
force-pushed
the
tests/rewrite-ssl-letsencrypt
branch
from
d4c86f5
to
b12e4c3
Compare
This comment has been minimized.
This comment has been minimized.
There was a mismatch between the output and expected output between these two files "find key for" and "find key & cert for". Changed to "find key and/or cert for" to make the warning more clear that it's issued for either or both failure conditions.
|
I know everyone is quite busy, no rush but one more review is needed so pinging maintainers for anyone that finds the time to spare. |
There was a problem hiding this comment.
LGTM (from an overall perspective - could be that there are some minors but they can be adressed later) and as far as test are passing should be fine too.
Thanks @polarathene for putting this together and adding comments like a pro. Future contributors will Thank you, i'm Sure.
|
Cheers to you both for taking a look over! 😀 @casperklein did you want to review this as well, or am I good to merge? :) |
|
Thx for asking. Normally I am happy to review all. But I'll definitely have no time in the next two weeks for a review. Feel free to merge 👍 |
|
Merged it myself because I have another upcoming PR that I did not want to rebase :D I figured this was okay :) |
Does that also apply to to the remaining
I assume that was not the housekeeping one you opened, since that would make no sense 😕 Besides the test file itself, there wasn't really much changes elsewhere in this PR that it should require rebasing? (not that I could see that being a problem before pushing a PR if needed) I generally prefer merging these larger PRs of mine since I tend to avoid a merge commit message with all of the PR commit messages concatenated by slimming it down to only the main highlights that are relevant; often from a part of my opening PR message like the summary. Too late now as that would now require rewrite of history on master :( Ah well, no worries 😅 |
|
I will leave merging your commits to yourself in the future, sorry. |
|
@georglauterbach don't worry about it, all good :) I was just a little confused what the rush was regarding avoiding a rebase (I thought that is what all the merge commits from master to each PR was removing the need for?). |
Indeed, but I prefer rebasing over merging in some occurrences. Not that it mattered too much here :) |
No.
The more reviews, the better 👍 But I doubt, you'll find one 😉 After three weeks some would probably already did a review. |
Description
This has been extracted from #2245 as it does not need to be part of that PR.
It is rather dependent upon other PRs split out ( #2274 #2275 #2278 #2279 #2284 ) from the referenced PR #2245 above being merged to successfully pass the test suite.(EDIT: All merged)Summary: This test still has the same coverage, with some additional tests covered and corrected/improved.
common_containerhelper functions fromtest_helper/common:This minimizes the initial setup and cleanup at the end of tests, highlighting only what's relevant.
Many of which can be used for the other TLS tests at a later stage.
Along with some TODO items that still need to be addressed. Should be easier to grok the test flow.
test/config/letsencryptcerts have been removed from the project:Replaced in the test with
example.testcerts, which aren't expired and are better tested against.For ease of review via diffs, I have carefully staged out additional commits to show an iterative process of refactoring the original test into better shape before it adopts the
common_containerapproach, and then converts toexample.testcerts, followed by adding additional test coverage to finish up.See individual commits for further details on changes they introduced if needing more information.
Type of change
Checklist: