fix(tracer): don't mark sni_radixtree_match ERROR on alt SNI misses - #13995
Open
blarghmatey wants to merge 2 commits into
Open
blarghmatey wants to merge 2 commits into
blarghmatey wants to merge 2 commits into
Conversation
…d alt SNI miss verify_https_client re-runs the SNI router with the Host header as alt_sni and treats a miss as non-fatal, and match_and_set already skips its error log for that case. It still set the span status to ERROR, so any HTTPS request whose Host has no SSL object of its own (e.g. behind a CDN that connects with an origin hostname as SNI) exports a trace containing an error span. Tail samplers that keep traces with errors then keep nearly all of them. The status is now set in the same branch as the error log, so only a caller that passes no alt_sni marks the span. ssl_client_hello_phase, which also passes alt_sni, already marks its own span when the handshake SNI has no certificate.
The comment implied alt_sni always means an expected miss. ssl_client_hello_phase also passes the handshake SNI as alt_sni, and it logs and marks its own span when that lookup fails.
blarghmatey
force-pushed
the
fix/sni-alt-sni-span-status
branch
from
September 29, 2026 13:57
0e911d2 to
1fae379
Compare
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
With
apisix.tracing: true, every HTTPS request whose Host has no SSL object of its own exports asni_radixtree_matchspan with status ERROR ("failed match SNI"), even though the request succeeds.The span comes from the second SNI lookup in
verify_https_client, which callsmatch_and_set(ctx, true, host)with the Host header asalt_sni. A miss there is non-fatal (if not matched then return true end), andmatch_and_setalready skipscore.log.errorwhenalt_sniis set. Thespan: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_statusinto the existingif not alt_snibranch. Here is what that means for each caller ofmatch_and_set:verify_https_clientpasses the Host asalt_sni. A miss is expected, and the span is no longer ERROR. This is the fix.ssl_client_hello_phasealso passesalt_sni(the handshake SNI). A miss there is fatal, but the caller already logs it and sets its ownssl_client_hello_phasespan to ERROR ("no matched SSL"). Those handshake spans are then released bytracer.release()before any log phase, so they weren't exported before this change either.verify_tls_client(stream preread) passes noalt_sniand 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_codefield tot/lib/test_otel.lua'sverify_tree, and a case tot/plugin/opentelemetry6.tthat sends SNItest.comwithHost: localhostand asserts thatsni_radixtree_matchis UNSET. I ran the file against master with the otelcol-contrib collector fromci/pod. Without the fix, TEST 12 fails withexpected 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