Anchor the 404 check in tls_check_cert_select_plugin - #13503
Merged
Conversation
The test asserted on the bare substring "404", which also matches the ephemeral port number in curl output, so a port such as 62404 made the run fail at random. Match the response status line instead, covering both HTTP/1.1 and HTTP/2. Three assertions also referenced the wrong test run variable, appending to the first run instead of their own, so runs 1-3 never checked that an exchange happened. Point them at the run they belong to. Fixes: apache#13501
Contributor
There was a problem hiding this comment.
Pull request overview
This PR stabilizes the tls_check_cert_select_plugin gold test by making the HTTP 404 assertion unambiguous (no longer colliding with ephemeral port numbers in curl output) and by correcting misdirected assertions so each test run validates its own exchange outcome.
Changes:
- Replace unanchored
"404"assertions with a named regex constant (HTTP_404 = r"HTTP/[\d.]+ 404") that matches HTTP status lines but not port numbers. - Fix three places where assertions were accidentally appended to
trinstead of the activetr2run, re-enabling intended per-run validation. - Update comments to document why a 404 is the expected signal of a successful exchange in this test configuration.
bneradt
approved these changes
Aug 5, 2026
cmcfarlen
added a commit
that referenced
this pull request
Aug 5, 2026
The test asserted on the bare substring "404", which also matches the ephemeral port number in curl output, so a port such as 62404 made the run fail at random. Match the response status line instead, covering both HTTP/1.1 and HTTP/2. Three assertions also referenced the wrong test run variable, appending to the first run instead of their own, so runs 1-3 never checked that an exchange happened. Point them at the run they belong to. Fixes: #13501 (cherry picked from commit b609d7a)
Contributor
Author
|
Cherry-picked to the 10.2.x branch as e4ea73b for the 10.2.0 release. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
tls_check_cert_select_plugin.test.pyfails intermittently in CI depending only on which ephemeral port AuTest hands thetsprocess. It asserted on the bare substring"404":ContainsExpression/ExcludesExpressionare regexes searched per line, and404is unanchored, so it also matches the port number in ordinary curl output. When the port contains404— e.g.62404— the excluded expression matches lines like:and the last test run fails:
Run 6 ("Test new version of bar cert with bad CA") is the one that fails because it is the only one using
ExcludesExpression. TheContainsExpression("404")assertions elsewhere are spuriously satisfied by the same collision, which is arguably worse since they can pass without a real HTTP exchange.This is currently causing 10.2.x PR runs to fail.
Why a 404 is the right thing to assert
The origin registers a response only for
GET / HTTP/1.1with noHostheader, butproxy.config.url_remap.pristine_host_hdris enabled, so every request forwards its originalHostand does not match. The 404 from the origin is what proves the exchange completed. That intent is preserved; only the pattern changes.Changes
This cannot match a port number, and covers both
HTTP/1.1 404 Not Foundand HTTP/2'sHTTP/2 404, since the TLS port offers h2 via ALPN.tr2blocks usedtr., so the "Should make an exchange" check for runs 1-3 was appended to run 0 instead. Run 0's report shows it four times while runs 1-3 never asserted it at all. They now point at their own run, so those three runs actually verify what they were meant to.Test plan
I verified the new pattern against the exact lines from the CI failure plus the status lines it must still match:
"404"* Added bar.com:62404:127.0.0.1 to DNS cache* Trying 127.0.0.1:62404...* Connected to bar.com (127.0.0.1) port 62404< HTTP/1.1 404 Not Found< HTTP/2 404< HTTP/1.1 200 OKAll nine cases I checked behave correctly; the old pattern false-positived on three port lines that the new one rejects.
yapf0.43.0 (the version pinned intools/yapf.sh) reports the file clean.Note that fixing the run-variable references activates three assertions that were previously inert, so CI is the real verification for those three runs. All four runs go through the same
map /remap to the same origin with a non-matchingHost, so all four are expected to 404 identically — but if a run turns out not to, that part can be dropped and handled separately.Backport
Both bugs date to 7dbb6cb (#6609, 2021-06-25), verified with
git log -Son the exact strings, so this affects9.2.x,10.0.x,10.1.x,10.2.xandmaster. The assertion lines are identical on master and 10.2.x — only the unrelatedssl_multicertblock differs — so this should cherry-pick cleanly. 10.2.x wants it for the RC.Fixes #13501