Skip to content

Refactor: remove SPIRE_ENABLED from authbridge-config and per-namespace envoy-config CM - #447

Merged
akram merged 4 commits into
rossoctl:mainfrom
letv1nnn:spire-enabled-update
Jun 20, 2026
Merged

Refactor: remove SPIRE_ENABLED from authbridge-config and per-namespace envoy-config CM#447
akram merged 4 commits into
rossoctl:mainfrom
letv1nnn:spire-enabled-update

Conversation

@letv1nnn

Copy link
Copy Markdown
Contributor

Summary

RHAIENG-4941: mTLS is now baseline — two per-namespace configuration levers are redundant:

  • SPIRE_ENABLED key in authbridge-config CMclientregistration_controller now derives spireEnabled from r.SpireTrustDomain != "" instead of reading from the ConfigMap. The key is no longer written by authbridgeconfig_controller.
  • Per-namespace envoy-config CM — no longer read by the webhook. Envoy config is always generated from template. The envoy-config volume is now Optional: true so pods don't fail when the CM is absent. Per-agent envoy CM is always created in envoy-sidecar mode (guard on mtlsMode != MTLSModeDisabled removed).

Not touched: Container env var SPIRE_ENABLED in container_builder.go (gates spiffe-helper in combined image, sourced from injection decision, not CM).

Changes

  1. clientregistration_controller — derive spireEnabled from SpireTrustDomain, remove SpireEnabled from authbridgeConfig struct
  2. authbridgeconfig_controller — stop writing SPIRE_ENABLED, remove SpireTrustDomain from AuthbridgeConfigPlatform
  3. namespace_config + resolved_config — remove SpireEnabled and EnvoyYAML fields, remove envoy-config CM read
  4. envoy_template — remove EnvoyYAML early-return bypass
  5. volume_builder — make envoy-config volume Optional in all builders
  6. pod_mutator — always create per-agent envoy CM in envoy-sidecar mode
  7. Delete config/authbridge/envoy-config.yaml, update kustomization, agentruntime_controller, docs

Test plan

  • go build ./... passes
  • Controller unit tests pass (13 tests including 2 new)
  • Injector unit tests pass (all tests including new TestEnvoyConfigVolume_Optional)
  • go vet ./... clean
  • Kind cluster validation: AC1-AC4 verified
    • AC1: authbridge-config has no SPIRE_ENABLED key
    • AC2: No envoy-config CM created, no errors
    • AC3: envoy-config volume is Optional, template always used
    • AC4: Leftover envoy-config CM ignored without error
  • No cm.Data["SPIRE_ENABLED"] in production code
  • No code reads per-namespace envoy-config CM
  • envoy-config volume is Optional in all builders
  • Container env var SPIRE_ENABLED untouched in container_builder.go

🤖 Generated with Claude Code

@letv1nnn
letv1nnn requested a review from a team as a code owner June 18, 2026 16:20

@akram akram left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review — tested end-to-end on ROSA HCP with SPIFFE

@letv1nnn good work on this cleanup. I rebased the PR on current main, built the operator image, and deployed it on our ROSA HCP cluster (redbank-demo-2 namespace) with SPIRE enabled and federated-jwt auth. Here's what I found.

What works well

  • spireEnabled derivation from SpireTrustDomain — correct and cleaner than reading from the CM. The client registration controller properly generates SPIFFE URIs as client IDs when the trust domain flag is set.
  • Removing SPIRE_ENABLED from authbridge-config — the container env var SPIRE_ENABLED (in container_builder.go) is untouched, which is the right call. The CM key was redundant.
  • Removing the per-namespace envoy-config CM — the template rendering path and Optional: true on the volume work correctly. No pod scheduling failures.
  • Per-agent envoy CM always created in envoy-sidecar mode — correct since there's no namespace-level fallback anymore.
  • Tests and docs are thorough and accurate.

Issues found during live testing

1. Missing jwt_audience in token-exchange pipeline config (crash)

When CLIENT_AUTH_TYPE=federated-jwt, the webhook's synthesizePipeline() sets identity.type=spiffe in the outbound token-exchange plugin config but does not set identity.jwt_audience. The authbridge binary crashes at startup:

token-exchange config: tokenexchange: identity.type=spiffe requires identity.jwt_audience to be set

Root cause: NamespaceConfig doesn't read JWT_AUDIENCE from the authbridge-config CM, and synthesizePipeline() doesn't inject it into the identity block.

Fix: Read JWT_AUDIENCE in ReadNamespaceConfig() and set identity["jwt_audience"] alongside identity["type"] = "spiffe" in synthesizePipeline(). I have a fix with tests on branch test/pr447-spire-cleanup on my fork if you want to pick it up.

