feat(health): add switch mTLS support#3297
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds switch mTLS support for the health service. It introduces TLS config and validation, validates cert material at startup, threads TLS state through discovery, and updates NVUE, NMX-T, and NMX-C collectors to use switch-specific HTTPS/mTLS connection paths. ChangesmTLS support for switch collectors
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 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 (4)
crates/health/src/collectors/nmxt.rs (1)
727-753: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
value_scenarios!for this total-operation table.
nmxt_endpoint_urlis a total operation, and the companionnmxc.rstest in this same PR already expresses the identical scheme-switching table viavalue_scenarios!. Adopting the sharedcarbide-test-supporthelper here removes the hand-rolledTestCasestruct and keeps the two collectors consistent.♻️ Suggested table-driven rewrite
#[test] fn nmxt_endpoint_url_switches_scheme_when_tls_enabled() { use carbide_test_support::value_scenarios; value_scenarios!( run = |tls_enabled| nmxt_endpoint_url("10.0.0.9", tls_enabled); "endpoint URL scheme" { false => "http://10.0.0.9:9352/xcset/nvlink_domain_telemetry".to_string(), true => "https://10.0.0.9:9352/xcset/nvlink_domain_telemetry".to_string(), } ); }🤖 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/collectors/nmxt.rs` around lines 727 - 753, The `test_nmxt_endpoint_url_switches_scheme_when_tls_enabled` case should use the shared `carbide_test_support::value_scenarios!` helper instead of a custom `TestCase` table. Rewrite the test as a compact table-driven scenario around `nmxt_endpoint_url`, matching the existing `nmxc.rs` style in this PR, and remove the hand-rolled loop/struct so both collectors stay consistent.Source: Coding guidelines
crates/health/src/tls.rs (1)
391-397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
.ok()overlet _ = ...to discard the install result.Discarding the
Resultfrominstall_default()vialet _ = ...matches the exact anti-pattern flagged by the style guide.♻️ Proposed fix
- INSTALL_PROVIDER.call_once(|| { - let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); - }); + INSTALL_PROVIDER.call_once(|| { + rustls::crypto::aws_lc_rs::default_provider().install_default().ok(); + });As per coding guidelines, "Avoid using
let _unused = foo();to discard errors. Prefer.ok()to convert errors into Options, which are safely discardable."🤖 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/tls.rs` around lines 391 - 397, The ensure_rustls_provider function discards the result of rustls::crypto::aws_lc_rs::default_provider().install_default() using let _, which violates the style guideline. Update the INSTALL_PROVIDER.call_once closure in ensure_rustls_provider to call .ok() on install_default() instead, so the result is intentionally and idiomatically ignored without the anti-pattern.Source: Coding guidelines
crates/health/src/discovery/context.rs (1)
246-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
new_with_tls_config's explicit parameter looks redundant for its only production caller.
lib.rs::run_servicecomputeslet tls_config = config.tls.switch.clone();and passes it explicitly intonew_with_tls_config, but that value is always identical to what the.or_else(|| config.tls.switch.clone())fallback (line 297) would already produce from the sameArc<Config>. The extra parameter andpub(crate)constructor add a second API surface that isn't exercised by any test (the added test only calls the plainnew()). Consider droppingnew_with_tls_configand havingrun_servicecallDiscoveryLoopContext::new(limiter, metrics_manager, config.clone())directly, unless a future caller genuinely needs to inject a differenttls_configthanconfig.tls.switch.As per path instructions, "Abstractions should justify their existence: Do not add abstractions 'just in case'. Wait until there is a real requirement for them."
🤖 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/discovery/context.rs` around lines 246 - 258, The extra tls_config parameter and pub(crate) constructor on DiscoveryLoopContext::new_with_tls_config appear redundant because run_service can already derive the same value from the shared Config. Remove new_with_tls_config if there is no real alternate caller, and update run_service to construct DiscoveryLoopContext via DiscoveryLoopContext::new(limiter, metrics_manager, config.clone()) so the fallback logic stays in one place and the API surface is smaller.Source: Path instructions
crates/health/src/endpoint/model.rs (1)
76-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a plain
Stringreturn instead ofCow<'_, str>since both branches always allocate.Both match arms return
Cow::Owned(...); there is no borrowed path, so theCowindirection doesn't buy anything here — aStringreturn would be equally cheap and simpler for callers.Separately, the same "bracket IPv6, pass through IPv4" formatting logic is now duplicated in at least three places:
BmcAddr::to_url()(this file), this newswitch_connect_host_for_uri(), andRestClient::new()'s host derivation (crates/health/src/collectors/nvue/rest/client.rs). Extracting one shared helper (e.g.fn uri_host(ip: IpAddr) -> String) would remove the risk of the three copies drifting apart.🤖 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/endpoint/model.rs` around lines 76 - 81, The switch_connect_host_for_uri method always returns an owned value in both IpAddr::V4 and IpAddr::V6 branches, so replace the Cow<'_, str> return type with String and keep the same IPv4 pass-through / IPv6 bracket formatting. Also factor the duplicated URI host formatting logic shared with BmcAddr::to_url() and RestClient::new() into one helper (for example, a uri_host-style function) and update all callers to use it so the behavior stays consistent.
🤖 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/collectors/nmxt.rs`:
- Around line 727-753: The
`test_nmxt_endpoint_url_switches_scheme_when_tls_enabled` case should use the
shared `carbide_test_support::value_scenarios!` helper instead of a custom
`TestCase` table. Rewrite the test as a compact table-driven scenario around
`nmxt_endpoint_url`, matching the existing `nmxc.rs` style in this PR, and
remove the hand-rolled loop/struct so both collectors stay consistent.
In `@crates/health/src/discovery/context.rs`:
- Around line 246-258: The extra tls_config parameter and pub(crate) constructor
on DiscoveryLoopContext::new_with_tls_config appear redundant because
run_service can already derive the same value from the shared Config. Remove
new_with_tls_config if there is no real alternate caller, and update run_service
to construct DiscoveryLoopContext via DiscoveryLoopContext::new(limiter,
metrics_manager, config.clone()) so the fallback logic stays in one place and
the API surface is smaller.
In `@crates/health/src/endpoint/model.rs`:
- Around line 76-81: The switch_connect_host_for_uri method always returns an
owned value in both IpAddr::V4 and IpAddr::V6 branches, so replace the Cow<'_,
str> return type with String and keep the same IPv4 pass-through / IPv6 bracket
formatting. Also factor the duplicated URI host formatting logic shared with
BmcAddr::to_url() and RestClient::new() into one helper (for example, a
uri_host-style function) and update all callers to use it so the behavior stays
consistent.
In `@crates/health/src/tls.rs`:
- Around line 391-397: The ensure_rustls_provider function discards the result
of rustls::crypto::aws_lc_rs::default_provider().install_default() using let _,
which violates the style guideline. Update the INSTALL_PROVIDER.call_once
closure in ensure_rustls_provider to call .ok() on install_default() instead, so
the result is intentionally and idiomatically ignored without the anti-pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 74882ac4-4302-4a10-90e4-61acbe72eb2d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/health/Cargo.tomlcrates/health/example/config.example.tomlcrates/health/src/collectors/mod.rscrates/health/src/collectors/nmxc.rscrates/health/src/collectors/nmxt.rscrates/health/src/collectors/nvue/gnmi/client.rscrates/health/src/collectors/nvue/gnmi/subscriber.rscrates/health/src/collectors/nvue/rest/client.rscrates/health/src/collectors/nvue/rest/collector.rscrates/health/src/config.rscrates/health/src/discovery/context.rscrates/health/src/discovery/spawn.rscrates/health/src/endpoint/model.rscrates/health/src/lib.rscrates/health/src/tls.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
jayzhudev
force-pushed
the
health/switch-mtls-support
branch
from
0a35cb3 to
2acc945
Compare
Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
Reuse one mTLS HTTP client for the shared [tls.switch] profile instead of rebuilding reqwest clients per switch target. This keeps TLS reload bounded to the configured reload window and avoids per-target cert parsing/client construction at scale. Signed-off-by: Jay Zhu <jayzhu@nvidia.com>
jayzhudev
force-pushed
the
health/switch-mtls-support
branch
from
2acc945 to
746c2ac
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3297.docs.buildwithfern.com/infra-controller |
|
Do we expect single cert/key for all switches health connects to? |
Yes, as I understand that's the expectation. Support of "cert domains" for different groups of switches is intended, but today one site-wide cert group is used (and supported by RMS). When switches use different server cert groups, the client cert bundle can stay the same. If we later need different switch server CAs or TLS server names per switch group, we could add multiple TLS profiles and a mapping from switch groups to profiles. We don't have this as an immediate requirement yet, but will address as use cases evolve. |
2 participants
Add switch-specific mTLS support for switch collectors.
This introduces a new
[tls.switch]config profile for NVUE REST, NVUE gNMI, NMX-T, and NMX-C switch-host connections. It includes switch CA bundle, client certificate, client key, and optional TLS server name for DNS-SAN verification while connecting to discovered switch IPs.Added switch TLS material loading, validation, and HTTP/gRPC TLS construction. Cert material is validated at startup and when the HTTP client provider or gRPC channels are built, with clear errors for missing, unreadable, malformed, mismatched, not-yet-valid, or expired material.
Cert rotation behavior:
[tls.switch]profile.Note:
For periodic HTTP collectors,
tls_server_nameis used for SNI and certificate verification, not DNS resolution. Request URLs and HTTP Host headers stay on the discovered switch IP. This lets all switch targets share one HTTP client for the single[tls.switch]material set while still connecting to each discovered switch IP.Tradeoff:
This PR first considered rebuilding mTLS HTTP clients per switch target per poll iteration because it gives very direct reload semantics: read certs, validate certs, build client, scrape target. That is simple, but it is expensive at scale. At 20k switch targets (imaginary) and 30s poll intervals, that shape can cause about 20k cert read/parse/validate cycles and 20k HTTP client constructions per interval.
The final implementation uses
hyper+hyper-rustlsfor the mTLS HTTP path instead ofreqwest.reqwest::Clientis expensive to construct and is immutable after construction, and its resolver override is client-wide for a host name, which does not fit this routing model: all switches may share one TLS server name, while TCP must connect to many different switch IPs.The Hyper path keeps URL host / HTTP Host as the switch IP, applies
tls_server_nameonly to SNI and certificate verification, and shares one cached HTTP client provider for the[tls.switch]profile. Cert reload is attempted on the first HTTP collector use after the reload window expires, with the window based on the shortest enabled HTTP switch collector poll interval. This avoids per-target TLS/client construction in the steady state without tying client reuse to switch inventory changes.Related issues
Closes #3122
Type of Change
Breaking Changes
Testing
Manual testing was performed on a primary switch for both mTLS and non-mTLS modes.