refactor(authlib): proxy listeners session-event parity with extproc#412
Conversation
Closes the feature gap between the proxy-sidecar listeners
(forwardproxy, reverseproxy) and the envoy-sidecar listener
(extproc) in their session-event recording. Operators on
proxy-sidecar mode were seeing impoverished abctl rows because the
proxy listeners only carried a subset of what extproc carries on
each event.
What was missing on proxy listeners (now fixed):
* Plugin Invocations on accept-path events. Previously only the
deny path threaded Invocations through; happy-path requests +
responses left the PLUGIN/ACTION columns blank in abctl.
* Inbound response event. reverseproxy.modifyResponse ran the
inbound pipeline's response phase but never appended a
Phase:SessionResponse event. Inbound A2A `message/stream`
requests showed up as orphan request rows.
* Rekey default → A2A contextId. The first turn of an A2A
conversation arrives without a contextId (the agent assigns it
on response). Without rekey, the inbound request + outbound
MCP/inference calls during processing landed in `default` while
only the response went to the contextId bucket. Now mirrors
extproc.rekeyInboundSession: response phase calls Rekey when
the parser reveals the contextId, merging the whole turn.
* Host, StatusCode, Duration fields on every event. Were unset on
proxy events; abctl's HOST / STATUS / DURATION columns stayed
blank for proxy-sidecar pods.
* Pointer aliasing on Extensions.A2A/MCP/Inference. Events stored
pointers to the live pctx structs; OnResponse mutated the same
structs and the previously-recorded request events
retroactively showed response-phase fields (artifact,
finalStatus, token counts). extproc avoided this with shallow-
copy Snapshot helpers; proxy listeners did not.
* Identity (Subject/ClientID/Scopes) and the plugin-public
Plugins map (Custom entries with /event suffix) were never
surfaced on proxy listener events.
* Error on response events derived from Security.Blocked or non-
2xx StatusCode.
Implementation: lifted the 8 snapshot/derive helpers
(SnapshotA2A, SnapshotMCP, SnapshotInference, SnapshotInvocations,
SnapshotPlugins, SnapshotIdentity, DeriveError, DurationSince) from
the extproc package into authlib/pipeline/snapshot.go as exported
functions. extproc switches to the shared package versions; both
proxy listeners now use the same helpers, single source of truth.
Tier 1 cross-listener dedup also folded in:
* extractBearer was duplicated byte-for-byte across extauthz,
forwardproxy, and extproc. Moved to authlib/auth/bearer.go as
auth.ExtractBearer.
* writeRejection was duplicated byte-for-byte across forwardproxy
and reverseproxy. Moved to a new
authlib/listener/httpx/render.go as httpx.WriteRejection — the
home for HTTP-listener-shared helpers.
Verified locally: full authlib test suite passes; all three
combined images build clean; weather-agent in proxy-sidecar mode
in a kind cluster produces abctl events with full PLUGIN/ACTION/
HOST/STATUS/DURATION/Identity columns and merges into the A2A
contextId bucket on every turn.
Tier 2 cross-listener dedup (recordInboundReject /
recordOutboundReject / inboundSessionID / Rekey-as-method)
intentionally deferred — needs design care on the abstraction
surface (event-builder taking pctx + action + direction + phase).
Better as a separate PR.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
… tests, comments Picked up suggestions from the PR rossoctl#412 review: * New authlib/pipeline/snapshot_test.go covers the helpers that this PR newly exported. Specifically: - SnapshotA2A / SnapshotMCP / SnapshotInference: protect the no-pointer-aliasing contract by mutating the source after snapshot and asserting the snapshot is unchanged. - SnapshotIdentity: nil case + Identity-populated case + Agent-populated case, plus a check that the snapshot's Scopes slice is independently owned (mutating it can't bleed back to the source). - SnapshotPlugins: empty input, /event-suffix filter + suffix-stripping, isolation against non-marshalable values (one bad plugin event must not drop the rest). - DeriveError: Blocked branch, non-2xx branch, 2xx no-op, and Blocked-takes-precedence-over-StatusCode policy. - DurationSince: zero-time short-circuit + valid start. * New TestReverseProxy_ModifyResponse_RekeyAndResponseEvent in the reverseproxy test suite locks in the modifyResponse behavior introduced by this PR — without it, future refactors of the event-construction path could silently regress to "request lands in default, response lands in a fresh contextId bucket, no merge." The test drives a real httptest server, uses an inline a2aStamp plugin to mimic the parser stamping a SessionID during OnResponse, and asserts (a) Sessions.Rekey moved events out of `default` into the contextId bucket, and (b) a SessionResponse event was appended carrying StatusCode + Invocations. * Comments added at: - forwardproxy.go widened append guard (MCP || Inference || Invocations || plugins != nil) — noted that each clause is intentional and removing one silently regresses event coverage. - reverseproxy.go modifyResponse rekey condition — noted why empty SessionID and "default" SessionID are skipped. The Tier 2 dedup work for recordInboundReject / recordOutboundReject is filed as rossoctl#413 so it doesn't fall off the radar. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
esnible
left a comment
esnible
left a comment
There was a problem hiding this comment.
Clean refactor that achieves the stated parity goals. All 8 helpers correctly extracted as exported symbols with shallow-copy semantics; nil handling preserved (verified Invocations.FilteredByPhase is nil-safe on receiver, so SnapshotInvocations(nil, ...) is safe). Dedup of extractBearer/writeRejection is byte-equivalent, and the new httpx package separation reads well. Reverseproxy modifyResponse correctly mirrors extproc rekey + response-event flow, with thorough new test coverage in pipeline/snapshot_test.go and TestReverseProxy_ModifyResponse_RekeyAndResponseEvent.
Areas reviewed: Go source, Go tests, package structure, behavior parity, dedup correctness
Commits: 2, both signed-off
CI: 19/19 passing (Spellcheck skipped)
Two non-blocking suggestions inline.
Assisted-By: Claude (Anthropic AI)
| // Does not consult X-Forwarded-Proto. Honoring that header is only | ||
| // safe when the upstream proxy is trusted; wiring a trust policy is | ||
| // deferred until we have a concrete multi-hop deployment story. | ||
| // https, absent => http. Reverse-proxy-specific helper. |
There was a problem hiding this comment.
suggestion: The requestScheme godoc lost the contract note about X-Forwarded-Proto and trusted-upstream policy. Pre-PR, this comment documented (a) the intentional divergence from Context.Scheme's "empty when undetermined" convention, (b) the failure mode behind a TLS-terminating ingress (r.TLS nil even though the caller used https), and (c) the deferred trust-policy plan. Those are the kinds of design notes future readers will want when deciding whether to trust this scheme value. Worth restoring — the dedup work is a clean opportunity to prune writeRejection's comment, but requestScheme's contract note is unrelated and seems to have been swept up by accident.
There was a problem hiding this comment.
Restored in 96533a3. Brought back the three contract paragraphs (Context.Scheme divergence, TLS-terminator failure mode, deferred X-Forwarded-Proto trust policy). Agreed it was unrelated to the dedup — sweep-up.
| // it ensures auth-only outbound traffic and pure observability | ||
| // events show up in abctl. Don't narrow this back without | ||
| // understanding why each clause is necessary. | ||
| if ev.MCP != nil || ev.Inference != nil || ev.Invocations != nil || plugins != nil { |
There was a problem hiding this comment.
suggestion: The forwardproxy gate was widened here to MCP || Inference || Invocations || plugins (with a clear rationale comment about auth-only outbound traffic and observability events). The reverseproxy peer at server.go:125 still gates inbound recording on pctx.Extensions.A2A != nil only. If an inbound request has gate plugin invocations or plugin-public Custom events but no A2A extension (e.g. a non-A2A inbound or an A2A request that fails to parse), reverseproxy will silently drop it from the session stream while forwardproxy would record it on the outbound side. Is the asymmetry intentional (reverseproxy is A2A-only by contract), or should the inbound gate widen for symmetry? A one-line comment in reverseproxy stating the contract would make this less surprising either way.
There was a problem hiding this comment.
Door #1 — the asymmetry is intentional. Reverseproxy is A2A-only by design: modifyResponse's rekey-from-default-to-contextId logic is A2A-specific, and the SessionID keying assumes A2A. Forwardproxy's wider gate (MCP/Inference/Invocations/plugins) is correct for outbound because outbound traffic isn't A2A-only.
Added a 5-line contract comment at the inbound gate in 96533a3 — points at modifyResponse for the rekey rationale and explicitly notes that non-A2A inbound (or A2A that fails to parse) is not recorded here. If we later add a non-A2A inbound listener, this comment is the breadcrumb to widen the gate.
… notes, document A2A-only inbound gate Two non-blocking suggestions from esnible's PR rossoctl#412 review: 1. requestScheme godoc — restore the contract note paragraphs that the refactor's writeRejection-comment dedup swept up by accident: - Intentional divergence from Context.Scheme's "empty when undetermined" convention (this listener always returns http/https based on r.TLS). - Failure mode behind a TLS-terminating ingress: r.TLS is nil on the inner hop even though the caller's actual scheme was https. - Deferred X-Forwarded-Proto trust-policy plan. These are design notes future readers need when deciding whether to trust this scheme value; pruning was unintentional. 2. Inbound A2A gate — add a one-line contract comment at the `pctx.Extensions.A2A != nil` gate explaining the asymmetry with forwardproxy. Reverseproxy is A2A-only by design (session keying and modifyResponse rekey logic are A2A-specific). Forwardproxy widens its analogous gate to MCP/Inference/Invocations/plugins because outbound traffic is not A2A-only. Non-A2A inbound is intentionally not recorded here. Comment-only changes; no behavior change. Build + vet + tests all pass on authlib/listener/reverseproxy and authlib/pipeline. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
github-project-automation
Bot
moved this from Backlog
to Done
in Kagenti Issue Prioritization
3 participants
Summary
Closes the feature gap between the proxy-sidecar listeners (
forwardproxy,reverseproxy) and the envoy-sidecar listener (extproc) in their session-event recording. Operators on proxy-sidecar mode (the default since #361) were seeing impoverished abctl rows because the proxy listeners only carried a subset of what extproc carries on each event. After this PR, abctl renders the same level of detail regardless of mode.Field-by-field parity table after this PR:
Invocations(plugin timeline)message/streamrows orphanedmodifyResponsedefault→ A2AcontextIdrekeyInboundSessiondefaultandcontextIdHostpctx.HostStatusCodeDurationpipeline.DurationSince(pctx.StartedAt)Identity(Subject/ClientID/Scopes)snapshotIdentitypipeline.SnapshotIdentity(pctx)Plugins(Custom/eventmap)snapshotPluginspipeline.SnapshotPlugins(pctx.Extensions.Custom)Erroron responsederiveErrorpipeline.DeriveError(pctx)A2A/MCP/Inferencesnapshots (no pointer aliasing)pipeline.Snapshot{A2A,MCP,Inference}Implementation
Lifted helpers from
extproctoauthlib/pipeline/The 8 snapshot/derive helpers that extproc had as private functions move to
authlib/pipeline/snapshot.goas exported symbols, so both extproc and the proxy listeners share one source of truth:pipeline.SnapshotA2A,SnapshotMCP,SnapshotInference— shallow copies that prevent the request event's struct from being retroactively mutated whenOnResponsewrites to the same pointerpipeline.SnapshotInvocations(ext, phase)— phase-filtered invocation slicepipeline.SnapshotPlugins(custom)— JSON-marshals plugin-public observability events frompctx.Extensions.Custompipeline.SnapshotIdentity(pctx)— captures Subject/ClientID/Scopes/AgentID offpctx.Identityandpctx.Agentpipeline.DurationSince(start)— wall-clock helperpipeline.DeriveError(pctx)—EventErrorfromSecurity.Blockedor non-2xx StatusCodeextproc's local definitions are deleted; its call sites updated to use the package-level versions. extproc test file references updated similarly.
Plumbed through the proxy listeners
forwardproxy.handleRequest(request + response phases) andreverseproxy.handleRequest+modifyResponsenow use the same shared helpers as extproc. Listener structure unchanged; only the event-construction sites changed.reverseproxy.modifyResponsegained two new things that extproc had:Phase: SessionResponseevent append (was missing entirely).Sessions.Rekey(default, A2A.SessionID)call when the response reveals the contextId.Tier-1 cross-listener dedup
extractBearerauth.ExtractBearer(authlib/auth/bearer.go)writeRejectionhttpx.WriteRejection(new pkgauthlib/listener/httpx/)Net: 3 helper functions consolidated, ~30 LOC of duplication removed, behavior unchanged.
Out of scope (Tier 2 — separate PR)
recordInboundReject/recordOutboundRejectevent-construction intopipeline.BuildDenyEvent(...). ~60 LOC of duplication remaining; needs design care on the abstraction surface (event-builder taking pctx + action + direction + phase + statusCode).inboundSessionID(pctx)andrekeyInboundSession-as-method into shared homes. Both are small; safe to defer.Test plan
go test ./...passes across authlib (28 packages, all pass)GOWORK=off(CI mode)golang:1.24-alpineteam1namespace, proxy-sidecar mode, kind cluster — abctl rows now show:message/streamrequest paired with a response rowcontextIdbucket (no orphans indefault)jwt-validationranAssisted-By: Claude (Anthropic AI) noreply@anthropic.com