feat(stack): surface NATS server-side TLS in self-managed stack

[sparve-nv]

Summary

Surface NATS server-side TLS in the self-managed stack so it can be enabled from environments/<env>.yaml (via nats.config.nats.tls + nats.config.merge.allow_non_tls), instead of only by misusing the secrets file or patching global.yaml.gotmpl. Both knobs default off, so existing installs render byte-identical.

Additional Details

The nats release's chart values are only ../global.yaml.gotmpl + ../secrets/<env>-secrets.yaml — the environment file is not in the release's values: list, so it reaches the release only where global.yaml.gotmpl re-emits it via dig … .Values. The nats: block wired reloader.image, podDisruptionBudget, and a storageClass-gated config: block, but had no passthrough for config.nats.tls / config.merge.allow_non_tls. Net effect: a nats.config.nats.tls value in environments/<env>.yaml was silently dropped and NATS started with no cert — which the INFO-first worker client experiences as a silent hang, not an error.

This matters for split-plane deployments (workers outside the control-plane cluster reaching NATS over a public, L4-passthrough load balancer): TLS must terminate at the NATS server, because an LB cannot — the INFO-first client deadlocks an LB-terminated handshake. allow_non_tls: true keeps in-cluster plaintext clients working on the same :4222.

Fix — add the passthrough as per-field dig into the single existing config: block (a second config: key under nats: would be silently collapsed to the last one, dropping the block):

• deploy/stacks/self-managed/global.yaml.gotmpl — surface config.nats.tls (emitted only with a value) and config.merge.allow_non_tls (emitted only when an explicit bool), folded into the existing config: block next to server_tags and the storageClass jetstream override.
• deploy/stacks/self-managed/environments/base.yaml — document the two knobs as commented examples (so they don't materialize).
• deploy/stacks/self-managed/tests/nats-tls-wiring.sh — new wiring test, registered in the Makefile.

Operators then set it in environments/<env>.yaml:

nats:
  config:
    nats:
      tls:
        enabled: true
        secretName: nats-server-tls   # cert-manager-issued, publicly trusted
    merge:
      allow_non_tls: true

For the Reviewer

• Default-off / backward-compatible: tls uses with and allow_non_tls uses kindIs "bool", so the stack never invents a default — an env file that sets neither renders byte-identical, and the chart's own default applies.
• Convention: this is the per-field dig pattern already used in this file (reloader.image, podDisruptionBudget, startupProbe, and the apiKeys/sis/reval route flags), rather than a whole-subtree merge — it keeps the override surface tight and stack-owned.
• Single-block requirement: everything stays in one config: block on purpose; a second config: (or merge:) key collapses silently, which is why allow_non_tls nests under the existing merge next to server_tags.
• Auth-callout preserved: the stack only emits merge.server_tags (+ optional allow_non_tls); Helm deep-merges the chart's config.merge.accounts/authorization underneath, unchanged.

For QA

Automated test (make test). Added deploy/stacks/self-managed/tests/nats-tls-wiring.sh, wired into the offline make test target next to nats-placement-tags.sh. Following the existing value-wiring pattern (pdb-value-wiring.sh), it renders the nats release values with helmfile write-values and asserts via yq:

• default (unset): no config.nats.tls, no config.merge.allow_non_tls, and server_tags preserved;
• override: config.nats.tls.{enabled,secretName} and config.merge.allow_non_tls: true thread through, and server_tags survives alongside allow_non_tls (guards the single-block collapse);
• explicit false: config.merge.allow_non_tls: false is honored (explicit bool, not just truthy).

Manual verification.

• Byte-identical default: rendered the nats release values on this branch vs origin/main with a default env — a raw diff is empty for both the no-storageClass and storageClass=gp3 cases, confirming no change when the knobs are unset.
• Wrapper-chart render: rendered the real helm-nvcf-nats chart (+ synadia nats subchart) nats.conf ConfigMap:
  • override → the client :4222 listener carries tls { cert_file, key_file } and allow_non_tls: true, with server_tags and the auth_callout accounts preserved;
  • default → no tls and no allow_non_tls; server_tags and auth_callout intact.
• Neighbors: nats-placement-tags.sh and pdb-value-wiring.sh still pass.

Issues

Fixes #1342

Checklist

• I am familiar with the Contributing Guidelines.
• I have signed off my commits for DCO compliance.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

Summary by CodeRabbit

• New Features

  • Added optional NATS TLS configuration for self-managed deployments.
  • Added support for explicitly allowing or disabling non-TLS client connections.
  • Added guidance for configuring cert-manager-issued NATS server certificates.

• Bug Fixes

  • Ensured NATS TLS settings and existing server tags are correctly preserved during configuration rendering.

• Tests

  • Added integration coverage for default, enabled, and disabled non-TLS behavior and TLS value propagation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0ca1509a-039a-4651-b573-bcab6ca819a4

📥 Commits

Reviewing files that changed from the base of the PR and between ed1fefc and 85482dc.

📒 Files selected for processing (4)

• deploy/stacks/self-managed/Makefile
• deploy/stacks/self-managed/environments/base.yaml
• deploy/stacks/self-managed/global.yaml.gotmpl
• deploy/stacks/self-managed/tests/nats-tls-wiring.sh

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

📝 Walkthrough

Walkthrough

The self-managed stack now passes optional NATS server TLS and allow_non_tls settings from environment values into the merged chart configuration. A Bash integration test validates default, enabled, and explicitly disabled settings.

Changes

NATS TLS wiring

Layer / File(s)	Summary
NATS configuration passthrough deploy/stacks/self-managed/environments/base.yaml, deploy/stacks/self-managed/global.yaml.gotmpl	The environment guidance documents split-plane NATS TLS settings. The template conditionally emits TLS and boolean allow_non_tls values while preserving existing NATS configuration.
NATS wiring integration checks deploy/stacks/self-managed/tests/nats-tls-wiring.sh, deploy/stacks/self-managed/Makefile	The integration test renders default, enabled, and explicitly disabled configurations. The default test target runs the new script.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🔵 Low · up to 85482

The change is mergeable with explicit owner awareness: existing installations remain unchanged, but enabling the documented mixed TLS/plaintext mode on a public NATS listener allows unencrypted traffic and should be restricted to environments where that exposure is acceptable.

Sequence Diagram(s)

sequenceDiagram
  participant EnvironmentValues
  participant GlobalTemplate
  participant Helmfile
  participant NATSChart
  EnvironmentValues->>GlobalTemplate: provide optional TLS and allow_non_tls values
  GlobalTemplate->>Helmfile: render merged NATS configuration
  Helmfile->>NATSChart: pass rendered chart values

Loading

Suggested reviewers: athappa-nv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (3 skipped: 3 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title uses the required Conventional Commits format, "feat(stack):", and accurately describes the primary change: exposing NATS server-side TLS in the self-managed stack.
Linked Issues check	✅ Passed	The changes satisfy issue #1342. They pass through nats.config.nats.tls and nats.config.merge.allow_non_tls only when configured, preserve the existing merged config block, retain server tags an…
Out of Scope Changes check	✅ Passed	All changes directly support issue #1342. The template wiring, environment documentation, integration test, and Makefile registration are within the stated scope.

Full details: Linked Issues check

Explanation

The changes satisfy issue #1342. They pass through nats.config.nats.tls and nats.config.merge.allow_non_tls only when configured, preserve the existing merged config block, retain server tags and related settings, document the options, and add wiring tests for default, enabled, and disabled behavior.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (3 skipped: 3 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}