From 736dded3194178386d64b072b93d9376bb438308 Mon Sep 17 00:00:00 2001 From: Dallin Stevens Date: Thu, 14 May 2026 16:32:13 -0500 Subject: [PATCH 1/2] fix(operator): inject THV_SESSION_REDIS_PASSWORD for MCPServer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MCPServer.spec.sessionStorage.passwordRef is accepted by the CRD schema and the runner reads THV_SESSION_REDIS_PASSWORD from pkg/transport/session/redis_config.go, but the operator never bridged the two: populateScalingConfig in mcpserver_runconfig.go copies only Address/DB/KeyPrefix into RunConfig, and no env var is added to the proxy runner container. Result: any MCPServer that points its session storage at a password-protected Redis fails on connect with "NOAUTH Authentication required". PR #4368 (which introduced the runconfig wiring) explicitly noted in its summary that "the Redis password is intentionally excluded and injected as a pod env var instead". The env-var injection landed for VirtualMCPServer in #4215 / buildRedisPasswordEnvVar but the parallel MCPServer change was never made. This change closes that gap by mirroring the VirtualMCPServer pattern: - Add buildMCPServerRedisPasswordEnvVar in mcpserver_controller.go which returns []corev1.EnvVar with a single SecretKeyRef-backed THV_SESSION_REDIS_PASSWORD entry when sessionStorage.provider == "redis" and passwordRef is set; returns nil otherwise. - Append the helper's output to the proxy container's env slice in the MCPServer reconciler, immediately before user-supplied resourceOverrides.proxyDeployment.env so user overrides can still win on name collisions. - Add TestBuildMCPServerRedisPasswordEnvVar mirroring TestBuildRedisPasswordEnvVar for VirtualMCPServer (nil storage, memory provider, redis without passwordRef, redis with passwordRef). - No CRD changes — the schema already supports passwordRef. Tested: - go build ./cmd/thv-operator/... succeeds - go test ./cmd/thv-operator/controllers/ all green (3.7s) --- .../controllers/mcpserver_controller.go | 27 +++ .../controllers/mcpserver_replicas_test.go | 178 ++++++++++++++++++ 2 files changed, 205 insertions(+) diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index d911a307c4..f7bba5d72e 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -42,6 +42,7 @@ import ( "github.com/stacklok/toolhive/cmd/thv-operator/pkg/validation" "github.com/stacklok/toolhive/pkg/container/kubernetes" "github.com/stacklok/toolhive/pkg/transport" + "github.com/stacklok/toolhive/pkg/transport/session" ) // MCPServerReconciler reconciles a MCPServer object @@ -1145,6 +1146,11 @@ func (r *MCPServerReconciler) deploymentForMCPServer( } } + // Mount Redis password secret when session storage provider is Redis. + // Appended after user overrides so the secretRef-backed env wins on + // name collision (ResourceOverrides.Env only accepts plain strings). + env = append(env, r.buildRedisPasswordEnvVar(m)...) + // Add volume mounts for user-defined volumes for _, v := range m.Spec.Volumes { volumeMounts = append(volumeMounts, corev1.VolumeMount{ @@ -2447,6 +2453,27 @@ func (r *MCPServerReconciler) validateSessionStorageForReplicas(ctx context.Cont } } +// buildRedisPasswordEnvVar returns the THV_SESSION_REDIS_PASSWORD env var when +// sessionStorage.provider == "redis" and passwordRef is set; returns nil otherwise. +func (*MCPServerReconciler) buildRedisPasswordEnvVar(m *mcpv1beta1.MCPServer) []corev1.EnvVar { + if m.Spec.SessionStorage == nil || + m.Spec.SessionStorage.Provider != mcpv1beta1.SessionStorageProviderRedis || + m.Spec.SessionStorage.PasswordRef == nil { + return nil + } + return []corev1.EnvVar{{ + Name: session.RedisPasswordEnvVar, + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: m.Spec.SessionStorage.PasswordRef.Name, + }, + Key: m.Spec.SessionStorage.PasswordRef.Key, + }, + }, + }} +} + // setRateLimitConfigCondition sets the RateLimitConfigValid status condition. func setRateLimitConfigCondition(mcpServer *mcpv1beta1.MCPServer, status metav1.ConditionStatus, reason, message string) { meta.SetStatusCondition(&mcpServer.Status.Conditions, metav1.Condition{ diff --git a/cmd/thv-operator/controllers/mcpserver_replicas_test.go b/cmd/thv-operator/controllers/mcpserver_replicas_test.go index 07987edbba..8dde6ad202 100644 --- a/cmd/thv-operator/controllers/mcpserver_replicas_test.go +++ b/cmd/thv-operator/controllers/mcpserver_replicas_test.go @@ -19,6 +19,7 @@ import ( mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" "github.com/stacklok/toolhive/pkg/container/kubernetes" + "github.com/stacklok/toolhive/pkg/transport/session" ) func TestReplicaBehavior(t *testing.T) { @@ -1163,3 +1164,180 @@ func TestRateLimitConfigValidation(t *testing.T) { }) } } + +// TestMCPServerBuildRedisPasswordEnvVar tests conditional Redis password env var injection. +func TestMCPServerBuildRedisPasswordEnvVar(t *testing.T) { + t.Parallel() + + r := &MCPServerReconciler{} + passwordRef := &mcpv1beta1.SecretKeyRef{Name: "redis-secret", Key: "password"} + + tests := []struct { + name string + storage *mcpv1beta1.SessionStorageConfig + expectEnVar bool + }{ + { + name: "nil sessionStorage produces no env var", + storage: nil, + expectEnVar: false, + }, + { + name: "memory provider produces no env var", + storage: &mcpv1beta1.SessionStorageConfig{Provider: "memory"}, + expectEnVar: false, + }, + { + name: "redis without passwordRef produces no env var", + storage: &mcpv1beta1.SessionStorageConfig{Provider: mcpv1beta1.SessionStorageProviderRedis, Address: "redis:6379"}, + expectEnVar: false, + }, + { + name: "redis with passwordRef produces THV_SESSION_REDIS_PASSWORD", + storage: &mcpv1beta1.SessionStorageConfig{Provider: mcpv1beta1.SessionStorageProviderRedis, Address: "redis:6379", PasswordRef: passwordRef}, + expectEnVar: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m := &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-mcp", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerSpec{SessionStorage: tc.storage}, + } + env := r.buildRedisPasswordEnvVar(m) + if tc.expectEnVar { + require.Len(t, env, 1) + assert.Equal(t, session.RedisPasswordEnvVar, env[0].Name) + assert.Empty(t, env[0].Value, "must not use plaintext Value") + require.NotNil(t, env[0].ValueFrom) + require.NotNil(t, env[0].ValueFrom.SecretKeyRef) + assert.Equal(t, passwordRef.Name, env[0].ValueFrom.SecretKeyRef.Name) + assert.Equal(t, passwordRef.Key, env[0].ValueFrom.SecretKeyRef.Key) + } else { + assert.Empty(t, env) + } + }) + } +} + +// TestMCPServerDeploymentInjectsRedisPasswordEnvVar asserts the rendered proxy +// Deployment carries the THV_SESSION_REDIS_PASSWORD env var with a SecretKeyRef. +func TestMCPServerDeploymentInjectsRedisPasswordEnvVar(t *testing.T) { + t.Parallel() + + passwordRef := &mcpv1beta1.SecretKeyRef{Name: "redis-secret", Key: "password"} + + mcpServer := &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mcp-redis", + Namespace: "default", + }, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "test-image:latest", + Transport: "streamable-http", + ProxyPort: 8080, + SessionStorage: &mcpv1beta1.SessionStorageConfig{ + Provider: mcpv1beta1.SessionStorageProviderRedis, + Address: "redis:6379", + PasswordRef: passwordRef, + }, + }, + } + + testScheme := createTestScheme() + r := newTestMCPServerReconciler(nil, testScheme, kubernetes.PlatformKubernetes) + + deployment, err := r.deploymentForMCPServer(t.Context(), mcpServer, "test-checksum") + require.NoError(t, err) + require.NotNil(t, deployment) + require.NotEmpty(t, deployment.Spec.Template.Spec.Containers) + + // The proxy runner container is the toolhive container — scan its env. + var proxyContainer *corev1.Container + for i, c := range deployment.Spec.Template.Spec.Containers { + if c.Name == "toolhive" { + proxyContainer = &deployment.Spec.Template.Spec.Containers[i] + break + } + } + require.NotNil(t, proxyContainer, "deployment must contain the toolhive proxy container") + + var found bool + for _, e := range proxyContainer.Env { + if e.Name == session.RedisPasswordEnvVar { + found = true + assert.Empty(t, e.Value, "password must not appear as plaintext") + require.NotNil(t, e.ValueFrom) + require.NotNil(t, e.ValueFrom.SecretKeyRef) + assert.Equal(t, passwordRef.Name, e.ValueFrom.SecretKeyRef.Name) + assert.Equal(t, passwordRef.Key, e.ValueFrom.SecretKeyRef.Key) + } + } + assert.True(t, found, "deployment proxy container should contain %s env var", session.RedisPasswordEnvVar) +} + +// TestMCPServerDeploymentRedisPasswordOverridesUserEnvOnCollision asserts the +// secretRef-backed env var wins over a plaintext ResourceOverrides override +// with the same name (last-wins kubelet ordering). +func TestMCPServerDeploymentRedisPasswordOverridesUserEnvOnCollision(t *testing.T) { + t.Parallel() + + passwordRef := &mcpv1beta1.SecretKeyRef{Name: "redis-secret", Key: "password"} + + mcpServer := &mcpv1beta1.MCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-mcp-redis-collision", + Namespace: "default", + }, + Spec: mcpv1beta1.MCPServerSpec{ + Image: "test-image:latest", + Transport: "streamable-http", + ProxyPort: 8080, + SessionStorage: &mcpv1beta1.SessionStorageConfig{ + Provider: mcpv1beta1.SessionStorageProviderRedis, + Address: "redis:6379", + PasswordRef: passwordRef, + }, + ResourceOverrides: &mcpv1beta1.ResourceOverrides{ + ProxyDeployment: &mcpv1beta1.ProxyDeploymentOverrides{ + Env: []mcpv1beta1.EnvVar{ + {Name: session.RedisPasswordEnvVar, Value: "user-supplied-plaintext"}, + }, + }, + }, + }, + } + + testScheme := createTestScheme() + r := newTestMCPServerReconciler(nil, testScheme, kubernetes.PlatformKubernetes) + + deployment, err := r.deploymentForMCPServer(t.Context(), mcpServer, "test-checksum") + require.NoError(t, err) + require.NotNil(t, deployment) + + var proxyContainer *corev1.Container + for i, c := range deployment.Spec.Template.Spec.Containers { + if c.Name == "toolhive" { + proxyContainer = &deployment.Spec.Template.Spec.Containers[i] + break + } + } + require.NotNil(t, proxyContainer) + + // Find the LAST occurrence — kubelet's duplicate-name resolution is + // last-wins, so that's the one that actually applies to the container. + var last *corev1.EnvVar + for i, e := range proxyContainer.Env { + if e.Name == session.RedisPasswordEnvVar { + last = &proxyContainer.Env[i] + } + } + require.NotNil(t, last, "deployment proxy container should contain %s env var", session.RedisPasswordEnvVar) + assert.Empty(t, last.Value, "final occurrence must be the secretRef-backed one (no plaintext)") + require.NotNil(t, last.ValueFrom) + require.NotNil(t, last.ValueFrom.SecretKeyRef) + assert.Equal(t, passwordRef.Name, last.ValueFrom.SecretKeyRef.Name) + assert.Equal(t, passwordRef.Key, last.ValueFrom.SecretKeyRef.Key) +} From a6af4daaa4405a2d3c55776228de07093dd0bd68 Mon Sep 17 00:00:00 2001 From: Dallin Stevens Date: Thu, 14 May 2026 17:05:57 -0500 Subject: [PATCH 2/2] ci: retrigger after infrastructure-canceled run The previous Tests / Test Go Code job (76080809592) was killed by GitHub Actions infrastructure mid-run ("task: Signal received: \"terminated\"", "runner has received a shutdown signal") ~5.5 min into 'task test-coverage'. No test failures surfaced in the truncated logs. Push an empty commit to retrigger.