feat(nats): add TLS support via cert-manager - #2684

Actionable comments posted: 1

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/apps/nats/Makefile`:
- Around line 7-8: Before performing the jq patch that sets
.properties.tls.properties.enabled.type in values.schema.json, first run a
precondition check that the path .properties.tls.properties.enabled exists (fail
with an error and exit if it does not) so the patch cannot create missing paths
and mask generator drift; after the precondition succeeds, apply the existing jq
patch to set the type and keep the existing post-check that validates the type
equals ["boolean","null"]. Use the same values.schema.json target and the same
jq-based validation/exit pattern so the new check sits immediately before the jq
patch line.

Code Review

This pull request introduces TLS support for the NATS application, implementing a tri-state logic for the tls.enabled flag that defaults to the external access setting if omitted. The implementation includes new cert-manager templates for automated certificate generation, updates to the NATS HelmRelease configuration, and extensive unit tests. Review feedback recommends using the dig function in Helm templates when accessing nested values like _namespace and _cluster to ensure defensive handling of potentially missing keys.

NOT LGTM — clean cert chain with correct usages, the upstream chart's config-reloader sidecar handles cert rotation cleanly via SIGHUP (verified through nats.reloaderConfig extracting cert/key/CA paths), and the dashboard RBAC only exposes <release>-credentials (no TLS-secret leak like #2680). One blocker: the cluster-route URLs don't match the SAN list, and replicas: 2 is the default for this chart so this hits on first install.

Business context: Add TLS for NATS managed service — client connections on :4222 and inter-pod cluster routes on :6222 — via a self-contained cert-manager chain, with tls.enabled tri-state falling back to external when unset.

Blockers

B1: Cluster TLS handshake fails because route URLs use short-form pod hostnames not in the SAN list

File: packages/apps/nats/templates/nats.yaml:93-102 + packages/apps/nats/templates/certmanager.yaml:60-64

Issue: The upstream NATS chart at packages/system/nats/charts/nats/files/config/cluster.yaml:12-17 constructs each cluster route URL as:

{{ proto }}://{{ release }}-{{ i }}.{{ release }}-headless:{{ port }}

…unless routeURLs.useFQDN: true, in which case it appends .{{ ns }}.svc.{{ clusterDomain }}. The upstream default is useFQDN: false (verified at packages/system/nats/charts/nats/values.yaml:60), and this PR's HelmRelease values don't override it — only routeURLs.k8sClusterDomain is set, which has no effect when useFQDN is off.

The cert SAN list (certmanager.yaml:60-64) covers <release>-headless{,.<ns>.svc{,.<cluster>}} plus the wildcard *.<release>-headless.<ns>.svc.<cluster>. X.509 wildcards match exactly one DNS label at the leftmost position — so the wildcard matches <release>-0.<release>-headless.<ns>.svc.<cluster> (FQDN form) but does NOT match <release>-0.<release>-headless (short form). The short form has fewer labels than the wildcard pattern.

Evidence: packages/system/nats/charts/nats/files/config/cluster.yaml:12-17 — the $domain variable stays at $.Values.headlessService.name (i.e. <release>-headless) when useFQDN is false; the route URL template printf "%s://%s%s-%d.%s:%d" then yields tls://<release>-N.<release>-headless:6222. NATS Go client TLS verification (used internally by the server for cluster route dialing) compares the SAN list against the literal URL hostname, not the post-resolution FQDN. No insecure option is set in the cluster TLS block.

Impact: Default install (this chart ships replicas: 2, see values.yaml:59) + TLS auto-on (whenever external: true is set, which also triggers TLS via the tri-state fallback) → cluster won't form. Pods log x509: certificate is valid for <list>, not <release>-0.<release>-headless and reroute attempts loop. Single-replica deployments happen to work because no peer route is dialed.

Fix: Force FQDN route URLs in the HelmRelease values whenever TLS is enabled:

cluster:
  enabled: true
  routeURLs:
    useFQDN: true
    k8sClusterDomain: {{ $clusterDomain }}
  replicas: {{ .Values.replicas }}

This swaps the route URL to <release>-N.<release>-headless.<ns>.svc.<cluster> which the existing wildcard SAN covers. No SAN changes needed.

Non-blocking follow-ups

1. certmanager.yaml:25-26: CA privateKey only sets rotationPolicy: Never, leaving algorithm/size at cert-manager defaults (RSA-2048). The leaf certificate has no privateKey block at all. Non-uniform across the sibling TLS PRs (mariadb uses RSA-4096 explicitly); RSA-2048 is still sound for a tenant-scoped CA but worth a brief look for a project-level convention.

2. _tls.tpl:11-17 — kindIs "invalid" correctly handles tls.enabled == nil (the Makefile patches the schema to ["boolean","null"]). One edge case: if a future codegen drift unknowingly changes the schema to ["boolean"] only, the Makefile's precondition check (commit 68264383) will catch the schema rebuild but not a manually-passed null — though Helm itself will reject null against "type":"boolean", so the failure mode is at install time, not template time. Acceptable; the Makefile precondition is a good safeguard.

3. nats.yaml:62-66: tlsCA.enabled: true with secretName: <release>-ca mounts the CA secret cluster-wide for verification. cert-manager's CA secret (<release>-ca) is a kubernetes.io/tls secret that also contains tls.key (the CA private key). The NATS upstream chart's tlsCAVolume mounts the whole secret. Tenant pods don't have direct read access to that secret (no separate RBAC change exposes it), but it's worth noting that NATS pods themselves can read the CA private key from the mounted volume. Strictly speaking the pod doesn't need the CA private key — only ca.crt — but the chart mounts everything from the Secret. Not a blocker (this is on the NATS pod, not on tenant users), but a defense-in-depth improvement would be a separate CA-cert-only ConfigMap.

LGTM. Both findings from my prior review on this branch are addressed in this push, plus the schema-array regression and the missing digital signature are folded in.

• a62225796 fix(nats): force cluster route useFQDN when TLS enabled to match cert SAN wildcard — closes the SAN-vs-route-URL mismatch I flagged. The nats.useFQDN: true override under $tlsEnabled aligns the route URL (<release>-N.<release>-headless.<ns>.svc.<cluster-domain>) with the cert's wildcard *.<release>-headless.<ns>.svc.<cluster-domain> SAN.
• b00b17448 refactor(nats): switch CA and leaf private keys to ECDSA P-256 + leaf usages [digital signature, key encipherment, server auth, client auth] — digital signature was present already, but the explicit ECDSA + the keep of the same usages survives the rewrite.
• 682643831 fix(nats): add precondition check before schema patch in Makefile + cd7b06797 chore(nats): regenerate codegen artifacts + 1f830806d refactor(nats): drop null from tls.enabled schema, use kindIs invalid pattern — same schema-shape fix as the rest of the batch (apiextv1.JSONSchemaProps can't unmarshal ["boolean","null"]).

dashboard RBAC exposes only <release>-credentials — the TLS Secret isn't granted to tenant use, so the B3 pattern that's blocking the chart-side cert-manager peer PRs doesn't apply here. NATS config-reloader sidecar handles SIGHUP-driven cert rotation in the chart already, so no Reloader annotation needed.

LGTM — TLS via cert-manager is correctly wired for both client (4222) and cluster (6222) traffic, the cluster-route SAN mismatch from the earlier review is fixed and test-covered, generated artifacts are in sync, and all 51 helm-unittest cases pass.

Business context: adds TLS to the managed NATS app via a self-contained cert-manager chain (SelfSigned issuer → CA → leaf), with a tri-state tls.enabled that falls back to external when unset.

Non-blocking follow-ups

1. The CA uses ECDSA P-256 with rotationPolicy: Never and the leaf ECDSA P-256 with rotationPolicy: Always + renewBefore: 720h — consistent and sound. Worth confirming the sibling TLS PRs converge on the same algorithm/curve for cross-app uniformity (packages/apps/nats/templates/certmanager.yaml:25-28,51-54).
2. tlsCA mounts the whole <release>-ca secret (including the CA private key) into NATS pods, though only ca.crt is needed for trust. Not a tenant-facing leak (no RBAC surfaces it; the dashboard exposes only <release>-credentials) and the pod is the trust anchor anyway — a CA-cert-only ConfigMap would be defence-in-depth, not a requirement (packages/apps/nats/templates/nats.yaml:62-66).
3. Tri-state default-on means an existing install with external: true and no tls set flips TLS on upon upgrade, breaking plaintext external clients. This matches the secure-by-default stance and is documented — worth calling out as intended breaking behaviour in the release note.