Skip to content

refactor(pipeline): Detype framework + consolidate single-owner authlib packages#392

Merged
huang195 merged 6 commits into
rossoctl:mainfrom
huang195:feat/detype-framework
May 10, 2026
Merged

refactor(pipeline): Detype framework + consolidate single-owner authlib packages#392
huang195 merged 6 commits into
rossoctl:mainfrom
huang195:feat/detype-framework

Conversation

@huang195

@huang195 huang195 commented May 10, 2026

Copy link
Copy Markdown
Member

Summary

Two coupled cleanups in one PR:

  1. Decouple pipeline/ from plugin-specific types. pipeline.Context no longer imports authlib/validation or authlib/routing. Invocation no longer has 9 hardcoded jwt-validation + token-exchange diagnostic fields.
  2. Move single-owner packages into their owning plugin. Four authlib/ packages had exactly one in-tree importer each. They now live under the plugin that owns them, so authlib/ top-level reflects what's actually shared.

Third-party plugins get typed slots they can use (Identity interface, Invocation.Details map) and a cleaner example of what to colocate inside their own directory vs what the framework actually shares.

What changed

Framework detyping (commits d68ab24, f0a877f, dcda457)

  • pctx.Route deleted. Was written by nothing, read by a single listener helper that always returned empty. Dead code.
  • pctx.Claims *validation.Claimspctx.Identity Identity (interface). Identity exposes Subject() / ClientID() / Scopes(). jwt-validation wraps its *validation.Claims in an in-package adapter. Future auth plugins (SAML, mTLS, custom) plug in without the framework importing their types.
  • Invocation's 9 plugin-specific fields → Details map[string]string. ExpectedIssuer, TokenSubject, TokenAudience, TokenScopes, RouteMatched, RouteHost, TargetAudience, RequestedScopes, CacheHit all collapse into a flat string→string map. Opaque to the framework; abctl renders as key=value rows in the detail pane. Suggested key + encoding conventions (snake_case keys, booleans as "true"/"false", scopes space-joined, audiences comma-joined) documented in plugin-reference.md.

Package moves (commit 3729a52)

History-preserving git mv:

From To
authlib/validation/ authlib/plugins/jwtvalidation/validation/
authlib/exchange/ authlib/plugins/tokenexchange/exchange/
authlib/cache/ authlib/plugins/tokenexchange/cache/
authlib/spiffe/ authlib/plugins/tokenexchange/spiffe/

Each plugin converts from a flat file (plugins/<name>.go) to its own directory (plugins/<name>/plugin.go). main.go adds side-effect imports for the two moved plugins — same pattern an out-of-tree plugin would use.

Kept as shared building blocks: authlib/bypass, authlib/routing, authlib/auth. Rationale for each in the commit message for 3729a52.

Docs refresh (commit bbfba9d)

framework-architecture.md, plugin-reference.md, plugin-tutorial.md, authlib/README.md all updated for the new Identity / Details / package layout.

Review fixup (commit a11985f)

Expand plugin-reference.md Details-encoding guidance with a type→encoding table including the scope-vs-audience split and RFC rationale. Add TestJWTValidation_OnRequest_MultiAudience_CommaJoined locking in the comma-join encoding.

Wire schema change (breaking)

/v1/sessions invocations[*] entries: the 9 named diagnostic fields are gone; plugin-specific context now lives under details: {key: value, ...}. SessionEvent.targetAudience removed. abctl in this repo is updated in the same PR; any external consumer of /v1/sessions needs to migrate.

Migration notes for operators

  • aud log attribute dropped from outbound records (authlib/session/store.go). The value was redundant with Invocation.Details["target_audience"]; queries grepping for aud=... against structured logs should switch to matching on the Invocation's details.target_audience field instead. The target audience is still recorded — just on a different path.
  • Wire-field renames are also key renames. Dashboards, alerts, and saved abctl filters that referenced invocation.expectedIssuer, invocation.targetAudience, etc. need to move to invocation.details.expected_issuer, invocation.details.target_audience, and so on. Key names are snake_case by convention.

