Skip to content

Enable TLS for operator-wide Redis defaults - #6838

Open
Sanskarzz wants to merge 2 commits into
stacklok:mainfrom
Sanskarzz:feat/default-redis-tls
Open

Sanskarzz wants to merge 2 commits into
stacklok:mainfrom
Sanskarzz:feat/default-redis-tls

Conversation

@Sanskarzz

@Sanskarzz Sanskarzz commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

The runtime and per-workload APIs support Redis TLS after #6731 and #6767, but workloads using the operator-wide defaultRedis fallback still cannot enable it. This prevents that fallback from connecting to TLS-only Redis endpoints.

  • Add explicit global TLS enablement and an optional CA Secret reference to Helm values.
  • Validate operator environment settings before manager startup, preventing malformed TLS configuration from silently becoming plaintext.
  • Reuse session TLS conversion, CA projection, and deployment drift handling for MCPServer, MCPRemoteProxy, and VirtualMCPServer. Rate limiters continue to share the selected session storage TLS settings through the existing runtime wiring.
  • Preserve explicit per-workload Redis overrides and plaintext behavior when TLS is disabled.

Closes #6827

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Focused operator tests through a temporary Taskfile using the repository's linker flags and race detector: controllerutil, vmcpconfig, controllers, and app.
  • Operator lint through Task with CI's pinned golangci-lint v2.14.0: 0 issues.
  • task helm-unittest: 85 operator tests and 3 CRD-chart tests passed.
  • Chart-testing ct lint --config ct.yaml through Task with explicit paths to the pinned tool's schema and YAML lint configuration: both charts and their CI values files passed.
  • task helm-docs and task license-check.
  • Codespell v2.4.3 over tracked files, including filenames and hidden files.
  • Manual testing in an isolated Kind cluster with the locally built operator, TLS-only password-protected Redis, and a projected private CA. Also verified that malformed TLS settings stop the actual operator binary before Kubernetes setup.

Live verification used published v0.54.0 workload runtimes with the modified operator:

  • MCPServer and VirtualMCPServer workloads became ready using only global Redis defaults, including the password and CA projections.
  • A Legacy MCP session initialized through vMCP, listed backend tools, and had its session key persisted in TLS-only Redis.
  • Restarting the operator with a different global CA Secret reference created new workload pods. The vMCP pod using the unrelated CA failed certificate verification and did not become ready; there was no plaintext downgrade. The previous healthy replica remained available during the failed rollout.

The full repository unit suite, task test-e2e, and all operator integration suites were not run. The installed Snap Task and linter binaries are blocked here; verification used standalone Task and CI's pinned linter.

API Compatibility

  • This PR does not break the v1beta1 API.

No CRD fields, schemas, or runtime TLS types change. Global TLS defaults to disabled; existing address/password precedence remains unchanged.

Changes

File or area Change
Operator Helm values/template TLS settings, validation, and environment bridge
controllerutil/defaultredis.go, app/app.go Parse global TLS and reject invalid settings before controller startup
controllerutil/session_redis_tls.go Reuse TLS conversion and CA projection for the selected global fallback
MCPServer/MCPRemoteProxy run-config builders and vMCP converter Forward global TLS into existing runtime configuration
MCPServer controller and vMCP Deployment builder Match mounts and drift detection to the existing fallback selection policy
Existing Go/chart tests Failures, precedence, all three workloads, CA projections, and reference changes
Chart README and architecture docs Configuration, namespace requirements, compatibility, and rotation behavior

Does this introduce a user-facing change?

Yes. Enable verified TLS for workloads using the global fallback:

operator:
  defaultRedis:
    addr: redis.example.com:6380
    existingSecret: redis-password
    existingSecretKey: password
    tls:
      enabled: true
      caCertSecretRef:
        name: redis-ca
        key: ca.crt

Omit the CA reference to use system roots. Password and CA Secrets must exist in each workload's namespace; the operator projects references into workloads without copying Secrets between namespaces.

