feat(runtime): add TLS support to NATS transport - #13096
Conversation
WalkthroughThis change adds NATS TLS runtime options, environment constants, connection handling, validation, tests, and documentation. It supports custom CA certificates, insecure verification mode, and ChangesNATS TLS support
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
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 `@docs/fern/pages/reference/components/tls-configuration.mdx`:
- Around line 147-177: Update the NATS documentation near the NATS TLS
configuration and the Encrypted paths table to explicitly include inference
request traffic when NATS is used as the request plane. Expand the NATS
description and its table row while retaining the existing JetStream recovery
and audit-log coverage.
In `@lib/runtime/src/transports/nats.rs`:
- Around line 294-301: The NATS configuration validation only checks the server
URL scheme and allows TLS options with nats://. Update validate_nats_server and
the pre-connection validation flow to reject nats:// when either
tls_ca_cert_path or tls_insecure is configured, returning a startup error; add
tests covering both invalid combinations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44d54a53-4745-42a9-a9f9-dd90210c2285
📒 Files selected for processing (4)
components/src/dynamo/common/configuration/groups/runtime_args.pydocs/fern/pages/reference/components/tls-configuration.mdxlib/runtime/src/config/environment_names.rslib/runtime/src/transports/nats.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 392544b33b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
🎯 Code Coverage (details) 🔗 Commit SHA: 33549bd | Docs | View more details | Give us feedback! |
392544b to
a9ac40c
Compare
a9ac40c to
ea71d2b
Compare
ea71d2b to
642d246
Compare
|
Update: addressed all automated-review feedback (Devin, CodeRabbit, Codex, dynamo-review-agent). Latest revision folds these into the existing commits; per-thread replies have specifics. Correctness / behavior
Docs
Verification: |
642d246 to
01e0b28
Compare
01e0b28 to
9fff8bf
Compare
Enable opt-in TLS for the NATS client connection. A custom rustls TLS config is applied when NATS_TLS_CA_CERT_PATH is set or NATS_TLS_INSECURE is truthy. When only a tls:// URL is used without explicit TLS env vars, async-nats handles TLS natively with system roots. New env vars: NATS_TLS_CA_CERT_PATH, NATS_TLS_INSECURE. The NATS server URL validator now accepts tls:// (case-insensitive) in addition to nats://. No process-wide crypto provider is installed to avoid racing with the HTTP path's aws_lc_rs provider. Signed-off-by: Walid <walid.elbouchikhi@datadoghq.com>
NATS TLS support means the page now covers more than TCP transports, so retitle to TLS and update the subtitle to reflect TCP + NATS. Signed-off-by: Walid <walid.elbouchikhi@datadoghq.com>
On this branch NATS TLS exists, so the earlier "KV event plane is not encrypted" wording contradicted the NATS TLS section. Clarify: TCP request/response streams are encrypted via DYN_TCP_TLS_*, NATS traffic via NATS TLS; the KV event plane is encrypted only when it runs over NATS with NATS TLS configured — over ZMQ it is not. Also soften the "Encrypted paths" caption (ZMQ event plane not covered). Signed-off-by: Walid <walid.elbouchikhi@datadoghq.com>
9fff8bf to
33549bd
Compare
|
/ok to test 33549bd |
What
Adds opt-in TLS for Dynamo's NATS transport (JetStream indexer recovery/replay + audit sink), continuing the TLS stack after the TCP response streams (#10921) and TCP request plane (#12533).
NATS_TLS_CA_CERT_PATH/NATS_TLS_INSECUREenv vars, plus--nats-tls-ca-cert-path/--nats-tls-insecureCLI flags on all backends viaDynamoRuntimeArgGroup.NATS_SERVERnow accepts thetls://scheme in addition tonats://. Atls://URL without explicit TLS env vars letsasync-natshandle TLS natively with system roots; aNATS_TLS_CA_CERT_PATH(or insecure mode) applies a custom rustls client config and requires TLS.ringcrypto provider as the process default soasync-natsdoesn't panic onCryptoProviderlookup when it sees atls://URL (bothringandaws-lc-rsare compiled in transitively, so rustls 0.23 can't auto-detect).TCP TLS→TLS, intro now covers all transports), adds a NATS TLS section, and clarifies the KV/ZMQ event-plane boundary.Notes on cert handling
Dynamo is a NATS client here: it verifies the NATS server against
NATS_TLS_CA_CERT_PATHand presents no client identity (with_no_client_auth). So the server-cert hot-reload from #12533 doesn't apply to this path — there's no served leaf cert to rotate. The CA trust anchor is loaded once (rotating the CA requires a restart, same as the TCP client). Client-identity certs (and their reload) arrive with the upcoming NATS/TCP mTLS PR.Testing
cargo test -p dynamo-runtime --lib nats— 8 pass (2 ignored; they require a live NATS server).cargo fmt --checkandcargo build -p dynamo-runtimeclean.Part of #10809.
Summary by CodeRabbit
New Features
tls://URLs.Documentation