Test plan

  • go build ./... and go test -race ./... pass in both modules (authbridge/authlib, authbridge/cmd/authbridge) under GOWORK=off
  • Invariant: grep -rn 'authlib/(validation|exchange|cache|spiffe)' authbridge/authlib/pipeline/ returns empty
  • Invariant: old authlib/{validation,exchange,cache,spiffe}/ directories removed
  • abctl filter + detail pane render Details correctly
  • Multi-audience encoding test locks in comma-join
  • End-to-end smoke in kind: weather-agent + github-issue demos still produce session events with structured details

Out of scope

  • Extensions named slots → protocol registry (separate PR, being explored next)
  • authlib/routing generic refactor (defer until a second plugin needs it)
  • CRD-driven operator UX (separate repo)

Assisted-By: Claude (Anthropic AI) noreply@anthropic.com

huang195 added 5 commits May 10, 2026 08:52
pctx.Route was defined on Context but never written by any plugin.
The only reader — listener's routeAudience() helper in extproc —
always returned "" because pctx.Route was always nil. The route
decision already surfaces through token-exchange's auth.OutboundResult
and the Invocation stream; the dead field on Context was a vestige of
an earlier composition shape.

Delete:
  - Route *routing.ResolvedRoute field on pipeline.Context
  - routing import from authlib/pipeline/context.go
  - routeAudience() helper in cmd/authbridge/listener/extproc/server.go
  - Two TargetAudience: routeAudience(pctx) field assignments (now
    zero-valued, same observable result)
  - TestRouteAudience (tested dead code)
  - Route: &routing.ResolvedRoute{...} in TestRecordOutboundResponseSession
    (was setting state nothing reads); test simplified to
    CapturesHostAndDuration.

Net: -28 LOC. One fewer plugin-specific package imported by the pipeline
framework. SessionEvent.TargetAudience is now always zero in the live
events; the field itself stays for now (wire-schema removal is part of
the Invocation-field collapse in a follow-on commit on this branch).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
pipeline.Context previously imported authlib/validation to type its
Claims field as *validation.Claims — leaking jwt-validation's output
type into the framework. Any third-party auth plugin (SAML, mTLS,
custom header-auth) had no clean slot for subject identity; they'd
either have to shoehorn their data into validation.Claims
semantically, or publish into Extensions.Custom and lose the typed
Identity-on-event surface that the listener's snapshotIdentity
depends on.

This commit:

1. Introduces pipeline.Identity as a three-method interface:
   Subject(), ClientID(), Scopes(). Empty-string / nil returns are
   valid — a SPIFFE authenticator with no "clientID" just returns "".
   Framework code reads via the interface only.

2. Renames Context.Claims → Context.Identity, changes the type from
   *validation.Claims → Identity. Breaking source change; any
   external plugin reading pctx.Claims.Subject now reads
   pctx.Identity.Subject().

3. Deletes the authlib/validation import from pipeline/context.go.
   pipeline/ no longer imports any plugin-specific package.

4. Adds a claimsIdentity adapter in the plugins package (colocated
   with jwt-validation) that wraps *validation.Claims. jwt-validation
   sets pctx.Identity = claimsIdentity{c: result.Claims}.

5. Adds a testIdentity adapter in plugintesting so the JWT stub used
   by listener tests continues to populate pctx.Identity without
   reaching into validation.Claims through the framework.

6. Adds a stubIdentity in extproc's server_test so TestSnapshotIdentity
   and the recordOutboundResponseSession_CapturesHostAndDuration test
   set pctx.Identity directly without depending on validation.Claims.

Invariant check:

  $ grep -rn 'authlib/(validation|routing)' authbridge/authlib/pipeline/
  (no output — pipeline/ is now free of plugin-specific imports)

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
…s map

