Repository navigation
Conversation
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>
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
reyortiz3
left a comment
There was a problem hiding this comment.
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:
-
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 eachpopulateScalingConfig*re-parses the env. Could the default be read and validated once at startup and passed in? -
The fallback policy is encoded twice.
SessionRedisTLSVolumesWithDefaultand thepopulateScalingConfig*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.
|
Also, adding some of this to stacklok/toolhive-core#386 so we can re-use it later |
Summary
The runtime and per-workload APIs support Redis TLS after #6731 and #6767, but workloads using the operator-wide
defaultRedisfallback still cannot enable it. This prevents that fallback from connecting to TLS-only Redis endpoints.Closes #6827
Type of change
Test plan
controllerutil,vmcpconfig,controllers, andapp.task helm-unittest: 85 operator tests and 3 CRD-chart tests passed.ct lint --config ct.yamlthrough Task with explicit paths to the pinned tool's schema and YAML lint configuration: both charts and their CI values files passed.task helm-docsandtask license-check.Live verification used published v0.54.0 workload runtimes with the modified operator:
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
v1beta1API.No CRD fields, schemas, or runtime TLS types change. Global TLS defaults to disabled; existing address/password precedence remains unchanged.
Changes
controllerutil/defaultredis.go,app/app.gocontrollerutil/session_redis_tls.goDoes this introduce a user-facing change?
Yes. Enable verified TLS for workloads using the global fallback:
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, andTOOLHIVE_DEFAULT_REDIS_TLS_CA_SECRET_KEY. Global TLS always verifies certificates; it has noinsecureSkipVerifyoption.Implementation plan
Implementation plan
Special notes for reviewers
provider: memory. Existing password selection is unchanged. CA projection follows each runtime selection policy.subPath, and the runtime loads trust at startup.ReadDefaultRedisConfigretains its signature and panics on invalid TLS rather than downgrading. Normal operator startup validates first, reports the error, and exits before starting controllers.