blarghmatey opened a new pull request, #13995:
URL: https://github.com/apache/apisix/pull/13995
### Description
With `apisix.tracing: true`, every HTTPS request whose Host has no SSL
object of its own exports a `sni_radixtree_match` span with status ERROR
("failed match SNI"), even though the request succeeds.
The span comes from the second SNI lookup in `verify_https_client`, which
calls `match_and_set(ctx, true, host)` with the Host header as `alt_sni`. A
miss there is non-fatal (`if not matched then return true end`), and
`match_and_set` already skips `core.log.error` when `alt_sni` is set. The
`span:set_status(ERROR)` call sat outside that guard, so the span was still
marked as an error.
We hit this behind a CDN. The CDN connects with an origin hostname as SNI,
which our SSL objects cover, and sends the public hostname as Host, which they
don't. The Host lookup misses on nearly every request, so nearly every trace
contained an error span. An OpenTelemetry tail sampler with a keep-errors
policy ended up keeping 80-90% of traces on one of our gateways, and error
rates computed from spans were inflated.
This moves `set_status` into the existing `if not alt_sni` branch. Here is
what that means for each caller of `match_and_set`:
- `verify_https_client` passes the Host as `alt_sni`. A miss is expected,
and the span is no longer ERROR. This is the fix.
- `ssl_client_hello_phase` also passes `alt_sni` (the handshake SNI). A miss
there is fatal, but the caller already logs it and sets its own
`ssl_client_hello_phase` span to ERROR ("no matched SSL"). Those handshake
spans are then released by `tracer.release()` before any log phase, so they
weren't exported before this change either.
- `verify_tls_client` (stream preread) passes no `alt_sni` and keeps ERROR,
though tracing is a no-op in the stream subsystem.
The comment above the guard is updated to say which callers own the failure.
It also adds an optional `status_code` field to `t/lib/test_otel.lua`'s
`verify_tree`, and a case to `t/plugin/opentelemetry6.t` that sends SNI
`test.com` with `Host: localhost` and asserts that `sni_radixtree_match` is
UNSET. I ran the file against master with the otelcol-contrib collector from
`ci/pod`. Without the fix, TEST 12 fails with `expected status_code=0, got=2`;
with it, all 36 assertions pass. Locally I also saw TEST 5 and TEST 11
occasionally fail at the handshake with "failed to match any SSL certificate by
SNI: test.com" on the first request after the SSL object was written. That
happened with and without this change, and the new TEST 11 uses the same
request pattern as TEST 5.
#### Which issue(s) this PR fixes:
No existing issue.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change (no
user-facing behavior or config changes)
- [x] I have verified that this change is backward compatible
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01HW1dQizMCYXQfLMctNVNEc
--
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]
