Document SPIFFE association declarations - #6501
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6501 +/- ##
==========================================
+ Coverage 78.49% 78.56% +0.07%
==========================================
Files 776 776
Lines 76415 76494 +79
==========================================
+ Hits 59983 60099 +116
+ Misses 16427 16390 -37
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b758757 to
f57d37e
Compare
5525559 to
a0921d9
Compare
f57d37e to
c5e6f6e
Compare
a0921d9 to
68125e7
Compare
JAORMX
left a comment
There was a problem hiding this comment.
The new architecture documentation accurately explains the intended model, but currently presents it as operational when it is not reachable.
The example and related text say that SPIFFE declarations create static OAuth clients, install the storage overlay, and reserve client IDs at startup (docs/arch/18-spiffe-association-declarations.md:3,34-78; docs/arch/11-auth-server-storage.md:435-445; docs/arch/09-operator-architecture.md:241). However, the current runtime hard-rejects every non-empty spiffe_trust_domains configuration before that setup can run (pkg/authserver/config.go:317-345). Applying the documented CRD example therefore prevents the embedded auth server from starting; the described static-client, collision, restart, and reservation behavior cannot yet occur.
Please describe these as declarations that are currently rejected pending live SVID verification, or defer the operational-runtime claims until that gate is lifted.
JAORMX
left a comment
There was a problem hiding this comment.
Changes requested:
docs/arch/18-spiffe-association-declarations.md:3,66-78,docs/arch/11-auth-server-storage.md:437-445, anddocs/arch/09-operator-architecture.md:241: the documentation describes static-client registration as operational, butRunConfig.Validaterejects every non-emptyspiffe_trust_domainsconfiguration before auth-server initialization. State that declarations are currently rejected pending SVID verification, or defer the operational claims.
c5e6f6e to
ff2fc59
Compare
68125e7 to
34db653
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Changes requested:
docs/arch/18-spiffe-association-declarations.md:3,23,34,62,66-78,82-94,docs/arch/11-auth-server-storage.md:437-445,docs/arch/09-operator-architecture.md:241, anddocs/operator/crd-api.md:4251-4260,4272-4275describe static-client registration, durable reservations, decorators, restart behavior, and policy enforcement as operational.RunConfig.Validaterejects every non-empty SPIFFE trust-domain configuration before auth-server initialization (pkg/authserver/config.go:317-345), so those effects cannot occur. Describe the fields as declarations that are currently rejected pending live SVID verification, and defer the operational claims until that gate is lifted.
All current CI checks are green; no local tests were run.
ff2fc59 to
3b10dc8
Compare
34db653 to
c31b0a1
Compare
0cd94a9 to
bcbe083
Compare
4093b33 to
37440bf
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at 37440bfc9a76fe0f6bb56e7c5678c0b7f9759472 (scoped to the #6501 layer over #6500): the prior documentation blockers remain unresolved.\n\n- Blocking: docs/arch/18-spiffe-association-declarations.md:3,23,34-78,82-94, docs/arch/09-operator-architecture.md:241, docs/arch/11-auth-server-storage.md:435-445, and docs/arch/README.md:144-147 still present non-empty SPIFFE declarations as an operational, supported static-client configuration. RunConfig.Validate() rejects every non-empty SPIFFE trust configuration before authorization-server initialization (pkg/authserver/config.go:317-345; the operator path is covered at cmd/thv-operator/pkg/controllerutil/authserver_test.go:3531-3568). The YAML example cannot be deployed; registry/overlay/reservation/restart effects therefore cannot occur. State prominently that declarations are currently not deployable and result in terminal invalid configuration until live SVID verification exists.\n\n- Blocking: docs/arch/18-spiffe-association-declarations.md:23 says resources does not reach request-time enforcement, but it is carried into the SPIFFE client and enforced by the token-exchange handler (pkg/authserver/spiffe_association_registry.go:69-80, pkg/authserver/server/registration/spiffe_client.go:72-76, pkg/authserver/server/tokenexchange/handler.go:820-859).\n\n- Blocking: docs/arch/18-spiffe-association-declarations.md:41 uses nonexistent CRD field embeddedAuthServer.allowedAudiences; its values are derived from resolved incoming OIDC configuration. Also, line 78 says changed or removed associations take effect after restart, whereas changed durable placeholders conflict and removal leaves the placeholder (pkg/authserver/storage/spiffe_decorator.go:154-193).\n\n- Blocking: docs/arch/11-auth-server-storage.md:441 describes CIMD as cache-only, but it persists resolved clients through UpsertDCRIssuedClient (pkg/authserver/storage/cimd_decorator.go:288-305).\n\nExact-head CI: 40/40 successful. This is a documentation-correctness request, independent of CI.
bcbe083 to
9dd0362
Compare
37440bf to
322c907
Compare
The runtime already supports declaring SPIFFE trust domains and associating SPIFFE principals with OAuth clients, but only through a hand-authored auth-server RunConfig file; there was no way to declare it through the MCPExternalAuthConfig/VirtualMCPServer CRDs. Add SPIFFETrustDomainConfig and SPIFFEClientConfig CRD types (mirroring the runtime shape field-for-field), CEL admission validation for the paired-configuration, duplicate-name, and trust-domain/principal cross-reference invariants, and Go-level bridging into authserver.ValidateSPIFFETrust for the checks CEL cannot express. Audiences, scopes, and resource-indicator items now carry length and shape constraints at admission time, matching the convention already used for AllowedResources and TrustedIssuerConfig elsewhere in this file, instead of deferring all of it to reconcile time. Refs #6199 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
9dd0362 to
a9921a6
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at 322c90793ba38b5fa4975ff7160a4358dc18458f, scoped to this documentation layer over its inherited #6500 content (the new #6500-only 9dd0362..a9921a6 delta was reviewed and approved separately, not attributed here).
Two documentation-correctness blockers remain:
-
docs/arch/18-spiffe-association-declarations.md:14says the SPIFFE declaration has no configurablegrant_typesfield and that the runtime supplies it. That is true only for the CRD conversion.SPIFFEClientAuthRunConfigexposesGrantTypesin the standalone RunConfig schema; validation restricts it to the token-exchange value (pkg/authserver/spiffe_trust.go:149-151,636; converter:cmd/thv-operator/pkg/controllerutil/authserver.go:352-373). Say explicitly that the CRD omits and synthesizes this field, rather than making the incorrect runtime-schema claim. -
docs/arch/18-spiffe-association-declarations.md:79says a removed association “does not take effect.” On a successful restart, removal does take effect for the in-memory registry/overlay: no configured association means no static client overlay. The durable inert placeholder remains and prevents reuse of the client ID, but it does not resurrect the removed static client. This also contradicts the sentence that follows, which correctly says the overlay is reconstructed from current configuration (pkg/authserver/spiffe_association_registry.go:24-28,57-79;pkg/authserver/storage/spiffe_decorator.go:27-34). Reword to distinguish the active policy from its unreclaimed reservation.
The previously requested startup-gate disclosure, reconcile-derived audiences, request-time resource enforcement, and CIMD durable-write explanation are now correct. CI status is author-owned and separate from this review decision.
322c907 to
cd854a9
Compare
cd854a9 to
3d0e7e5
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at 3d0e7e57bc8245c8a525a83589b0213cc4bcac6a, scoped to the #6501 documentation commit over inherited, merged #6500 (a9921a6).\n\nBlocking documentation-correctness issue: docs/arch/11-auth-server-storage.md:437-445 still describes installing SPIFFEStorageDecorator, static-client overlay behavior, and durable placeholder claims as current behavior when SPIFFE declarations are configured. But every non-empty configuration is rejected by RunConfig.Validate() before storage creation or decorator installation (pkg/authserver/config.go:317-345; pkg/authserver/runner/embeddedauthserver.go:78-86). An operator reading this storage section directly can reasonably expect the described behavior, although it cannot occur in this build.\n\nPlease lead this section with the same current-state disclosure as the SPIFFE declarations document: configuration is currently rejected at startup and the decorator/overlay/reservation behavior is future design behavior until live SVID verification enables it.\n\nThe two prior blockers are resolved: the docs correctly distinguish standalone RunConfig.GrantTypes from the CRD's synthesized value (docs/arch/18-spiffe-association-declarations.md:14), and accurately describe association removal as removing the active overlay after a successful restart while retaining an unreclaimed durable reservation (line 79).\n\nExact-head CI is author-owned and separate from this review decision: 33 successful, 15 in progress, and zero failed checks at review time.
The runtime and CRD-exposure layers for SPIFFE client-auth have no architecture documentation, leaving the design (trust domains, principal-to-client association, the still-enforced validation gate) undiscoverable outside the code itself. Add docs/arch/18-spiffe-association-declarations.md describing the model, and cross-link it from the operator and auth-server storage architecture docs and the docs/arch index. State the terminal current behavior (RunConfig.Validate() rejects any non-empty configuration pending real SVID verification) prominently rather than presenting the YAML example as deployable, and correct several claims that had drifted from the current implementation: resources now reach request-time enforcement, CIMD does persist durable client state (via UpsertDCRIssuedClient, which refuses to clobber a configured client), and a changed or removed association does not take effect after restart (the former fails closed, the latter is tracked as #6477). Refs #6199 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
3d0e7e5 to
cac9d6a
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at cac9d6ab530a4cb8bff547ad857b7af12afccfea, scoped only to the #6501 documentation commit over inherited, merged #6500 (a9921a6).
Approved. The previous documentation blocker is resolved: docs/arch/11-auth-server-storage.md:437 now clearly states that non-empty SPIFFE configuration is rejected before runner, storage, or decorator setup, and correctly frames the decorator/overlay/reservation text as future design behavior. Cross-references and the new association document consistently retain that current-state boundary.
Non-blocking follow-up: the PR descriptions Special notes still says resources does not flow into the runtime OAuth client, while the current code and the new architecture doc correctly show that it does. This is an inaccurate historical PR-description note only; it does not affect the merged documentation or review decision.
Exact-head CI: 48/48 successful. CI is reported separately and did not determine this approval.
Summary
A configuration-only feature (SPIFFE trust domains + static workload-to-client associations, no live SVID verification yet) could easily be mistaken for a completed authentication feature if it isn't clearly scoped in the architecture docs.
docs/arch/18-spiffe-association-declarations.md, documenting the trust/static-client flow, validation boundaries, startup collision handling, restart behavior, and an explicit list of what this feature does NOT do (bundle fetching, live SVID verification, token issuance through SPIFFE).09-operator-architecture.md,11-auth-server-storage.md, and the arch docs index.MinLength/MaxLength) onSPIFFEClientConfig.Audiences/.Scopes, matching the markers already present on sibling fields on the same type — caught while writing the doc's schema description and cross-checking it against the actual CRD markers.Refs #6200, #6205
Type of change
Test plan
task test)task lint-fix)task operator-manifests/task crdref-genre-run to confirm the CRD marker addition produces the expected schema/docs diff and nothing else drifts.API Compatibility
v1beta1API — the marker addition only narrows validation on a field with no existing users (this feature isn't released yet).Does this introduce a user-facing change?
Documentation only, plus slightly stricter (but backward-compatible) validation on two SPIFFE CRD fields.
Special notes for reviewers
This adapts a doc originally written on an earlier, abandoned branch to the current schema — every factual claim (field paths,
bundleSource/resourcesbehavior, collision-handling semantics) was independently re-verified against current source rather than carried over from the old draft; several were corrected in the process (stale field path, an outdated "fails closed on any collision" claim that's now more nuanced after #6474's hardening work, and clarifying thatresourcesdoes flow into the runtime OAuth client and is enforced separately fromaudiencesat token-exchange time). Stacked on #6500 (merged).The whole feature (both static-client registration in #6500 and this doc's claims about it) is currently unreachable end-to-end:
RunConfig.Validate()rejects any non-empty SPIFFE trust configuration before the runner starts, pending real SVID verification. The doc states this prominently rather than presenting the design as deployable.