2. Missing spiffe: block in per-agent config.yaml (crash)

The authbridge binary (v0.6.0-alpha.4) creates the in-process SPIFFE Provider (Workload API client) only when the top-level spiffe: block is present in config.yaml. The webhook's ensurePerAgentConfigMap() never injects this block, so the token-exchange plugin fails:

token-exchange: spiffe identity requires a SPIFFE provider to be injected

Root cause: ensurePerAgentConfigMap() doesn't receive spireEnabled and doesn't inject the spiffe: block. The mtls: and tls_bridge: blocks follow a pattern that should be extended to spiffe:.

Fix: Add spireEnabled bool parameter to ensurePerAgentConfigMap(), inject spiffe: {socket: <platform_socket_path>} when true, scrub it when false. Same branch has this fix.

Suggestion

Both issues above are pre-existing gaps exposed by this PR making spireEnabled work correctly via SpireTrustDomain. They should either be included in this PR or landed as a companion PR before merge — without them, any cluster with --spire-trust-domain set and CLIENT_AUTH_TYPE=federated-jwt will have authbridge crashes.

Fixes are on akram/kagenti-operator branch test/pr447-spire-cleanup (commits eeb7fe9 and f63064d).

Minor nits

  • The authbridge-config CM in watched namespaces needs the kagenti.io/defaults=true label to be visible to the operator's scoped cache. Pre-existing namespaces that had the CM created before the label was required will hit ConfigMap "authbridge-config" not found errors until re-labeled. Not caused by this PR but worth noting in upgrade docs.

Overall: LGTM on the core refactor, but the two pipeline config gaps need to be addressed before merge.

@letv1nnn
letv1nnn force-pushed the spire-enabled-update branch 2 times, most recently from 14b24d6 to 5f9845a Compare June 19, 2026 14:23
@letv1nnn
letv1nnn requested a review from akram June 19, 2026 14:41

@huang195 huang195 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.

Reviewed the diff in full and verified against source at HEAD (5f9845a). Posting as a comment — deferring the verdict to @akram, who validated this end-to-end on ROSA HCP.

Core refactor — verified correct & complete

  • spireEnabled now derives from the injection decision / --spire-trust-domain (pod_mutator.go:185, clientregistration_controller.go:242), not the removed CM field.
  • Grep at HEAD confirms: no production code reads cm.Data["SPIRE_ENABLED"] (only the intentional container env var in container_builder.go remains), nothing reads the per-namespace envoy-config CM, zero leftover EnvoyYAML/.SpireEnabled refs (compiles clean), and the envoy-config volume is Optional in all three builders. All four of the unchecked test-plan boxes are in fact true.

@akram's two CHANGES_REQUESTED crashes are addressed

Commits 2562a15 (jwt_audience) and 5f9845a (spiffe block) implement exactly the fixes described. Source parity holds: the operator already writes JWT_AUDIENCE=issuer, and the spiffe: block is injected/scrubbed symmetric to the existing mtls/tls_bridge pattern. The CHANGES_REQUESTED state predates both commits.

One non-blocking suggestion (inline)

synthesizePipeline jwt_audience fallback — see the inline comment.

Cross-repo heads-up (for a companion kagenti change, not this PR)

Since the operator no longer reads SPIRE_ENABLED or the namespace envoy-config CM, the kagenti backend's _ensure_authbridge_configmaps now writes two dead values: authbridge-config["SPIRE_ENABLED"] and the envoy-config CM. Harmless (the envoy volume is now Optional), but worth a follow-up cleanup so the backend stops emitting config the operator ignores.

Also note the new else { delete(cfg, "spiffe") } branch tightens the coupling: any workload whose effective config carries identity.type: spiffe while the operator computes spireEnabled=false will now have its SPIFFE provider block stripped, producing the same startup crash. Today the mTLS-baseline auto-enable (pod_mutator.go:290) keeps identity.type and spireEnabled in lockstep, so it's latent — but it's most fragile during the kagenti-extensions → operator webhook migration this PR's docs reference.

Nits

  • The PR Summary / Changes list doesn't mention the two fix commits (jwt_audience, spiffe block) — worth updating so reviewers and the changelog reflect the full scope.
  • The PR body footer says "Generated with Claude Code"; the kagenti convention is Assisted-By: Claude Code.

Assisted-By: Claude Code

