Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prefer API V2 when both offered in a single directory, improve test error reports #691

Merged
merged 6 commits into from
Aug 15, 2021

Conversation

tlhackque
Copy link
Contributor

@tlhackque tlhackque commented Jul 30, 2021

This doesn't happen with Let'sEncrypt, but it does with at least one other CA. The ACME spec allows it.

This fixes getssl, but doesn't provide a test.

Also a (somewhat ugly) attempt at fixing #692

Fixes #690
Fixes #692

This doesn't happen with Let'sEncrypt, but it does with at least
one other CA.  The ACME spec allows it.
@tlhackque tlhackque changed the title Prefer API V2 when both offered in a single directory Prefer API V2 when both offered in a single directory, improve test error reports Jul 30, 2021
@tlhackque tlhackque marked this pull request as draft July 30, 2021 13:03
@tlhackque
Copy link
Contributor Author

The test logging seems to cause confusion with the test harness.

Outside the test harness, it seems to do what's intended (you have to provide fd3, of course).

It seems as though check_output_for_errors may need to run in its "less strict" mode. But as I don't really understand the test system, perhaps you (@timkimber) can help sort this out.

See , e.g. https://github.com/tlhackque/getssl/runs/3201899538?check_suite_focus=true

By the way, I also tried to do the logging work in a separate branch, but couldn't persuade GitHub to run the tests (I did add the branch to the .yml files).

I've made this PR into a "draft" until this is resolved.

@tlhackque tlhackque force-pushed the master branch 8 times, most recently from 577819e to dc7d65f Compare July 31, 2021 01:21
@tlhackque
Copy link
Contributor Author

I've made some progress:

  • Removed a spurrious exit leftover from my testing that caused test failures
  • Forced the "liberal compare"
  • Made tests run from a fork repo do the update checks against that repo

Note that tests run here due to a pull request on a fork will fail because the update checks will use this repo - which won't have the new version...

Should include the correct branch in update checks & run tests on branch commits - exercise for the reader.

It seems that the test harness uses errexit, so a trap ERR is necessary to get the output. There may be cases where getssl expects commands' exit status to be ignored, which this will catch. If so, they'll need to be flagged with || true, which will suppress the trap.

Tests are running in my repo - hopefully this is converging. I will check in later.

@tlhackque tlhackque force-pushed the master branch 2 times, most recently from 540cb93 to be10a59 Compare July 31, 2021 11:01
The test harness will suppress output unless an error occurs.

Upgrade testing was failing in forked repos CI because the
source repo was hard-coded.  Use the CI environment to use
the fork's repo.
@tlhackque tlhackque force-pushed the master branch 10 times, most recently from 84d9cc6 to 9c93552 Compare August 3, 2021 14:49
@tlhackque tlhackque force-pushed the master branch 6 times, most recently from 9a5b9b6 to a05cf4e Compare August 3, 2021 20:37
The first fail logic seems scoped to a single test file.

Set the flag globally.
Since the tests run in a container, cleanup is automagic.

Various test tweaks to allow for skipping tests & null strings.
@tlhackque
Copy link
Contributor Author

After a fair bit of debugging (sorry about the force pushes - only way I found to debug CI & keep the tag straight), it turns out that the test framework (BATS) can do most of what we want.

I have skip after first fail working; debug output is displayed on failure.

I've fixed some issues that turned up when skipping; also worked on some of the diagnostics.

Tests that are expected to exit with failure status are a bit chatty. In particular 18 (check_retry_add_dns_command)...
seems like a small price to pay for being able to debug intermittents. I guess we could disable it - it doesn't seem to do much;
it would be more interesting if the retry used a non-failing add_dns command :-)

TODO:

  • Some tests (e.g. 17-test-spaces-in-sans-http01) assume that the last test will clean up the environment. This won't happen if skipped. The cleanup should be in teardown_file() functions instead.
  • The dynamic DNS tests - if they are enabled - probably will fail if more than one copy is run simultaneously - e.g. from the srvrco repo and a fork due to a fork push with a pending pull request. The fix would be to include the repo name (in GITHUB_REPOSITORY - something like: REPO="$(echo "$GITHUB_REPOSITORY" | cut -d/ -f1); [ -n "$REPO" ] && DOMAIN="$REPO-$DOMAIN) in the doman names. I assume that the names need to be registered with the dns accounts...so that's an exercise for the reader.
  • The dynu failures seem to be because there are multiple TXT records - one of which is the expected record, but letsEncrypt is objecting to the others. See https://github.com/srvrco/getssl/runs/3235447284?check_suite_focus=true Probably a failure of a previous run to clean up.

@tlhackque tlhackque marked this pull request as ready for review August 3, 2021 23:30
Only run cURL once to determine it's version.
Verify that a unified directory, API V2 is selected.
@timkimber
Copy link
Member

@tlhackque finally got a chance to fix the upgrade test - as you said I'd forgotten to tag the latest release

@timkimber timkimber merged commit 1982a94 into srvrco:master Aug 15, 2021
@tlhackque
Copy link
Contributor Author

@tlhackque finally got a chance to fix the upgrade test - as you said I'd forgotten to tag the latest release

That was clear in the logs - and the tests passed in my repo where I did tag it. (Which is why every push of an incremental commit was a force push - had to move the tag...)

I'll create new issues for the TODOs that I didn't handle (mostly because you need to do stuff.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make tests easier to debug Prefer API V2 when a CA directory offers both
2 participants