Repository navigation
Fix: record the same fields on a denied event from every listener #1313
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| // Package sessionevent builds the parts of a session event that every | ||
| // listener should record identically. | ||
| // | ||
| // It exists because the four recorders that emit a phase:"denied" event — one | ||
| // per direction per listener — grew apart from the same copy-pasted body. | ||
| // extproc recorded Plugins, Identity and Duration on both directions; the | ||
| // reverse proxy recorded TLS and none of those three; the forward proxy | ||
| // recorded none of the four. So an operator reading /v1/sessions saw a | ||
| // different event shape depending on which listener served the request, and | ||
| // which fields were populated was a property of the serving listener rather | ||
| // than of the request — the one thing a parity suite exists to prevent. | ||
| // | ||
| // Every field is derived from pctx and is 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 a real value. | ||
| package sessionevent | ||
|
|
||
| import ( | ||
| "time" | ||
|
|
||
| "github.com/rossoctl/cortex/core/pipeline" | ||
| authtls "github.com/rossoctl/cortex/core/tlsconfig" | ||
| ) | ||
|
|
||
| // Deny builds the phase:"denied" event for a request a pipeline plugin | ||
| // rejected on OnRequest. | ||
| // | ||
| // dir is a parameter rather than read from pctx.Direction because the | ||
| // recorders are already one-per-direction and several of their unit tests | ||
| // construct a bare pctx with no Direction set. Reading it from pctx would | ||
| // relabel those events silently — Inbound is the zero value — which is a | ||
| // worse failure than passing the constant the caller already knows. | ||
| // | ||
| // The caller keeps two decisions. Whether to record at all: a deny from a | ||
| // plugin that appended no Invocation carries no attribution, and a | ||
| // content-free denial event is noise. And which session bucket to append to: | ||
| // that genuinely differs per listener, inbound keying on the A2A contextId | ||
| // while outbound inherits the most-recently-updated session. | ||
| func Deny(pctx *pipeline.Context, action pipeline.Action, dir pipeline.Direction) pipeline.SessionEvent { | ||
| var status int | ||
| var code, message string | ||
| if action.Violation != nil { | ||
| // Read the structured fields directly. Render() produces the HTTP wire | ||
| // payload — status, headers, JSON body — which is the wrong shape for a | ||
| // session event; what belongs here is the semantic Code and Reason. | ||
| status = action.Violation.Status | ||
| if status == 0 { | ||
| status = pipeline.StatusFromCode(action.Violation.Code) | ||
| } | ||
| code = action.Violation.Code | ||
| message = action.Violation.Reason | ||
| } | ||
| return pipeline.SessionEvent{ | ||
| At: time.Now(), | ||
| Direction: dir, | ||
| Phase: pipeline.SessionDenied, | ||
| RequestID: pctx.RequestID(), | ||
| Invocations: pipeline.SnapshotInvocations(pctx.Extensions.Invocations, pipeline.InvocationPhaseRequest), | ||
| Plugins: pipeline.SnapshotPlugins(pctx.Extensions.Custom), | ||
| Identity: pipeline.SnapshotIdentity(pctx), | ||
| Host: pctx.Host, | ||
| HTTPMethod: pctx.Method, | ||
| HTTPPath: pctx.Path, | ||
| StatusCode: status, | ||
| Error: &pipeline.EventError{ | ||
| Kind: "policy", | ||
| Code: code, | ||
| Message: message, | ||
| }, | ||
| Duration: pipeline.DurationSince(pctx.StartedAt), | ||
|
Member
There was a problem hiding this comment. nit: in proxy-sidecar mode this line also changes a figure users see. |
||
| Client: pctx.ClientInfo(), | ||
| TLS: TLS(pctx), | ||
| } | ||
| } | ||
|
|
||
| // TLS summarizes the connection the request arrived on, and is nil whenever | ||
| // there is nothing true to say: a plaintext caller, or a listener that never | ||
| // sees the client's connection at all. In envoy-sidecar mode Envoy terminates | ||
| // the TLS and ext_proc receives the request over gRPC, so pctx.TLS is unset | ||
| // there and this records nothing rather than inventing a handshake. | ||
| func TLS(pctx *pipeline.Context) *pipeline.EventTLS { | ||
| if pctx == nil || pctx.TLS == nil { | ||
| return nil | ||
| } | ||
| return pipeline.NewEventTLS(pctx.TLS, authtls.PeerSPIFFEID(pctx.PeerCertificate())) | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
pipeline/snapshot.go:104-109lists theSnapshotInferenceexemptions 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 skipsSnapshotInference. Read literally, that comment now flagssessionevent.Denyas the bug. Suggest namingsessionevent.Denythere, as the one builder the four denial recorders share.