identity := map[string]interface{}{}
if nsConfig.ClientAuthType == ClientAuthTypeFederatedJWT {
identity["type"] = IdentityTypeSpiffe
if nsConfig.JWTAudience != "" {

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.

suggestionidentity.type is set to spiffe unconditionally for federated-jwt, but jwt_audience only when JWTAudience != "". If authbridge-config has no JWT_AUDIENCE key — a hand-authored or pre-upgrade CM, or when KeycloakPublicURL is unset so the operator skips writing ISSUER/JWT_AUDIENCE — this emits identity.type=spiffe with no audience, reproducing the startup crash @akram reported under a narrower condition. Operator-managed CMs self-heal (they get JWT_AUDIENCE=issuer), so this is non-blocking. Consider falling back to nsConfig.Issuer here (exactly what the operator writes JWT_AUDIENCE to anyway) so the spiffe identity always carries an audience.

@akram akram left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @huang195 for the thorough review.

On the inline suggestion (jwt_audience fallback): Good catch. The operator-managed CMs always have JWT_AUDIENCE populated (it's written by buildConfigMapData as issuer), so the zero-audience path only triggers on hand-authored CMs. I'd rather fail loudly there than silently fall back to a potentially wrong audience — the crash message is clear and actionable. But I'm open to adding a log warning when identity.type=spiffe is emitted without jwt_audience, so operators get a heads-up before the pod crashes.

On the cross-repo cleanup: Agreed, I'll file a follow-up to clean up the dead SPIRE_ENABLED and envoy-config writes in the kagenti backend.

On the spireEnabled/identity.type lockstep fragility: Noted. The mTLS-baseline auto-enable keeps them in sync today, and the migration window is the only risk zone. Worth a defensive check in a future PR but not a blocker here.

Validation summary:

  • kind e2e: 41/41 specs PASSED
  • ROSA HCP (fresh namespace redbank-demo-pr447): test-token-exchange 3/3 PASSED, test-e2e PASSED

Approving and merging.

@akram akram changed the title refactor: remove SPIRE_ENABLED from authbridge-config and per-namespace envoy-config CM Refactor: remove SPIRE_ENABLED from authbridge-config and per-namespace envoy-config CM Jun 20, 2026
@akram
akram force-pushed the spire-enabled-update branch from a295819 to 3a3dd20 Compare June 20, 2026 06:45
@akram

akram commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

LGTM

letv1nnn and others added 4 commits June 20, 2026 15:29
…ce envoy-config CM

RHAIENG-4941: mTLS is now baseline. Two per-namespace configuration
levers are redundant and removed:

1. SPIRE_ENABLED key in authbridge-config CM — clientregistration
   controller now derives spireEnabled from r.SpireTrustDomain != ""
   instead of reading the CM. authbridgeconfig controller no longer
   writes this key.

2. Per-namespace envoy-config CM — no longer read by the webhook.
   Envoy config is always generated from template. The envoy-config
   volume is Optional so pods don't fail when the CM is absent.
   Per-agent envoy CM is always created in envoy-sidecar mode.

Container env var SPIRE_ENABLED in container_builder.go is untouched
(gates spiffe-helper in combined image, sourced from injection
decision, not CM).

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
…SPIFFE

The synthesizePipeline function was setting identity.type=spiffe for
federated-jwt auth but not propagating the JWT_AUDIENCE from the
authbridge-config ConfigMap. This caused authbridge-proxy to crash with
"identity.type=spiffe requires identity.jwt_audience to be set".

Read JWT_AUDIENCE from the namespace ConfigMap and inject it into the
token-exchange pipeline identity block alongside the type.

Co-authored-by: Akram Ben Aissi <akram.benaissi@gmail.com>
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
…PIRE enabled

The authbridge binary creates an in-process SPIFFE Provider (Workload
API client) only when the top-level spiffe: block is present in
config.yaml. Without it, token-exchange in spiffe mode fails with
"spiffe identity requires a SPIFFE provider to be injected".

Add spireEnabled bool parameter to ensurePerAgentConfigMap. When true,
inject spiffe: {socket: <platform_socket_path>}. When false, scrub any
stale spiffe block from the base YAML.

Co-authored-by: Akram Ben Aissi <akram.benaissi@gmail.com>
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Artem Lytvyn <alytvyn@redhat.com>
Signed-off-by: Akram <akram.benaissi@gmail.com>
@akram
akram force-pushed the spire-enabled-update branch from 3a3dd20 to 7fc9e03 Compare June 20, 2026 13:32

@kevincogan kevincogan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I took a pass over the latest head, and the changes look good to me.

I do not see anything from my side that should block this, assuming the remaining checks are green. Nice work!

@akram
akram merged commit 2e9f614 into rossoctl:main Jun 20, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants