fix(operator): inject THV_SESSION_REDIS_PASSWORD for MCPServer#5286
Open
dallinstevens wants to merge 2 commits into
Open
fix(operator): inject THV_SESSION_REDIS_PASSWORD for MCPServer#5286dallinstevens wants to merge 2 commits into
dallinstevens wants to merge 2 commits into
Conversation
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 stacklok#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 stacklok#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)
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.
54d4187 to
a6af4da
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5286 +/- ##
==========================================
+ Coverage 68.37% 68.43% +0.05%
==========================================
Files 619 619
Lines 63318 63335 +17
==========================================
+ Hits 43293 43342 +49
+ Misses 16794 16758 -36
- Partials 3231 3235 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
`MCPServer.spec.sessionStorage.passwordRef` is accepted by the CRD schema, and the runner reads `THV_SESSION_REDIS_PASSWORD` (see `pkg/transport/session/redis_config.go`), but the operator never wires the two together. `populateScalingConfig` in `cmd/thv-operator/controllers/mcpserver_runconfig.go` copies only `Address`/`DB`/`KeyPrefix` into RunConfig, and no env var is appended to the proxy runner container.
Result: any `MCPServer` pointed at a password-protected Redis fails on startup with:
```
{"level":"ERROR","msg":"error executing command","error":"failed to create Redis session storage: failed to connect to redis: NOAUTH Authentication required."}
```
reproducible at v0.27.2 and on `main` as of this PR's base.
Why this was missing
PR #4368 (which introduced MCPServer runconfig wiring) explicitly noted:
The env-var injection then landed for `VirtualMCPServer` in #4215 via `buildRedisPasswordEnvVar` — but the parallel MCPServer change wasn't made. From the user's side, the field looks supported (CRD validation passes, runner accepts the value) but silently no-ops at runtime.
Fix
Mirror the VirtualMCPServer pattern:
Scope
Test plan
Related