The environment bridge is TOOLHIVE_DEFAULT_REDIS_TLS_ENABLED, TOOLHIVE_DEFAULT_REDIS_TLS_CA_SECRET_NAME, and TOOLHIVE_DEFAULT_REDIS_TLS_CA_SECRET_KEY. Global TLS always verifies certificates; it has no insecureSkipVerify option.

Implementation plan

Implementation plan
  1. Extend global defaults with the existing Redis TLS type; validate before manager startup.
  2. Add explicit Helm enablement and CA references without changing address/password resolution.
  3. Reuse runtime TLS and CA projection helpers for all three workload kinds.
  4. Verify overrides, failure handling, CA reference changes, and a live TLS-only connection.
  5. Update chart and architecture documentation.

Special notes for reviewers

  • Explicit per-workload Redis settings override the entire global connection, including TLS. Explicit Redis without TLS stays plaintext even when global TLS is enabled.
  • Preserve the pre-existing provider-selection inconsistency: MCPRemoteProxy suppresses global defaults for any explicit session storage; MCPServer/vMCP can still select global Redis for explicit provider: memory. Existing password selection is unchanged. CA projection follows each runtime selection policy.
  • TLS or CA-reference changes update the desired Deployment. Changing CA contents under the same reference requires a workload restart: the existing mount uses subPath, and the runtime loads trust at startup.
  • ReadDefaultRedisConfig retains its signature and panics on invalid TLS rather than downgrading. Normal operator startup validates first, reports the error, and exits before starting controllers.
  • No new CLI flag, Redis client, mTLS, automatic CA reload, or chart version bump. Chart versions are release-bot managed.
  • Production scope: 10 files and 178 changed lines, excluding tests/docs/generated documentation.

Allow workloads using global Redis session defaults to request verified TLS with system roots or a namespace-local CA Secret. Reuse the session TLS wiring and CA projection paths while preserving explicit workload overrides and existing plaintext defaults.

Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
@Sanskarzz Sanskarzz added the kubernetes Items related to Kubernetes label Oct 15, 2026
@Sanskarzz Sanskarzz self-assigned this Oct 15, 2026
@codecov

codecov Bot commented Oct 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.65%. Comparing base (14c6066) to head (b1a18a0).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
cmd/thv-operator/app/app.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6838      +/-   ##
==========================================
+ Coverage   79.62%   79.65%   +0.02%     
==========================================
  Files         815      816       +1     
  Lines       82955    83205     +250     
==========================================
+ Hits        66056    66278     +222     
- Misses      16894    16922      +28     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>

@reyortiz3 reyortiz3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this. I checked it against toolhive-core's redisconn and the existing pkg/redisconfig path. It reuses the existing flow (sessionRedisTLS → SessionRedisTLSConfig → redisconfig.TLSConfig → redisconn.BuildTLSConfig) and adds no TLS handling of its own, so I see no conflict with core. I also checked the other callers of the changed helpers, and none is affected by the new nil→default behavior.

Two suggestions, both on choices the description already calls deliberate:

  1. sessionRedisTLS(nil) now reads the env and can panic. I understand the intent, and the startup validation makes it unreachable in the operator. A helper that was a pure function of its argument still gains a hidden env read, and each populateScalingConfig* re-parses the env. Could the default be read and validated once at startup and passed in?

  2. The fallback policy is encoded twice. SessionRedisTLSVolumesWithDefault and the populateScalingConfig* functions each decide when the global default applies. If they drift, a pod gets a CA mount without Redis, or Redis TLS without the CA. Could a test pin the two together?

Minor: a missing CA Secret only appears as a pod FailedMount, and the global default affects every workload that falls back to it. A line in the docs about that symptom would help. A values.schema.json might also replace some of the Helm fail checks, if the chart uses one.

Separately, not for this PR: the shared TLSConfig from #6731 duplicates storage.RedisTLSRunConfig (same two fields, different JSON tag casing). I'd like a follow-up to either upstream the CA-file loading into redisconn or unify the two types.

@reyortiz3

Copy link
Copy Markdown
Collaborator

Also, adding some of this to stacklok/toolhive-core#386 so we can re-use it later

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kubernetes Items related to Kubernetes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support TLS for operator-wide default Redis session storage

2 participants