Repository navigation
Fix: record the same fields on a denied event from every listener - #1313
Conversation
The four recorders that emit a phase:"denied" event — one per direction
per listener — grew apart from the same copy-pasted body:
Plugins Identity Duration TLS
extproc in/out yes yes yes no
reverseproxy in no no no yes
forwardproxy out no no no no
So which fields /v1/sessions published was a property of the listener
that served the request rather than of the request. The forward proxy
was the worst case, dropping all four: an operator reading a denial in
proxy-sidecar mode could not see who was denied, which plugin denied
them, or how long it took.
Collapse all four onto one builder in listener/internal/sessionevent,
which core/listener/... can import and which can therefore reach both
core/pipeline and core/tlsconfig without the cycle that keeps
pipeline.NewEventTLS taking its SPIFFE ID as a parameter. Each field is
derived from pctx and absent when pctx does not carry it, so a listener
that cannot know something records nothing for it rather than a zero
that reads as real — extproc still publishes no TLS, because Envoy
terminates the handshake and ext_proc never sees the client connection.
The callers keep the two decisions that genuinely differ per listener:
whether to record at all, and which session bucket to append to.
Why the parity suite missed it: its observation struct excluded
Identity, Duration and TLS by construction, and the deny fixture
authenticated as nobody and emitted no plugin event, so the three
dropped fields compared nil to nil and PluginKeys compared empty to
empty. The fixture now authenticates and emits, and the comparison
carries Identity (full) and Duration (presence — it is a live
time.Since, so an equality check across legs would flake). Both are
also asserted absolutely per listener, because the pairwise diff cannot
see a gap that both legs share, and these two recorders were copied
from each other. Verified failing against the pre-fix call sites on
both the inbound and outbound pair.
TLS stays out of parity deliberately, and the struct now says why
rather than calling it a follow-up: cross-listener equality on TLS
would assert something false, and all three drivers run plaintext, so
it would pass for the wrong reason. Its derivation is unit-tested
against a real tls.ConnectionState instead.
Fixes #936
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: cwiklik <cwiklikj@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDenial-event construction is centralized in shared helpers used by extproc, forward proxy, and reverse proxy listeners. Parity tests now compare identity and duration presence, and denial fixtures include identity and plugin events. ChangesDenial Session Event Consistency
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The denial-event consistency change appears mergeable after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Clean consolidation: all four denial recorders now call one builder, the parity suite gains absolute Identity/Duration checks, and the deny fixtures now authenticate and emit a plugin event, so they actually exercise those fields. No must-fix findings; four nits inline.
What I checked:
- Mutation: stripping Plugins/Identity/Duration from the forward-proxy call site fails
TestParity_OutboundDenyOnRequestwith exactly the messages the PR body quotes. - No cost double count: denial events now carry the plugin map, which the ledger reads its cost record from, but every deny recorder runs on a request-phase reject and the cost record is only published on the response pass.
- The PR's factual claims hold: every listener sets
StartedAton entry;pctx.TLSis never set in extproc or the forward proxy; the parity drivers run plaintext;Inboundis the zeroDirection;plugin.go:439is correct. core: build/vet/gofmt clean; listener, pipeline, cost and session tests pass.
| // events it's computed as now - matching-request.At. | ||
| // events it's computed as now - matching-request.At. On denied events | ||
| // it's DurationSince(pctx.StartedAt) — a denial ends the request, so | ||
| // it has a duration even though no response was ever recorded. |
There was a problem hiding this comment.
nit: you fixed the Duration doc, but the type-level godoc at line 72 still says a denied event "typically carries only Invocations (+ Identity, Host, Error)". After this PR every listener's denial also carries Plugins, Duration, Client and HTTPMethod/HTTPPath, plus TLS where the listener saw a handshake. That paragraph is where a consumer learns what a denial looks like, so it's worth updating with the field doc.
| code = action.Violation.Code | ||
| message = action.Violation.Reason | ||
| } | ||
| return pipeline.SessionEvent{ |
There was a problem hiding this comment.
nit: pipeline/snapshot.go:104-109 lists the SnapshotInference exemptions as "the four SessionDenied recorders and forwardproxy.recordTunnelOpened", then says "Anything else that builds a SessionEvent and skips this call is the bug above, not a sixth exemption." After this PR the four recorders don't build an event; this function does, and it skips SnapshotInference. Read literally, that comment now flags sessionevent.Deny as the bug. Suggest naming sessionevent.Deny there, as the one builder the four denial recorders share.
| Code: code, | ||
| Message: message, | ||
| }, | ||
| Duration: pipeline.DurationSince(pctx.StartedAt), |
There was a problem hiding this comment.
nit: in proxy-sidecar mode this line also changes a figure users see. cost/usage/usage.go:1348 counts every event with a Duration of 1ms or more toward latency, denied events included, so reverse- and forward-proxy denials now feed the /v1/usage latency mean in the laptop's Usage pane. A token-exchange deny after a slow IdP failure is one example. This matches extproc, and arguably it's the right number, since the client did wait that long. But it moves a visible metric, so it deserves a line in the PR body.
|
|
||
| // Subject / ClientID / Scopes make the spy stand in for an auth | ||
| // plugin, which is the only thing that ever populates pctx.Identity | ||
| // (jwtvalidation/plugin.go:439 does exactly this assignment). A |
There was a problem hiding this comment.
nit: jwtvalidation/plugin.go:439 is correct today but will go stale with the next edit above it. Naming the place instead (jwt-validation's OnRequest, pctx.Identity = claimsIdentity{...}) won't.
3 participants
Fixes #936.
The defect
The four recorders that emit a
phase:"denied"event — one per direction per listener — grew apart from the same copy-pasted body:PluginsIdentityDurationTLSSo which fields
/v1/sessionspublished was a property of the listener that served the request rather than of the request. It isn't a two-way split: the forward proxy dropped all four, so an operator reading a denial in proxy-sidecar mode — the default shape, and the one the laptop service runs — could not see who was denied, which plugin denied them, or how long it took.The fix
All four now call one builder,
core/listener/internal/sessionevent.That location is load-bearing.
core/listener/internal/is importable only fromcore/listener/..., which is where all four recorders live, so the helper can reach bothcore/pipelineandcore/tlsconfig— without the import cycle that is whypipeline.NewEventTLStakes its SPIFFE ID as a parameter instead of deriving it.Every field is derived from
pctxand absent whenpctxdoes not carry it, so a listener that cannot know something records nothing rather than a zero that reads as a real value. extproc still publishes noTLS, correctly: Envoy terminates the handshake and ext_proc receives the request over gRPC, so there is no client connection to describe.The callers keep the two decisions that genuinely differ per listener — whether to record at all (a deny from a plugin that appended no
Invocationcarries no attribution), and which session bucket to append to (inbound keys on the A2A contextId, outbound inherits the most-recently-updated session).Why the parity suite missed it, and what now catches it
observationexcludedIdentity,DurationandTLSby construction, and the deny fixture authenticated as nobody and emitted no plugin event. So the three dropped fields compared nil-to-nil andPluginKeyscompared empty-to-empty — a fixture only covers what it populates.The fixture now authenticates (via the spy setting
pctx.Identity, which is exactly whatjwtvalidation/plugin.go:439does) and emits a request-phase plugin event.Identityis compared in full;Durationis compared for presence, not equality — it is a livetime.Since, so two legs of one fixture legitimately differ and an equality check would flake. Both are also asserted absolutely per listener, because the pairwise diff cannot see a gap that both legs share, and these two recorders were copied from each other. Same blind spot the existingInferencecomment inobservationDiffdocuments about itself.Verified by stashing only the four production call sites:
TLSis deliberately excluded from parityNot deferred — the struct now says why rather than carrying the old "follow-up fixture pass" note. A cross-listener equality check on
TLSwould assert something false, since extproc legitimately has none; and it would pass here for the wrong reason anyway, because all three drivers run plaintexthttptestservers and every leg records nil. Its derivation is unit-tested against a realtls.ConnectionStateinlistener/internal/sessioneventinstead, which is where one can be constructed.Also
SessionEvent.Durationgodoc, which documented only the request and response phases and not the denied one.Verification
go test ./...incore— all packages pass, including all 12 underlistener/.cmd/agentop,cmd/cortex,cmd/cortex-envoy— pass.gofmt -l .clean,go vet ./...clean.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Summary by CodeRabbit