fix(injector): Mount spire-agent-socket into authbridge containers - #367
Conversation
…onfig Address review feedback on PR rossoctl#367. The mount path was duplicated as a hardcoded "/spiffe-workload-api" literal in two volumeMount sites, while the canonical SocketPath ("unix:///spiffe-workload-api/spire-agent.sock") already lives in defaults.go's SpiffeConfig. A future change to the SocketPath would silently leave the mount paths stale — the spiffe-helper would dial one path and find no socket at the other. Add a small spireSocketDir() helper that strips the "unix://" scheme prefix and the trailing socket filename, returning the directory portion suitable for use as a container mountPath. Both sites (BuildEnvoyProxyContainerWithSpireOption and BuildProxySidecarContainerWithPorts) now compute their mount path from b.cfg.Spiffe.SocketPath via this helper, so defaults.go is the single source of truth. Tests updated to derive the expected path the same way (rather than asserting against a hardcoded string), so the test stays in lockstep with any future SocketPath change. No behavior change at the canonical default — spireSocketDir( "unix:///spiffe-workload-api/spire-agent.sock") still returns "/spiffe-workload-api". This is a pure refactor for maintainability. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
pdettori
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix that adds the missing spire-agent-socket volume mount to both authbridge container variants when SPIRE is enabled. The spireSocketDir helper correctly derives the mount path from the canonical SpiffeConfig.SocketPath in defaults.go, eliminating drift risk. Tests are symmetric and properly derive expected values from the same source of truth.
Areas reviewed: Go (production + test), commit conventions, security
Commits: 2 commits, both signed-off ✓
CI: all 15 checks passing (Unit, Integration, E2E, Lint, CodeQL, Trivy)
| if stripped == "" || stripped == "/" { | ||
| return "" | ||
| } | ||
| // path.Dir would import "path"; using strings to avoid the |
There was a problem hiding this comment.
nit: The comment says path.Dir would "import path" as if that's costly — but path is stdlib (no binary bloat, no vendor impact). Not wrong to use strings, but the justification overstates the cost. Consider either using path.Dir for clarity or dropping the comment since the code is self-explanatory.
There was a problem hiding this comment.
Fair point — switched to path.Dir and dropped the misleading "stdlib import would be costly" comment. a644731.
| MountPath: "/etc/spiffe-helper", | ||
| ReadOnly: true, | ||
| }) | ||
| // SPIRE workload-API socket. The bundled spiffe-helper inside |
There was a problem hiding this comment.
nit: The 11-line comment block is thorough but could be trimmed to ~3 lines — the PR description and commit message already document the full rationale (why it was hidden, what consumes it). Long inline comments inside functions tend to go stale. The key info for a reader is: "Mount the CSI socket volume so the bundled spiffe-helper can reach the workload API. Path derived from SpiffeConfig.SocketPath."
There was a problem hiding this comment.
Trimmed both mount sites to two lines stating what the mount is for. Same trim applied to the proxy-sidecar variant for symmetry. a644731.
The webhook creates the spire-agent-socket POD-level volume (CSI driver csi.spiffe.io) when spire-enabled is true, but never mounts it into either authbridge container. The bundled spiffe-helper inside the combined authbridge images dials unix:///spiffe-workload-api/spire-agent.sock (per defaults.go SocketPath) and sits in a silent dial-loop forever because the directory doesn't exist in the container. Hidden until now because no in-production workload actually consumes SPIRE-issued material: - token-exchange uses /shared/client-secret.txt (Keycloak client_credentials), not JWT-SVIDs. - jwt-validation uses JWKS, not SPIRE. - /opt/svid*.pem and /opt/jwt_svid.token sit empty in every spire-enabled pod (verified — weather-agent ran 3+ days with kagenti.io/spire=enabled and /opt empty the whole time). kagenti-extensions PR rossoctl#424 (mTLS between authbridge sidecars via SPIRE X.509 SVIDs) is the first workload that actually consumes SPIRE material. Without this fix, the mTLS feature can't function because tls.ServerConfig errors at construction when /opt/svid.pem is missing. This completes the bundled-spiffe-helper migration alluded to in injection_decision.go's TODO — when the standalone spiffe-helper sidecar was removed and the helper moved into the combined authbridge images, the volume mount got lost. Adds the mount to both: - BuildEnvoyProxyContainerWithSpireOption (envoy-sidecar mode) - BuildProxySidecarContainerWithPorts (proxy-sidecar / lite modes) Mount path /spiffe-workload-api matches the SocketPath in defaults.go. ReadOnly is correct (the CSI volume itself is already read-only at the Pod level). Tests added: TestBuildEnvoyProxyContainer_SpireEnabled_HasSocketMount and TestBuildProxySidecarContainer_SpireEnabled_HasSocketMount lock the mount presence + path + ReadOnly when SPIRE is on; the matching SpireDisabled tests assert the mount is absent when SPIRE is off. Verified: all unit tests in internal/webhook/injector/ pass. Operator image rebuilt and rollout-restarted; mtls demo pods now have the new mount in their spec (kubelet attempts to mount, which is the correct change). Full end-to-end mTLS demo verification deferred to the kagenti-extensions PR rossoctl#424 reviewer once the SPIRE agent + CSI driver are restored on the test cluster — those were absent during local verification, blocking the mount-success path but not the operator's own correctness. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
…onfig Address review feedback on PR rossoctl#367. The mount path was duplicated as a hardcoded "/spiffe-workload-api" literal in two volumeMount sites, while the canonical SocketPath ("unix:///spiffe-workload-api/spire-agent.sock") already lives in defaults.go's SpiffeConfig. A future change to the SocketPath would silently leave the mount paths stale — the spiffe-helper would dial one path and find no socket at the other. Add a small spireSocketDir() helper that strips the "unix://" scheme prefix and the trailing socket filename, returning the directory portion suitable for use as a container mountPath. Both sites (BuildEnvoyProxyContainerWithSpireOption and BuildProxySidecarContainerWithPorts) now compute their mount path from b.cfg.Spiffe.SocketPath via this helper, so defaults.go is the single source of truth. Tests updated to derive the expected path the same way (rather than asserting against a hardcoded string), so the test stays in lockstep with any future SocketPath change. No behavior change at the canonical default — spireSocketDir( "unix:///spiffe-workload-api/spire-agent.sock") still returns "/spiffe-workload-api". This is a pure refactor for maintainability. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
3bd3239 to
15426e0
Compare
Two nits from @pdettori on PR rossoctl#367: 1. `spireSocketDir` used `strings.LastIndex` with a comment justifying "avoid importing path" — but `path` is stdlib, no real cost. Switch to `path.Dir`. Drops 4 LOC of state-keeping and the misleading comment; behavior preserved (still returns "" for inputs with no usable directory component). 2. The 11-line comment block above the `spire-agent-socket` mount restated context already in the PR description and commit message. Trim to two lines stating *what* the mount is for. Same trim applied to the proxy-sidecar variant for symmetry. Net: -29 LOC of comments + 4 LOC of code, +11 LOC (cleaner helper + shorter comments). Behavior unchanged; tests pass unchanged. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Summary
The webhook creates the
spire-agent-socketPOD-level volume (CSI drivercsi.spiffe.io) whenspire-enabledis true, but never mounts it into either authbridge container. The bundledspiffe-helperinside the combined authbridge images dialsunix:///spiffe-workload-api/spire-agent.sock(perinternal/webhook/config/defaults.go:73'sSocketPath) and sits in a silent dial-loop forever because the directory doesn't exist in the container.This PR completes the bundled-spiffe-helper migration alluded to in
injection_decision.go:22's TODO — when the standalone spiffe-helper sidecar was removed and the helper moved into the combined authbridge images, the volume mount got lost.Why this was hidden
No in-production workload actually consumes SPIRE-issued material today:
token-exchangeuses/shared/client-secret.txt(Keycloakclient_credentialsfrom a Secret the operator provisions), not JWT-SVIDs.jwt-validationuses JWKS over HTTP, not SPIRE./opt/svid*.pemand/opt/jwt_svid.tokensit empty in every spire-enabled pod. Verified:weather-agenthas been running 3+ days withkagenti.io/spire: enabledand/opt/empty the whole time.kagenti-extensionsPR #424 (mTLS between authbridge sidecars via SPIRE X.509 SVIDs) is the first workload that actually consumes SPIRE material. Without this fix, the mTLS feature can't function —tls.ServerConfigerrors at construction when/opt/svid.pemis missing.Change
Two locations in
internal/webhook/injector/container_builder.go:BuildEnvoyProxyContainerWithSpireOption(envoy-sidecar mode)BuildProxySidecarContainerWithPorts(proxy-sidecar / lite modes)Both append
spire-agent-socket → /spiffe-workload-api(ReadOnly) inside the existingif spireEnabledblock.The mount path
/spiffe-workload-apimatches theSocketPathindefaults.go. ReadOnly is correct — the CSI volume itself is already declared read-only at the Pod level (volume_builder.go:170).Test plan
go test -race -count=1 ./internal/webhook/injector/...— all pass, including new tests:TestBuildEnvoyProxyContainer_SpireEnabled_HasSocketMount— locks mount presence + path + ReadOnly when SPIRE is onTestBuildEnvoyProxyContainer_SpireDisabled_NoSocketMount— asserts absent when SPIRE is offTestBuildProxySidecarContainer_SpireEnabled_HasSocketMount— same shape for proxy-sidecarTestBuildProxySidecarContainer_SpireDisabled_NoSocketMount— proxy-sidecar negative casego vet ./...,gofmt -lcleanFiles
internal/webhook/injector/container_builder.go— 2 mount additions, ~24 LOC including commentsinternal/webhook/injector/container_builder_test.go— 4 new test functions, ~80 LOCNet: +106 / 0.
Out of scope
MTLSModeresolution chain (AgentRuntime CR spec + namespace ConfigMap + per-agent ConfigMap merge). Deferred to a follow-up operator PR. Keeps the demo's namespace-ConfigMap patch as the interim until that lands.kagenti.io/spiffe-helper-injectlabel tokagenti.io/spire-enabled(TODO atinjection_decision.go:22). Separate breaking-label PR.Release coordination
This fix should be released alongside
kagenti-extensionsPR #424 (or before it). Without this operator fix landing first, the authbridge mTLS feature can't function in any deployment.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com