pipeline.Invocation previously carried 9 jwt-validation + token-exchange-
specific diagnostic fields (ExpectedIssuer, ExpectedAudience,
TokenSubject, TokenAudience, TokenScopes, RouteMatched, RouteHost,
TargetAudience, RequestedScopes, CacheHit). Third-party plugins
inherited those slots whether they wanted them or not, and had no
typed home for their own domain-specific context (rate-limiter tokens-
remaining, DLP match-counts, broker consent-state, etc.).

Replace with Details map[string]string: plugin-specific context,
opaque to the framework, rendered as key=value rows by abctl.
Conventions:

  - snake_case keys scoped to the plugin's semantic domain
  - booleans as "true"/"false" (boolStr helper)
  - []string as space-joined (matches OAuth scope convention)
  - NEVER put raw tokens, signatures, or credentials in values

Migrations in this commit:

  jwt-validation  Invocation fields → Details:
                  expected_issuer, expected_audience (deny branch)
                  token_subject, token_audience, token_scopes (allow)

  token-exchange  Invocation fields → Details:
                  route_matched, route_host, target_audience,
                  requested_scopes, cache_hit

  SessionEvent.TargetAudience:  removed. Only populator was the
                  routeAudience() helper that went away with D1;
                  operators read target audience via the Invocation
                  Details map now.

  authlib/session/store.go:  drops the "aud" log attribute that was
                  reading the now-removed SessionEvent field.

  authlib/cmd/abctl/tui/events_pane.go:  filter-text search now
                  iterates inv.Details keys+values for substring match.
                  detail_pane.go needs no change — it renders the
                  session event as pretty-printed JSON and the new
                  "details" field shows up inline with everything else.

Test migrations: ~30 assertions across pipeline/session_test,
plugins/plugins_test, sessionapi/server_test, extproc/server_test
rewritten from `inv.ExpectedIssuer == "foo"` to
`inv.Details["expected_issuer"] == "foo"`. Mechanical.

Wire-breaking: the session API response shape changes. No versioning
exists today so no compat shim. Only known consumer is abctl (in-tree
on this branch).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Four authlib packages had exactly one in-tree importer each. Moving
them under the plugin that owns them makes the authlib/ layout honest
about what's shared vs plugin-local, and models the conventional
"plugin owns its internals" pattern for out-of-tree plugins.

Moves (git mv, history-preserving):

  authlib/validation  →  authlib/plugins/jwtvalidation/validation
  authlib/exchange    →  authlib/plugins/tokenexchange/exchange
  authlib/cache       →  authlib/plugins/tokenexchange/cache
  authlib/spiffe      →  authlib/plugins/tokenexchange/spiffe

Each plugin also converts from a flat file in authlib/plugins/ to its
own directory (own package):

  authlib/plugins/jwtvalidation.go   →  authlib/plugins/jwtvalidation/plugin.go
  authlib/plugins/jwtvalidation_identity.go → .../jwtvalidation/identity.go
  authlib/plugins/tokenexchange.go   →  authlib/plugins/tokenexchange/plugin.go

Plugin packages now import plugins.RegisterPlugin / plugins.StatsSource
from the parent plugins package explicitly. main.go adds side-effect
imports for the two moved plugins so their init() runs — same pattern
an external plugin would use.

Kept as shared building blocks:

  authlib/bypass   — any future inbound gate plugin wants path bypass
                     (healthz, readyz, .well-known). Clear multi-plugin
                     future, even though jwt-validation is the only
                     current importer.
  authlib/routing  — two in-tree users (jwt-validation, token-exchange).
                     External token-broker (PR rossoctl#391) rolled its own
                     router because authlib/routing.ResolvedRoute has
                     token-exchange-shaped fields — separate cleanup
                     deferred as a followup (make the routing
                     mechanism reusable without the schema).
  authlib/auth     — composition layer both gate plugins wrap via
                     auth.Auth. Lingers in authlib/ for now even though
                     it's effectively plugin-internal; natural
                     refactor candidate once a third gate plugin
                     emerges or when auth's Stats type is replaced by
                     a plugin-generic metrics interface.

Test-file reorganization:

  authlib/plugins/plugins_test.go  split into three:
    plugins_test.go (package plugins_test): YAML loader, Stats, Build
      — cross-plugin integration tests, black-box imports of jwtvalidation
      and tokenexchange via side-effect.
    plugins/jwtvalidation/plugin_test.go (package jwtvalidation):
      all JWT Configure/Ready/OnRequest tests + invokeOnRequest helper +
      mockJWTVerifier + newTestJWTValidation.
    plugins/tokenexchange/plugin_test.go (package tokenexchange):
      all TokenExchange tests + invokeOnRequest helper.

  Tests that accessed unexported plugin fields (p.cfg, p.inner,
  p.audienceDeriver) naturally moved into the plugin's own package
  where those fields are in-package.

Invariant checks:

  $ grep -rn 'authlib/(validation|exchange|cache|spiffe)' authbridge/authlib/pipeline/
  (empty — pipeline/ still free of plugin-specific imports)

  $ ls authbridge/authlib/ | grep -E '^(validation|exchange|cache|spiffe)$'
  (empty — old package directories removed)

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
- framework-architecture.md §3 Context: Identity field + interface
  replaces Claims; Route removed.
- framework-architecture.md §4 Invocation: Details map replaces the 9
  typed diagnostic fields.
- framework-architecture.md §7 SessionEvent: TargetAudience removed.
- framework-architecture.md §12 Versioning: new changelog entries for
  the detyping and the authlib reorganization.
- plugin-reference.md Invocation field reference: Details map + the
  suggested key conventions used by built-in plugins.
- plugin-tutorial.md Step 2 Record example: uses Details map.
- authlib/README.md package table: split into "shared building blocks"
  vs "plugins and their internals"; notes the relocation.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
…mma-join

PR rossoctl#392 review surfaced two related gaps around Invocation.Details value
encoding:

1. plugin-reference.md said "[]string space-joined (OAuth scope
   convention)" as a blanket rule, but jwt-validation writes aud with
   comma. The distinction is principled - RFC 6749 forbids spaces in
   scope tokens while RFC 7519 permits spaces in JWT audiences - but
   wasn't documented for third-party plugin authors.

2. plugin_test.go only covered single-element audiences, so a future
   drive-by change to strings.Join(..., " ") would pass CI and silently
   break consumers splitting on comma.

Docs: expand the Details encoding block into a type->encoding table
with the scope-vs-audience split and its RFC rationale. Add general
guidance on delimiter choice for third-party []string fields.

Test: new TestJWTValidation_OnRequest_MultiAudience_CommaJoined asserts
Audience{"aud-a","aud-b"} renders as "aud-a,aud-b" and token_scopes
keeps using space in the same record - locks both encodings in.

No code change; jwt-validation already uses comma for aud and space
for scopes.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>

@esnible esnible left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-structured refactor split cleanly into 6 commits. Pipeline decoupling via Identity interface and Details map is the right direction, with history-preserving package moves for the four single-owner packages. All 17 CI checks pass; all commits signed-off with proper Assisted-By attribution and conventional commit prefixes.

Verified: no old import paths (authlib/validation|exchange|cache|spiffe) remain on kept lines, no dangling TargetAudience/Claims field references, the multi-audience comma-join encoding is locked in by TestJWTValidation_OnRequest_MultiAudience_CommaJoined, and the claimsIdentity adapter in plugins/jwtvalidation/identity.go correctly bridges the type change across all callers (listeners, snapshotIdentity).

One note for before merge: the E2E kind smoke item in the test plan is unchecked. Given the wire-format break on /v1/sessions (the 9 typed diagnostic fields collapsing into details: {...} and targetAudience removal), a run through the weather-agent and github-issue demos would catch any round-trip mismatch between the new Details map and abctl's decoder before external consumers hit it. Not blocking — leaving as a heads-up.

Areas reviewed: Go (core pipeline, plugins, tests), Docs
Commits: 6, all signed-off
CI status: passing (17/17)

@huang195
huang195 merged commit 626604f into rossoctl:main May 10, 2026
17 checks passed
@huang195
huang195 deleted the feat/detype-framework branch May 10, 2026 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants