feat(health): add TLS and mTLS support for the OTLP sink#3478
Conversation
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Summary by CodeRabbit
WalkthroughChangesOTLP targets now support optional TLS/mTLS profiles with validation, CA and client identity loading, platform roots for implicit HTTPS, TLS server-name overrides, startup preflight, and periodic client replacement in logs and metrics drains. OTLP TLS and mTLS
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant run_service
participant otlp_preflight
participant OtlpDrainTask
participant connect_replacement_target
participant tonicChannel
run_service->>otlp_preflight: validate each configured TLS profile
OtlpDrainTask->>connect_replacement_target: refresh target client on reload tick
connect_replacement_target->>tonicChannel: connect with timeout
tonicChannel-->>connect_replacement_target: replacement channel or error
connect_replacement_target-->>OtlpDrainTask: client refresh result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/health/src/config.rs (1)
440-529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
tls_server_namevalidation logic withMtlsProfileConfig::validate.
OtlpTlsConfig::validate's trim/empty/whitespace/DnsName::try_fromblock (lines 508-521) is byte-for-byte identical toMtlsProfileConfig::validate's equivalent block (crates/health/src/config.rs:767-799, shown in graph context). Extracting a shared helper (e.g.fn validate_tls_server_name(path: &str, name: &str) -> Result<(), String>) would remove this duplication and keep the two error-message families from drifting apart over time.♻️ Sketch of the shared helper
+fn validate_tls_server_name(path: &str, tls_server_name: &str) -> Result<(), String> { + if tls_server_name.trim().is_empty() { + return Err(format!("{path}.tls_server_name must not be empty")); + } + + if tls_server_name.trim() != tls_server_name { + return Err(format!( + "{path}.tls_server_name must not contain leading or trailing whitespace" + )); + } + + DnsName::try_from(tls_server_name) + .map(|_| ()) + .map_err(|_| format!("{path}.tls_server_name must be a valid DNS name")) +}Then both
OtlpTlsConfig::validateandMtlsProfileConfig::validatecallvalidate_tls_server_name(&path, tls_server_name)?instead of repeating the block.🤖 Prompt for 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. In `@crates/health/src/config.rs` around lines 440 - 529, Extract the duplicated TLS server-name checks into a shared validate_tls_server_name helper that accepts the validation path and name, preserving the existing empty, whitespace, and DnsName error messages. Update both OtlpTlsConfig::validate and MtlsProfileConfig::validate to call the helper and remove their repeated validation blocks.crates/health/src/otlp/drain.rs (2)
54-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated TLS-reload interval construction into a shared helper.
The 14-line block computing
tls_reload_period(falling back toOtlpTlsConfig::DEFAULT_RELOAD_INTERVAL) and buildingtls_reload_intervalwithMissedTickBehavior::Delayis copied verbatim between the logs and metrics drains.otlp/mod.rsalready hosts shared helpers (target_endpoint,connect_replacement_target) for this cohort, making it a natural home for this logic too.
crates/health/src/otlp/drain.rs#L54-L77: replace this block with a call to a newpub(crate) fn tls_reload_interval(target: &OtlpTargetConfig) -> tokio::time::Intervalinotlp/mod.rs.crates/health/src/otlp/metrics_drain.rs#L63-L86: replace the identical block here with the same shared helper call.🤖 Prompt for 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. In `@crates/health/src/otlp/drain.rs` around lines 54 - 77, Extract the duplicated TLS reload interval construction into a new pub(crate) helper named tls_reload_interval in otlp/mod.rs, preserving the default reload period, delayed initial tick, and MissedTickBehavior::Delay. In crates/health/src/otlp/drain.rs lines 54-77 and crates/health/src/otlp/metrics_drain.rs lines 63-86, remove the local construction and call this helper with the OtlpTargetConfig.
94-113: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEmit a
carbide_instrument::Eventfor TLS reload outcomes in both drains.Both drains only log reload success/failure via
tracing::debug!/tracing::warn!; there is no counter to alert on repeated reload failures, so a rotting or unrotated certificate can go unnoticed until the channel eventually fails outright. Both signal drains already emit a structured event (OtlpExportFailed) for export failures — reload outcomes deserve the same treatment for a count/rate.
crates/health/src/otlp/drain.rs#L94-L113: define and emit acarbide_instrument::Event(e.g.OtlpTlsReloadFailed/OtlpTlsReloaded) withsignal = OtlpSignal::Logsalongside the existingtracing::warn!/tracing::debug!calls.crates/health/src/otlp/metrics_drain.rs#L103-L121: emit the same event withsignal = OtlpSignal::Metricsalongside itstracing::warn!/tracing::debug!calls.🤖 Prompt for 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. In `@crates/health/src/otlp/drain.rs` around lines 94 - 113, Define TLS reload outcome events alongside the existing OtlpExportFailed event, then emit success and failure events in the reload branches of crates/health/src/otlp/drain.rs lines 94-113 with signal = OtlpSignal::Logs, preserving the existing tracing logs. Apply the same event definitions and emissions in crates/health/src/otlp/metrics_drain.rs lines 103-121 with signal = OtlpSignal::Metrics, covering both successful and failed reloads.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/health/src/config.rs`:
- Around line 440-529: Extract the duplicated TLS server-name checks into a
shared validate_tls_server_name helper that accepts the validation path and
name, preserving the existing empty, whitespace, and DnsName error messages.
Update both OtlpTlsConfig::validate and MtlsProfileConfig::validate to call the
helper and remove their repeated validation blocks.
In `@crates/health/src/otlp/drain.rs`:
- Around line 54-77: Extract the duplicated TLS reload interval construction
into a new pub(crate) helper named tls_reload_interval in otlp/mod.rs,
preserving the default reload period, delayed initial tick, and
MissedTickBehavior::Delay. In crates/health/src/otlp/drain.rs lines 54-77 and
crates/health/src/otlp/metrics_drain.rs lines 63-86, remove the local
construction and call this helper with the OtlpTargetConfig.
- Around line 94-113: Define TLS reload outcome events alongside the existing
OtlpExportFailed event, then emit success and failure events in the reload
branches of crates/health/src/otlp/drain.rs lines 94-113 with signal =
OtlpSignal::Logs, preserving the existing tracing logs. Apply the same event
definitions and emissions in crates/health/src/otlp/metrics_drain.rs lines
103-121 with signal = OtlpSignal::Metrics, covering both successful and failed
reloads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7ceeaa63-cce0-4d38-82b3-1d22776df657
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/health/Cargo.tomlcrates/health/example/config.example.tomlcrates/health/src/config.rscrates/health/src/lib.rscrates/health/src/otlp/drain.rscrates/health/src/otlp/metrics_drain.rscrates/health/src/otlp/mod.rscrates/health/src/sink/otlp.rscrates/health/src/tls.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
Why
The OTLP sink may need to send logs and metrics directly to secured targets without relying on TLS termination elsewhere.
What's new
Each target can now configure:
HTTPS targets without a TLS table use the host or container's installed CA certificates for server verification. Plaintext HTTP targets remain supported.
Behavior
Certificate material is validated during startup and periodically reloaded. Replacement channels are adopted only after a successful connection. If rotated material is invalid or the replacement connection fails, the existing channel remains active.
Related issues
Closes #3470
Type of Change
Breaking Changes
The
tlstable is optional.Testing
Validated:
Additional Notes
Example mTLS configuration:
For server-authenticated TLS without client authentication, omit
client_cert_pathandclient_key_path.No Helm changes are included because the current chart does not template OTLP sink settings or certificate mounts.