From 3355bb5bda54685888c0f0061d3031cac77c9cf9 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Wed, 20 May 2026 15:51:26 -0400 Subject: [PATCH 1/3] fix(injector): Mount spire-agent-socket into authbridge containers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #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) Signed-off-by: Hai Huang --- .../webhook/injector/container_builder.go | 26 ++++++ .../injector/container_builder_test.go | 80 +++++++++++++++++++ 2 files changed, 106 insertions(+) diff --git a/kagenti-operator/internal/webhook/injector/container_builder.go b/kagenti-operator/internal/webhook/injector/container_builder.go index 923894a7..ecc1a8de 100644 --- a/kagenti-operator/internal/webhook/injector/container_builder.go +++ b/kagenti-operator/internal/webhook/injector/container_builder.go @@ -123,6 +123,19 @@ func (b *ContainerBuilder) BuildEnvoyProxyContainerWithSpireOption(spireEnabled MountPath: "/etc/spiffe-helper", ReadOnly: true, }, + // SPIRE workload-API socket. The bundled spiffe-helper inside + // the combined image dials unix:///spiffe-workload-api/spire-agent.sock + // (per defaults.go SocketPath). The CSI volume `spire-agent-socket` + // is already declared at the Pod level in volume_builder.go; + // without this mount the helper sits in a silent dial-loop and + // never writes /opt/svid*.pem or /opt/jwt_svid.token. Hidden + // until kagenti-extensions PR #424 (mTLS) became the first + // real consumer of SPIRE-issued material. + corev1.VolumeMount{ + Name: "spire-agent-socket", + MountPath: "/spiffe-workload-api", + ReadOnly: true, + }, ) } @@ -235,6 +248,19 @@ func (b *ContainerBuilder) BuildProxySidecarContainerWithPorts(spireEnabled bool MountPath: "/etc/spiffe-helper", ReadOnly: true, }, + // SPIRE workload-API socket. The bundled spiffe-helper inside + // the combined image dials unix:///spiffe-workload-api/spire-agent.sock + // (per defaults.go SocketPath). The CSI volume `spire-agent-socket` + // is already declared at the Pod level in volume_builder.go; + // without this mount the helper sits in a silent dial-loop and + // never writes /opt/svid*.pem or /opt/jwt_svid.token. Hidden + // until kagenti-extensions PR #424 (mTLS) became the first + // real consumer of SPIRE-issued material. + corev1.VolumeMount{ + Name: "spire-agent-socket", + MountPath: "/spiffe-workload-api", + ReadOnly: true, + }, ) } diff --git a/kagenti-operator/internal/webhook/injector/container_builder_test.go b/kagenti-operator/internal/webhook/injector/container_builder_test.go index ecbfb133..7b35df59 100644 --- a/kagenti-operator/internal/webhook/injector/container_builder_test.go +++ b/kagenti-operator/internal/webhook/injector/container_builder_test.go @@ -385,3 +385,83 @@ func TestBuildProxySidecarContainer_SpireEnabled(t *testing.T) { t.Error("svid-output volume mount should be present when SPIRE is enabled") } } + +// TestBuildEnvoyProxyContainer_SpireEnabled_HasSocketMount asserts that +// the SPIRE workload-API socket volume is mounted into the envoy-proxy +// container when SPIRE is on. The bundled spiffe-helper inside the +// combined image dials this socket; without the mount it sits in a +// silent dial-loop and never writes /opt/svid*.pem. +func TestBuildEnvoyProxyContainer_SpireEnabled_HasSocketMount(t *testing.T) { + builder := NewContainerBuilder(config.CompiledDefaults()) + container := builder.BuildEnvoyProxyContainerWithSpireOption(true) + + found := false + for _, vm := range container.VolumeMounts { + if vm.Name == "spire-agent-socket" { + found = true + if vm.MountPath != "/spiffe-workload-api" { + t.Errorf("spire-agent-socket mount path = %q, want /spiffe-workload-api (matches defaults.go SocketPath)", vm.MountPath) + } + if !vm.ReadOnly { + t.Error("spire-agent-socket mount should be read-only (CSI volume itself is read-only)") + } + break + } + } + if !found { + t.Error("envoy-proxy container missing spire-agent-socket mount when SPIRE is enabled — bundled spiffe-helper can't reach the workload API") + } +} + +// TestBuildEnvoyProxyContainer_SpireDisabled_NoSocketMount: with SPIRE +// off the socket mount must be absent — there's no spiffe-helper to +// dial the socket, and mounting it would still try to schedule the +// CSI volume. +func TestBuildEnvoyProxyContainer_SpireDisabled_NoSocketMount(t *testing.T) { + builder := NewContainerBuilder(config.CompiledDefaults()) + container := builder.BuildEnvoyProxyContainerWithSpireOption(false) + + for _, vm := range container.VolumeMounts { + if vm.Name == "spire-agent-socket" { + t.Error("envoy-proxy container should NOT have spire-agent-socket mount when SPIRE is disabled") + } + } +} + +// TestBuildProxySidecarContainer_SpireEnabled_HasSocketMount: same as +// the envoy-proxy variant but for the proxy-sidecar combined image. +// The bundled spiffe-helper has the same workload-API requirement. +func TestBuildProxySidecarContainer_SpireEnabled_HasSocketMount(t *testing.T) { + builder := NewContainerBuilder(config.CompiledDefaults()) + container := builder.BuildProxySidecarContainer(true) + + found := false + for _, vm := range container.VolumeMounts { + if vm.Name == "spire-agent-socket" { + found = true + if vm.MountPath != "/spiffe-workload-api" { + t.Errorf("spire-agent-socket mount path = %q, want /spiffe-workload-api (matches defaults.go SocketPath)", vm.MountPath) + } + if !vm.ReadOnly { + t.Error("spire-agent-socket mount should be read-only (CSI volume itself is read-only)") + } + break + } + } + if !found { + t.Error("proxy-sidecar container missing spire-agent-socket mount when SPIRE is enabled — bundled spiffe-helper can't reach the workload API") + } +} + +// TestBuildProxySidecarContainer_SpireDisabled_NoSocketMount mirrors +// the envoy-proxy negative test for the proxy-sidecar variant. +func TestBuildProxySidecarContainer_SpireDisabled_NoSocketMount(t *testing.T) { + builder := NewContainerBuilder(config.CompiledDefaults()) + container := builder.BuildProxySidecarContainer(false) + + for _, vm := range container.VolumeMounts { + if vm.Name == "spire-agent-socket" { + t.Error("proxy-sidecar container should NOT have spire-agent-socket mount when SPIRE is disabled") + } + } +} From 15426e0576494315c809cc6ba6433a07c18e63bf Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Wed, 20 May 2026 16:12:12 -0400 Subject: [PATCH 2/3] refactor(injector): Derive spire-agent-socket mount path from SpiffeConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on PR #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) Signed-off-by: Hai Huang --- .../webhook/injector/container_builder.go | 54 +++++++++++++------ .../injector/container_builder_test.go | 30 ++++++++--- 2 files changed, 61 insertions(+), 23 deletions(-) diff --git a/kagenti-operator/internal/webhook/injector/container_builder.go b/kagenti-operator/internal/webhook/injector/container_builder.go index ecc1a8de..d3e423a6 100644 --- a/kagenti-operator/internal/webhook/injector/container_builder.go +++ b/kagenti-operator/internal/webhook/injector/container_builder.go @@ -124,16 +124,19 @@ func (b *ContainerBuilder) BuildEnvoyProxyContainerWithSpireOption(spireEnabled ReadOnly: true, }, // SPIRE workload-API socket. The bundled spiffe-helper inside - // the combined image dials unix:///spiffe-workload-api/spire-agent.sock - // (per defaults.go SocketPath). The CSI volume `spire-agent-socket` - // is already declared at the Pod level in volume_builder.go; - // without this mount the helper sits in a silent dial-loop and - // never writes /opt/svid*.pem or /opt/jwt_svid.token. Hidden - // until kagenti-extensions PR #424 (mTLS) became the first - // real consumer of SPIRE-issued material. + // the combined image dials the SocketPath from defaults.go's + // SpiffeConfig (default unix:///spiffe-workload-api/spire-agent.sock). + // The CSI volume `spire-agent-socket` is already declared at + // the Pod level in volume_builder.go; without this mount the + // helper sits in a silent dial-loop and never writes + // /opt/svid*.pem or /opt/jwt_svid.token. Hidden until + // kagenti-extensions PR #424 (mTLS) became the first real + // consumer of SPIRE-issued material. Mount path derived from + // SpiffeConfig.SocketPath so the three sites — defaults.go, + // here, and the proxy-sidecar variant below — can never drift. corev1.VolumeMount{ Name: "spire-agent-socket", - MountPath: "/spiffe-workload-api", + MountPath: spireSocketDir(b.cfg.Spiffe.SocketPath), ReadOnly: true, }, ) @@ -200,6 +203,27 @@ func spireEnabledStr(b bool) string { return "false" } +// spireSocketDir returns the directory portion of the SPIRE workload-API +// socket path, suitable for use as a container mountPath. Drops the +// "unix://" scheme prefix and strips the trailing socket filename so +// e.g. "unix:///spiffe-workload-api/spire-agent.sock" yields +// "/spiffe-workload-api". Single source of truth: defaults.go's +// SpiffeConfig.SocketPath. Returns "" when the input has no path +// component (caller should fall back to a hardcoded default in that +// edge case, but the configured default is a well-formed unix:// URI). +func spireSocketDir(socketPath string) string { + stripped := strings.TrimPrefix(socketPath, "unix://") + if stripped == "" || stripped == "/" { + return "" + } + // path.Dir would import "path"; using strings to avoid the + // dependency for a one-liner. + if i := strings.LastIndex(stripped, "/"); i > 0 { + return stripped[:i] + } + return "" +} + // BuildProxySidecarContainer creates a combined authbridge container for proxy-sidecar mode. // Uses the authbridge image (authbridge-proxy + spiffe-helper bundled, no Envoy). // The app uses HTTP_PROXY env vars to route outbound traffic through the forward proxy. @@ -248,17 +272,13 @@ func (b *ContainerBuilder) BuildProxySidecarContainerWithPorts(spireEnabled bool MountPath: "/etc/spiffe-helper", ReadOnly: true, }, - // SPIRE workload-API socket. The bundled spiffe-helper inside - // the combined image dials unix:///spiffe-workload-api/spire-agent.sock - // (per defaults.go SocketPath). The CSI volume `spire-agent-socket` - // is already declared at the Pod level in volume_builder.go; - // without this mount the helper sits in a silent dial-loop and - // never writes /opt/svid*.pem or /opt/jwt_svid.token. Hidden - // until kagenti-extensions PR #424 (mTLS) became the first - // real consumer of SPIRE-issued material. + // SPIRE workload-API socket. See the matching mount in + // BuildEnvoyProxyContainerWithSpireOption for the full + // rationale. Mount path derived from SpiffeConfig.SocketPath + // (single source of truth in defaults.go). corev1.VolumeMount{ Name: "spire-agent-socket", - MountPath: "/spiffe-workload-api", + MountPath: spireSocketDir(b.cfg.Spiffe.SocketPath), ReadOnly: true, }, ) diff --git a/kagenti-operator/internal/webhook/injector/container_builder_test.go b/kagenti-operator/internal/webhook/injector/container_builder_test.go index 7b35df59..3e1891cb 100644 --- a/kagenti-operator/internal/webhook/injector/container_builder_test.go +++ b/kagenti-operator/internal/webhook/injector/container_builder_test.go @@ -392,15 +392,26 @@ func TestBuildProxySidecarContainer_SpireEnabled(t *testing.T) { // combined image dials this socket; without the mount it sits in a // silent dial-loop and never writes /opt/svid*.pem. func TestBuildEnvoyProxyContainer_SpireEnabled_HasSocketMount(t *testing.T) { - builder := NewContainerBuilder(config.CompiledDefaults()) + cfg := config.CompiledDefaults() + builder := NewContainerBuilder(cfg) container := builder.BuildEnvoyProxyContainerWithSpireOption(true) + // Derive the expected mount path from SpiffeConfig.SocketPath the + // same way the production code does, so a future change to the + // canonical SocketPath in defaults.go can't leave this test + // asserting against a stale literal. + wantPath := spireSocketDir(cfg.Spiffe.SocketPath) + if wantPath == "" { + t.Fatalf("spireSocketDir(%q) returned empty — defaults must declare a valid socket path", cfg.Spiffe.SocketPath) + } + found := false for _, vm := range container.VolumeMounts { if vm.Name == "spire-agent-socket" { found = true - if vm.MountPath != "/spiffe-workload-api" { - t.Errorf("spire-agent-socket mount path = %q, want /spiffe-workload-api (matches defaults.go SocketPath)", vm.MountPath) + if vm.MountPath != wantPath { + t.Errorf("spire-agent-socket mount path = %q, want %q (derived from SpiffeConfig.SocketPath %q)", + vm.MountPath, wantPath, cfg.Spiffe.SocketPath) } if !vm.ReadOnly { t.Error("spire-agent-socket mount should be read-only (CSI volume itself is read-only)") @@ -432,15 +443,22 @@ func TestBuildEnvoyProxyContainer_SpireDisabled_NoSocketMount(t *testing.T) { // the envoy-proxy variant but for the proxy-sidecar combined image. // The bundled spiffe-helper has the same workload-API requirement. func TestBuildProxySidecarContainer_SpireEnabled_HasSocketMount(t *testing.T) { - builder := NewContainerBuilder(config.CompiledDefaults()) + cfg := config.CompiledDefaults() + builder := NewContainerBuilder(cfg) container := builder.BuildProxySidecarContainer(true) + wantPath := spireSocketDir(cfg.Spiffe.SocketPath) + if wantPath == "" { + t.Fatalf("spireSocketDir(%q) returned empty — defaults must declare a valid socket path", cfg.Spiffe.SocketPath) + } + found := false for _, vm := range container.VolumeMounts { if vm.Name == "spire-agent-socket" { found = true - if vm.MountPath != "/spiffe-workload-api" { - t.Errorf("spire-agent-socket mount path = %q, want /spiffe-workload-api (matches defaults.go SocketPath)", vm.MountPath) + if vm.MountPath != wantPath { + t.Errorf("spire-agent-socket mount path = %q, want %q (derived from SpiffeConfig.SocketPath %q)", + vm.MountPath, wantPath, cfg.Spiffe.SocketPath) } if !vm.ReadOnly { t.Error("spire-agent-socket mount should be read-only (CSI volume itself is read-only)") From a6447313214419d77856c65a0289ba541832b516 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Wed, 20 May 2026 19:13:05 -0400 Subject: [PATCH 3/3] =?UTF-8?q?refactor(injector):=20Address=20review=20ni?= =?UTF-8?q?ts=20=E2=80=94=20use=20path.Dir,=20trim=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two nits from @pdettori on PR #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) Signed-off-by: Hai Huang --- .../webhook/injector/container_builder.go | 40 +++++-------------- 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/kagenti-operator/internal/webhook/injector/container_builder.go b/kagenti-operator/internal/webhook/injector/container_builder.go index d3e423a6..2af61b7f 100644 --- a/kagenti-operator/internal/webhook/injector/container_builder.go +++ b/kagenti-operator/internal/webhook/injector/container_builder.go @@ -18,6 +18,7 @@ package injector import ( "fmt" + "path" "strconv" "strings" @@ -123,17 +124,8 @@ func (b *ContainerBuilder) BuildEnvoyProxyContainerWithSpireOption(spireEnabled MountPath: "/etc/spiffe-helper", ReadOnly: true, }, - // SPIRE workload-API socket. The bundled spiffe-helper inside - // the combined image dials the SocketPath from defaults.go's - // SpiffeConfig (default unix:///spiffe-workload-api/spire-agent.sock). - // The CSI volume `spire-agent-socket` is already declared at - // the Pod level in volume_builder.go; without this mount the - // helper sits in a silent dial-loop and never writes - // /opt/svid*.pem or /opt/jwt_svid.token. Hidden until - // kagenti-extensions PR #424 (mTLS) became the first real - // consumer of SPIRE-issued material. Mount path derived from - // SpiffeConfig.SocketPath so the three sites — defaults.go, - // here, and the proxy-sidecar variant below — can never drift. + // SPIRE workload-API socket — bundled spiffe-helper dials it. + // Path derived from SpiffeConfig.SocketPath (defaults.go). corev1.VolumeMount{ Name: "spire-agent-socket", MountPath: spireSocketDir(b.cfg.Spiffe.SocketPath), @@ -204,24 +196,16 @@ func spireEnabledStr(b bool) string { } // spireSocketDir returns the directory portion of the SPIRE workload-API -// socket path, suitable for use as a container mountPath. Drops the -// "unix://" scheme prefix and strips the trailing socket filename so -// e.g. "unix:///spiffe-workload-api/spire-agent.sock" yields -// "/spiffe-workload-api". Single source of truth: defaults.go's -// SpiffeConfig.SocketPath. Returns "" when the input has no path -// component (caller should fall back to a hardcoded default in that -// edge case, but the configured default is a well-formed unix:// URI). +// socket path (e.g. "unix:///spiffe-workload-api/spire-agent.sock" → +// "/spiffe-workload-api"), suitable for use as a container mountPath. +// Single source of truth: defaults.go's SpiffeConfig.SocketPath. func spireSocketDir(socketPath string) string { stripped := strings.TrimPrefix(socketPath, "unix://") - if stripped == "" || stripped == "/" { + dir := path.Dir(stripped) + if dir == "." || dir == "/" { return "" } - // path.Dir would import "path"; using strings to avoid the - // dependency for a one-liner. - if i := strings.LastIndex(stripped, "/"); i > 0 { - return stripped[:i] - } - return "" + return dir } // BuildProxySidecarContainer creates a combined authbridge container for proxy-sidecar mode. @@ -272,10 +256,8 @@ func (b *ContainerBuilder) BuildProxySidecarContainerWithPorts(spireEnabled bool MountPath: "/etc/spiffe-helper", ReadOnly: true, }, - // SPIRE workload-API socket. See the matching mount in - // BuildEnvoyProxyContainerWithSpireOption for the full - // rationale. Mount path derived from SpiffeConfig.SocketPath - // (single source of truth in defaults.go). + // SPIRE workload-API socket — bundled spiffe-helper dials it. + // Path derived from SpiffeConfig.SocketPath (defaults.go). corev1.VolumeMount{ Name: "spire-agent-socket", MountPath: spireSocketDir(b.cfg.Spiffe.SocketPath),