cmcfarlen opened a new pull request, #13503:
URL: https://github.com/apache/trafficserver/pull/13503
## Description
`tls_check_cert_select_plugin.test.py` fails intermittently in CI depending
only on which ephemeral port AuTest hands the `ts` process. It asserted on the
bare substring `"404"`:
```python
tr.Processes.Default.Streams.All += Testers.ExcludesExpression("404",
"Should make an exchange")
```
`ContainsExpression`/`ExcludesExpression` are regexes searched per line, and
`404` is unanchored, so it also matches the **port number** in ordinary curl
output. When the port contains `404` — e.g. `62404` — the excluded expression
matches lines like:
```
* Added bar.com:62404:127.0.0.1 to DNS cache
* Trying 127.0.0.1:62404...
```
and the last test run fails:
```
file .../6-tr-Default/stream.all.txt : Should make an exchange - Failed
Reason: Contents ... contains expression: "404"
Details:
* Added bar.com:62404:127.0.0.1 to DNS cache : 1
... 0* Trying 127.0.0.1:62404... : 6
```
Run 6 ("Test new version of bar cert with bad CA") is the one that fails
because it is the only one using `ExcludesExpression`. The
`ContainsExpression("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.1` with no `Host`
header, but `proxy.config.url_remap.pristine_host_hdr` is enabled, so every
request forwards its original `Host` and does not match. The 404 from the
origin is what proves the exchange completed. That intent is preserved; only
the pattern changes.
## Changes
1. Match the response status line via a named constant instead of bare
digits:
```python
HTTP_404 = r"HTTP/[\d.]+ 404"
```
This cannot match a port number, and covers both `HTTP/1.1 404 Not Found`
and HTTP/2's `HTTP/2 404`, since the TLS port offers h2 via ALPN.
2. Fix three assertions that referenced the wrong test run variable. Lines
that belonged to `tr2` blocks used `tr.`, 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:
| Line | new pattern | old `"404"` | want |
|---|---|---|---|
| `* Added bar.com:62404:127.0.0.1 to DNS cache` | no | **yes** | no |
| `* Trying 127.0.0.1:62404...` | no | **yes** | no |
| `* Connected to bar.com (127.0.0.1) port 62404` | no | **yes** | no |
| `< HTTP/1.1 404 Not Found` | yes | yes | yes |
| `< HTTP/2 404` | yes | yes | yes |
| `< HTTP/1.1 200 OK` | no | no | no |
All nine cases I checked behave correctly; the old pattern false-positived
on three port lines that the new one rejects.
`yapf` 0.43.0 (the version pinned in `tools/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-matching `Host`, 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 7dbb6cb188 (#6609, 2021-06-25), verified with `git log -S`
on the exact strings, so this affects `9.2.x`, `10.0.x`, `10.1.x`, `10.2.x` and
`master`. The assertion lines are identical on master and 10.2.x — only the
unrelated `ssl_multicert` block differs — so this should cherry-pick cleanly.
10.2.x wants it for the RC.
Fixes #13501
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
