feat(redis): add TLS support via nginx stream sidecar - #2540

Actionable comments posted: 1

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@api/apps/v1alpha1/redis/types.go`:
- Around line 68-72: The Helm values schema currently allows any string for
tls.issuerRef.kind while the API type TLSIssuerRef (field Kind) enforces an enum
("Issuer" or "ClusterIssuer") and a default; update the Helm schema
(packages/apps/redis/values.schema.json) to add an enum constraint for
tls.issuerRef.kind with allowed values "Issuer" and "ClusterIssuer" and set the
same default ("ClusterIssuer") so Helm validation matches the TLSIssuerRef Kind
validation in the API.

---

Outside diff comments:
In `@api/apps/v1alpha1/redis/types.go`:
- Around line 1-10: The file api/apps/v1alpha1/redis/types.go is a generated
artifact that has been modified directly (header, imports/markers like resource
and v1), causing CI to overwrite it; revert any manual edits here and instead
make the required changes in the values-gen source used to produce this file,
then run the generation step (make generate) and commit the regenerated
artifacts so types.go (package redis and its imports) matches the canonical
generated output.

NOT LGTM — the nginx-sidecar TLS approach is sensible (Spotahome operator gap is real, sidecar avoids forking the operator) and the test coverage on both cozy-lib._tls.tpl and the redis chart is solid. But two combinations are broken in default paths: dashboard RBAC doesn't cover the new TLS-only services, so tenants cannot discover their master/replicas endpoints; and the auto-issued certificate carries only cluster-local SANs, so external clients fail TLS verification when external=true && tls.enabled=true.

Business context: Add client-facing TLS for Redis without forking the unmaintained Spotahome operator — an nginx stream sidecar terminates TLS on port 6380 and proxies to localhost:6379, where the operator probes/sentinel/replication continue plaintext.

Blockers

B1: dashboard RBAC missing TLS-only services

File: packages/apps/redis/templates/dashboard-resourcemap.yaml:10-15
Issue: When tls.enabled=true, the chart introduces new ClusterIP services {{ .Release.Name }}-master and {{ .Release.Name }}-replicas (service.yaml:6, 27) carrying the actual TLS endpoints. The dashboard Role only allows reading rfs-*, rfrm-*, rfrs-*, and *-external-lb. The new TLS services are not in resourceNames, so any dashboard/tenant view using this RBAC cannot fetch them.
Evidence: service.yaml:1-44 renders the two new services under if .Values.tls.enabled. dashboard-resourcemap.yaml:10-15 lists exactly the four legacy service names. With tls.enabled=true && external=false, those four services either don't exist or aren't where the TLS terminates, so the connection point is invisible from the tenant API.
Impact: Tenants enabling internal TLS see the secret in dashboard but no service to connect to. The whole non-external TLS path is unreachable from the dashboard.
Fix: Append {{ .Release.Name }}-master and {{ .Release.Name }}-replicas to the resourceNames list, gated on if .Values.tls.enabled (mirror the secrets block at lines 22-24).

B2: auto-issued external TLS certificate has only cluster-local SANs

File: packages/apps/redis/templates/tls-certificate.yaml:11-14
Issue: When tls.enabled=true && external=true, the chart exposes Redis through a LoadBalancer service and clients connect via the LB IP or provider hostname. The auto-generated Certificate only contains {{ .Release.Name }}-external-lb, {{ .Release.Name }}-external-lb.{{ .Release.Namespace }}, etc. — purely in-cluster names. TLS hostname verification fails for any external client unless they disable it or supply their own secret via tls.secretName.
Evidence: tls-certificate.yaml:1-22 builds $dnsNames only from cluster-internal patterns. service.yaml:50-79 creates the LB but the cert's SANs don't include any external-routable hostname. PR description claims external=true clients use redis-cli --tls --cacert ca.crt — but --cacert only fixes CA trust, not hostname mismatch.
Impact: The combination external=true && tls.enabled=true && tls.secretName="" (the documented default issuance path) is broken from the external connection perspective. Users who follow the README path get TLS errors and have to either disable verification or roll their own secret.
Fix: One of:
(a) Add a tls.dnsNames value letting the user inject the external hostname (or a list of hostnames) into the SAN list before rendering — minimum viable.
(b) Document explicitly that for external=true, users must provide tls.secretName with a cert containing the external hostname; fail-loud or warn if external=true && tls.enabled=true && tls.secretName=="".
(c) Discover the LB hostname/IP from the Service status and fold it into the SAN list — clean but requires post-provisioning regeneration.

Non-blocking follow-ups

1. api/apps/v1alpha1/redis/types.go:75 — TLSIssuerRef.kind has no +kubebuilder:validation:Enum=Issuer;ClusterIssuer tag, so a typo (ClusterIssue, cluseterIssuer, etc.) only fails late when cert-manager reconciles. The cozy-lib helper does template-time validation, but API-layer validation is expected for cozystack CRs and matches what other resources do.

2. packages/library/cozy-lib/templates/_tls.tpl:78 — - {{ . }} inside the dnsNames range is unquoted. Today's callers don't pass wildcard SANs, but *.example.com would be parsed as a YAML alias reference and break rendering. Quote it: - {{ . | quote }}. Trivial defense-in-depth for a helper meant to be reused (per PR description's reference to #473 phases).

3. packages/apps/redis/tests/tls-certificate_test.yaml:48 — the test fixture for a custom issuerRef.kind still uses ClusterIssuer (which is the default), so a regression where the chart hardcoded ClusterIssuer instead of consuming issuerRef.kind would not be caught. Switch the fixture to Issuer (and add the missing Issuer-kind resource if the test setup needs it) so the assertion is meaningful.

4. packages/system/redis-rd/cozyrds/redis.yaml openAPI schema marks tls.required: [enabled, issuerRef, secretName], with each field defaulted. Standard JSON Schema validators apply defaults before checking required, so this should work for partial submissions like tls: {enabled: true}. Worth confirming with cozystack-api's actual validation path that the dashboard's "toggle TLS" round-trip doesn't get rejected.

LGTM. Both blockers from the prior review are addressed:

• B1 (dashboard RBAC missing TLS services): 45e88cb84 adds {{ .Release.Name }}-master and {{ .Release.Name }}-replicas to dashboard-resourcemap.yaml:15-18 gated on .Values.tls.enabled, mirroring the secrets-section pattern at lines 26-28 (which already added the TLS secret to resourceNames). The non-external tls.enabled=true path is now reachable from the dashboard.
• B2 (auto-issued cert has only cluster-local SANs): 7dbfe8062 introduces the tls.extraDnsNames value (option (a) from the prior review — minimum-viable user-injected SAN list) and tls-certificate.yaml:20 passes it through the extraDnsNames parameter of cozy-lib.tls.certificate. Documented in values.yaml (@field {[]string} [extraDnsNames]) and the README parameter table. Users with external=true && tls.enabled=true now have a clean path to inject the LB hostname into the cert SANs without having to roll their own secret.

Non-blocking follow-ups from the prior review:

• Non-blocking #1 (kubebuilder enum on TLSIssuerRef.Kind): addressed in 6a0294ca9/7eb7296ef — IssuerKind type now carries +kubebuilder:validation:Enum="Issuer";"ClusterIssuer", so a typo fails at API admission rather than late at cert-manager reconcile time. CI's Verify generated code is up to date is green, so the deepcopy regeneration is in lockstep.
• Non-blocking #2 (cozy-lib helper unquoted - {{ . }} for dnsNames): addressed via the feat/cozy-lib-tls lineage that this branch merges; the dnsNames and extraDnsNames ranges now render as - {{ . | quote }}. The remaining unquoted issuerRef.{name,kind,group} scalars in this branch's copy of _tls.tpl are quoted on the canonical feat/cozy-lib-tls HEAD (#2485) and will land in main once #2485 merges; this PR is right to defer rather than carry conflicting copies.
• Non-blocking #3 (test fixture for custom issuerRef.kind reused the default): addressed in e0272dab7 — the Issuer-kind branch is now exercised by a non-default fixture, so a regression where the chart hardcoded ClusterIssuer would actually fail the test.

Verification:

• helm unittest packages/apps/redis → 30/30 pass locally across 5 suites (Dashboard RBAC, RedisFailover sidecar injection, Service architecture, TLS Certificate, TLS nginx ConfigMap).
• CI green including E2E and Verify generated code is up to date — the deepcopy regeneration on IssuerKind enum + extraDnsNames did not drift.
• Architectural decision worth flagging for future readers: the chart accepts the operator's plaintext-on-6379 contract as load-bearing (intra-cluster traffic from operator probes, sentinel, and replication stays plaintext on 6379; the nginx stream sidecar terminates TLS on 6380 and proxies to localhost:6379). This means TLS for cluster-internal connections is intentionally out of scope here and can only be addressed by either forking Spotahome or migrating to a different operator. Sentinel-aware clients still operate over plaintext, and the README is honest about that — clients that need automatic master failover with TLS are pointed at <release>-master:6380 (where the K8s Service selector follows the master role label and auto-routes after failover). Reasonable scope for an unmaintained-operator workaround.