feat(health): add ClusterEndpointSource for BCM/file fleet inventory#3359
Open
cdraman wants to merge 1 commit into
Open
feat(health): add ClusterEndpointSource for BCM/file fleet inventory#3359cdraman wants to merge 1 commit into
cdraman wants to merge 1 commit into
Conversation
Adds a new EndpointSource that ingests cluster rack inventory from either
a local JSON file or Cluster manager's JSON RPC API.
File path: reads a JSON inventory file containing
hostname, BMC IP, rack, and default credentials for each node.
Cluster Manager's JSON RPC path (placeholder, pending head-node IP confirmation):
- POST /json/ cmpart.getPartition("<partition>") → cluster BMC
username + password
- POST /json/ cmdevice.getDevices → per-node hostname + BMC IP
Both paths produce the same Vec<ClusterNode> fed into build_endpoints().
MAC addresses are derived deterministically from BMC IPv4 octets
(02:00:<o1>:<o2>:<o3>:<o4>) as internal cache keys only.
config: cluster source is Disabled by default; enable with:
[endpoint_sources.cluster]
inventory_path = "/path/to/inventory.json"
# or
cluster_manager_url = "https://<head-node>:8081"
Signed-off-by: Dasa Chandramouli <dchandramoul@nvidia.com>
Contributor
Summary by CodeRabbit
WalkthroughAdds a configurable ChangesCluster endpoint discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HealthService
participant ClusterEndpointSource
participant ClusterManager
participant BmcClient
HealthService->>ClusterEndpointSource: from_config
ClusterEndpointSource->>ClusterManager: Request credentials and devices
ClusterManager-->>ClusterEndpointSource: Return inventory data
ClusterEndpointSource->>BmcClient: Build BMC clients
ClusterEndpointSource-->>HealthService: Return endpoint source
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
crates/health/src/endpoint/cluster.rs (1)
1-414: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for the new parsing/extraction logic.
This entire new file —
extract_manager_devices,fetch_manager_credentials,read_from_file, andbuild_endpoints— ships without tests, despite being exactly the kind of parser/normalization logic the repo's testing conventions target.sources.rsandmodel.rs(in the same crate) both carry table-driven#[test]coverage for comparable logic.Consider adding
value_scenarios!/check_valuestables forextract_manager_devices(varying response shapes: top-level array,result/data/items/deviceswrappers, missing hostname/IP, invalid IP) and forread_from_file(valid inventory, malformed JSON, missing file). As per path instructions, "prefer table-driven scenarios ... especially for parsers, validators, and conversions."🤖 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/cluster.rs` around lines 1 - 414, Add table-driven tests following the crate’s existing value_scenarios!/check_values conventions for extract_manager_devices, covering top-level and result/data/items/devices wrappers, missing fields, and invalid IPs; add read_from_file cases for valid inventory, malformed JSON, and missing files. Use temporary inventory paths where needed and assert parsed nodes or expected errors, without changing production behavior.
🤖 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 `@crates/health/src/config.rs`:
- Around line 102-134: ClusterEndpointSourceConfig’s derived Debug
implementation exposes default_password in logs. Replace the derived Debug with
a manual implementation that redacts or omits default_password, following
StaticBmcEndpoint’s custom Debug pattern, while preserving the existing
serialization behavior unless separately required.
- Around line 104-134: Add ClusterEndpointSourceConfig::validate() to require
either a non-empty inventory_path or a configured cluster_manager_url, returning
a clear configuration error when both are absent; then invoke this validation
from Config::validate() whenever the cluster endpoint source is enabled.
In `@crates/health/src/endpoint/cluster.rs`:
- Around line 236-242: Preserve the absence of a rack identifier in the
cluster-manager parsing and endpoint construction. Update the logic producing
`rack` near the category/partition lookup to return an optional value rather
than defaulting to an empty string, and update `build_endpoints` to pass `None`
when no non-empty rack exists instead of unconditionally constructing
`Some(RackId::new(&node.rack))`; retain `Some(RackId)` only for a valid rack so
`BmcEndpoint::hash_key` falls back to the endpoint MAC.
- Around line 128-189: Remove the plaintext response logging from
fetch_manager_credentials by deleting the tracing::debug! call that emits raw.
If response diagnostics are required, create a redacted representation that
removes or masks bmcSettings.password and any equivalent credential fields
before logging it, while preserving structured logfmt fields.
- Around line 274-295: Refresh the cluster inventory during every discovery pass
instead of caching it only in ClusterEndpointSource::from_config. Update
ClusterEndpointSource and fetch_bmc_hosts to re-read via fetch_from_manager or
read_from_file, rebuild endpoints with the configured port, clients, proxy, and
cache size, and return the refreshed set so additions, removals, and IP changes
are observed without restarting.
- Around line 191-270: Make Cluster Manager parsing fail closed: update
fetch_manager_credentials to reject missing or unrecognized credential fields
instead of silently falling back, and update extract_manager_devices to return
or propagate a countable error when a non-empty getDevices response has no
recognized shape or produces zero valid nodes. Preserve explicit handling for
valid empty arrays, and ensure callers surface the parsing failure rather than
treating it as a successful empty inventory.
- Around line 88-93: The build_http function unconditionally disables TLS
certificate validation for the cluster-manager credential client. Remove
danger_accept_invalid_certs(true) and require normal trusted certificate-chain
validation, or gate the bypass behind an explicit opt-in configuration flag;
also avoid logging raw partition responses in the credential-fetching path.
In `@crates/health/src/lib.rs`:
- Around line 153-162: Make cluster endpoint initialization best-effort in
ClusterEndpointSource::from_config: handle device-inventory and URL/RPC failures
locally instead of propagating them through build_endpoint_wiring, log the
failure, and skip the cluster source (or apply the existing retry/backoff
mechanism) so other sources can start normally.
---
Nitpick comments:
In `@crates/health/src/endpoint/cluster.rs`:
- Around line 1-414: Add table-driven tests following the crate’s existing
value_scenarios!/check_values conventions for extract_manager_devices, covering
top-level and result/data/items/devices wrappers, missing fields, and invalid
IPs; add read_from_file cases for valid inventory, malformed JSON, and missing
files. Use temporary inventory paths where needed and assert parsed nodes or
expected errors, without changing production behavior.
🪄 Autofix (Beta)
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: 8600f962-7b95-4e00-bec6-6a463bc9bd9e
📒 Files selected for processing (4)
crates/health/src/config.rscrates/health/src/endpoint/cluster.rscrates/health/src/endpoint/mod.rscrates/health/src/lib.rs
Comment on lines
+102
to
+134
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct ClusterEndpointSourceConfig { | ||
| /// Path to a JSON inventory file containing BMC endpoints and credentials for the cluster. | ||
| /// Used when `cluster_manager_url` is absent. | ||
| #[serde(default)] | ||
| pub inventory_path: PathBuf, | ||
|
|
||
| /// Cluster manager head-node URL (e.g. https://10.x.x.x:8081). | ||
| /// When set, inventory and credentials are fetched live via cluster manager JSON RPC | ||
| /// instead of reading `inventory_path`. | ||
| #[serde(default)] | ||
| pub cluster_manager_url: Option<url::Url>, | ||
|
|
||
| /// Cluster manager partition to read bmcsettings from (default: "base"). | ||
| /// The cluster manager stores BMC username/password at partition level; this selects which partition. | ||
| #[serde(default = "default_cluster_manager_partition")] | ||
| pub cluster_manager_partition: String, | ||
|
|
||
| /// Fallback BMC username if cluster manager JSON RPC does not return one. | ||
| /// Cluster manager default is "bright" (set during head-node installation). | ||
| #[serde(default = "default_cluster_manager_username")] | ||
| pub default_username: String, | ||
|
|
||
| /// Fallback BMC password if cluster manager JSON RPC does not return one. | ||
| /// Must be set explicitly — no code-level default. | ||
| #[serde(default)] | ||
| pub default_password: Option<String>, | ||
|
|
||
| /// Optional BMC port override. None uses the BmcClient default (443/HTTPS). | ||
| #[serde(default)] | ||
| pub port: Option<u16>, | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Secret leakage via derived Debug/Serialize.
ClusterEndpointSourceConfig derives Debug and Serialize, and default_password: Option<String> is a plain field. Anywhere this config is logged with ?config or tracing::debug!(?cfg, ...) (or serialized for diagnostics), the password will be printed verbatim. Compare with StaticBmcEndpoint, which has a hand-written Debug impl (crates/health/src/config.rs:233-243) that deliberately omits username/password. This new struct breaks that established convention.
🔒 Suggested fix: redact the password in `Debug`
-#[derive(Debug, Clone, Serialize, Deserialize)]
+#[derive(Clone, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct ClusterEndpointSourceConfig {
...
}
+
+impl std::fmt::Debug for ClusterEndpointSourceConfig {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.debug_struct("ClusterEndpointSourceConfig")
+ .field("inventory_path", &self.inventory_path)
+ .field("cluster_manager_url", &self.cluster_manager_url)
+ .field("cluster_manager_partition", &self.cluster_manager_partition)
+ .field("default_username", &self.default_username)
+ .field("default_password", &self.default_password.as_ref().map(|_| "<redacted>"))
+ .field("port", &self.port)
+ .finish()
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[derive(Debug, Clone, Serialize, Deserialize)] | |
| #[serde(deny_unknown_fields)] | |
| pub struct ClusterEndpointSourceConfig { | |
| /// Path to a JSON inventory file containing BMC endpoints and credentials for the cluster. | |
| /// Used when `cluster_manager_url` is absent. | |
| #[serde(default)] | |
| pub inventory_path: PathBuf, | |
| /// Cluster manager head-node URL (e.g. https://10.x.x.x:8081). | |
| /// When set, inventory and credentials are fetched live via cluster manager JSON RPC | |
| /// instead of reading `inventory_path`. | |
| #[serde(default)] | |
| pub cluster_manager_url: Option<url::Url>, | |
| /// Cluster manager partition to read bmcsettings from (default: "base"). | |
| /// The cluster manager stores BMC username/password at partition level; this selects which partition. | |
| #[serde(default = "default_cluster_manager_partition")] | |
| pub cluster_manager_partition: String, | |
| /// Fallback BMC username if cluster manager JSON RPC does not return one. | |
| /// Cluster manager default is "bright" (set during head-node installation). | |
| #[serde(default = "default_cluster_manager_username")] | |
| pub default_username: String, | |
| /// Fallback BMC password if cluster manager JSON RPC does not return one. | |
| /// Must be set explicitly — no code-level default. | |
| #[serde(default)] | |
| pub default_password: Option<String>, | |
| /// Optional BMC port override. None uses the BmcClient default (443/HTTPS). | |
| #[serde(default)] | |
| pub port: Option<u16>, | |
| } | |
| #[derive(Clone, Serialize, Deserialize)] | |
| #[serde(deny_unknown_fields)] | |
| pub struct ClusterEndpointSourceConfig { | |
| /// Path to a JSON inventory file containing BMC endpoints and credentials for the cluster. | |
| /// Used when `cluster_manager_url` is absent. | |
| #[serde(default)] | |
| pub inventory_path: PathBuf, | |
| /// Cluster manager head-node URL (e.g. https://10.x.x.x:8081). | |
| /// When set, inventory and credentials are fetched live via cluster manager JSON RPC | |
| /// instead of reading `inventory_path`. | |
| #[serde(default)] | |
| pub cluster_manager_url: Option<url::Url>, | |
| /// Cluster manager partition to read bmcsettings from (default: "base"). | |
| /// The cluster manager stores BMC username/password at partition level; this selects which partition. | |
| #[serde(default = "default_cluster_manager_partition")] | |
| pub cluster_manager_partition: String, | |
| /// Fallback BMC username if cluster manager JSON RPC does not return one. | |
| /// Cluster manager default is "bright" (set during head-node installation). | |
| #[serde(default = "default_cluster_manager_username")] | |
| pub default_username: String, | |
| /// Fallback BMC password if cluster manager JSON RPC does not return one. | |
| /// Must be set explicitly — no code-level default. | |
| #[serde(default)] | |
| pub default_password: Option<String>, | |
| /// Optional BMC port override. None uses the BmcClient default (443/HTTPS). | |
| #[serde(default)] | |
| pub port: Option<u16>, | |
| } | |
| impl std::fmt::Debug for ClusterEndpointSourceConfig { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| f.debug_struct("ClusterEndpointSourceConfig") | |
| .field("inventory_path", &self.inventory_path) | |
| .field("cluster_manager_url", &self.cluster_manager_url) | |
| .field("cluster_manager_partition", &self.cluster_manager_partition) | |
| .field("default_username", &self.default_username) | |
| .field("default_password", &self.default_password.as_ref().map(|_| "<redacted>")) | |
| .field("port", &self.port) | |
| .finish() | |
| } | |
| } |
🤖 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 102 - 134,
ClusterEndpointSourceConfig’s derived Debug implementation exposes
default_password in logs. Replace the derived Debug with a manual implementation
that redacts or omits default_password, following StaticBmcEndpoint’s custom
Debug pattern, while preserving the existing serialization behavior unless
separately required.
Comment on lines
+104
to
+134
| pub struct ClusterEndpointSourceConfig { | ||
| /// Path to a JSON inventory file containing BMC endpoints and credentials for the cluster. | ||
| /// Used when `cluster_manager_url` is absent. | ||
| #[serde(default)] | ||
| pub inventory_path: PathBuf, | ||
|
|
||
| /// Cluster manager head-node URL (e.g. https://10.x.x.x:8081). | ||
| /// When set, inventory and credentials are fetched live via cluster manager JSON RPC | ||
| /// instead of reading `inventory_path`. | ||
| #[serde(default)] | ||
| pub cluster_manager_url: Option<url::Url>, | ||
|
|
||
| /// Cluster manager partition to read bmcsettings from (default: "base"). | ||
| /// The cluster manager stores BMC username/password at partition level; this selects which partition. | ||
| #[serde(default = "default_cluster_manager_partition")] | ||
| pub cluster_manager_partition: String, | ||
|
|
||
| /// Fallback BMC username if cluster manager JSON RPC does not return one. | ||
| /// Cluster manager default is "bright" (set during head-node installation). | ||
| #[serde(default = "default_cluster_manager_username")] | ||
| pub default_username: String, | ||
|
|
||
| /// Fallback BMC password if cluster manager JSON RPC does not return one. | ||
| /// Must be set explicitly — no code-level default. | ||
| #[serde(default)] | ||
| pub default_password: Option<String>, | ||
|
|
||
| /// Optional BMC port override. None uses the BmcClient default (443/HTTPS). | ||
| #[serde(default)] | ||
| pub port: Option<u16>, | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## files\n'
git ls-files crates/health/src | sed -n '1,120p'
printf '\n## outline config.rs\n'
ast-grep outline crates/health/src/config.rs --view expanded || true
printf '\n## outline cluster.rs\n'
ast-grep outline crates/health/src/cluster.rs --view expanded || true
printf '\n## search validate and endpoint source references\n'
rg -n "ClusterEndpointSourceConfig|validate\\(|inventory_path|cluster_manager_url|read_from_file" crates/health/src -n
printf '\n## targeted read config.rs around relevant lines\n'
wc -l crates/health/src/config.rs crates/health/src/cluster.rs
sed -n '1,260p' crates/health/src/config.rs
printf '\n--- cluster.rs ---\n'
sed -n '1,260p' crates/health/src/cluster.rsRepository: NVIDIA/infra-controller
Length of output: 24070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## config validation section\n'
sed -n '1290,1405p' crates/health/src/config.rs
printf '\n## cluster endpoint loader\n'
sed -n '260,360p' crates/health/src/endpoint/cluster.rs
printf '\n## config validation tests nearby\n'
sed -n '1680,1775p' crates/health/src/config.rsRepository: NVIDIA/infra-controller
Length of output: 10538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## cluster config impls/tests\n'
rg -n "impl ClusterEndpointSourceConfig|cluster_manager_url|inventory_path|endpoint_sources\.cluster|Loaded cluster endpoints|Failed to read cluster inventory" crates/health/src/config.rs crates/health/src/endpoint -n
printf '\n## default endpoint sources config\n'
sed -n '80,160p' crates/health/src/config.rs
printf '\n## endpoint source config tests around cluster/static config\n'
sed -n '1460,1710p' crates/health/src/config.rsRepository: NVIDIA/infra-controller
Length of output: 13386
Validate cluster endpoint source configuration before startup
Config::validate() does not enforce any requirement on ClusterEndpointSourceConfig, so enabling [endpoint_sources.cluster] with both inventory_path unset and cluster_manager_url = none falls through to an empty-path file read in crates/health/src/endpoint/cluster.rs. Add a ClusterEndpointSourceConfig::validate() and invoke it from Config::validate() to reject that case with a clear config error.
🤖 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 104 - 134, Add
ClusterEndpointSourceConfig::validate() to require either a non-empty
inventory_path or a configured cluster_manager_url, returning a clear
configuration error when both are absent; then invoke this validation from
Config::validate() whenever the cluster endpoint source is enabled.
Comment on lines
+88
to
+93
| fn build_http() -> Result<HttpClient, HealthError> { | ||
| HttpClient::builder() | ||
| .danger_accept_invalid_certs(true) | ||
| .build() | ||
| .map_err(|e| HealthError::GenericError(format!("Cluster manager HTTP client build failed: {e}"))) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant file with line numbers.
sed -n '1,220p' crates/health/src/endpoint/cluster.rs | cat -n
echo
echo "== search for build_http and credential fetch usage =="
rg -n "build_http|danger_accept_invalid_certs|credential|username|password|BMC|cluster manager|fetch.*credential|get.*credential|login" crates/health/src -S
echo
echo "== search for config flags related to TLS validation =="
rg -n "accept_invalid_certs|danger_accept_invalid_certs|tls.*invalid|insecure.*tls|skip.*cert|certificate verification|verify.*cert" crates/health/src -SRepository: NVIDIA/infra-controller
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cluster endpoint config section =="
sed -n '96,150p' crates/health/src/config.rs | cat -n
echo
echo "== cluster.rs around the raw response logging and credential fetch flow =="
sed -n '128,190p' crates/health/src/endpoint/cluster.rs | cat -n
echo
echo "== exact occurrences of raw response logging in cluster.rs =="
rg -n "response = \?raw|Raw cluster manager|bmcSettings|default_password|default_username|danger_accept_invalid_certs" crates/health/src/endpoint/cluster.rs -n -SRepository: NVIDIA/infra-controller
Length of output: 6177
Disable TLS certificate bypass on the cluster-manager credential client. build_http() unconditionally sets danger_accept_invalid_certs(true) for the HTTPS client that fetches BMC credentials, and this path also logs the raw partition response at debug. That makes the credential flow vulnerable to MITM and log exposure. Add an explicit opt-in flag or require a trusted certificate chain here.
🤖 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/cluster.rs` around lines 88 - 93, The build_http
function unconditionally disables TLS certificate validation for the
cluster-manager credential client. Remove danger_accept_invalid_certs(true) and
require normal trusted certificate-chain validation, or gate the bypass behind
an explicit opt-in configuration flag; also avoid logging raw partition
responses in the credential-fetching path.
Comment on lines
+128
to
+189
| async fn fetch_manager_credentials( | ||
| http: &HttpClient, | ||
| json_rpc_url: &Url, | ||
| cfg: &ClusterEndpointSourceConfig, | ||
| ) -> (String, Option<String>) { | ||
| // cmsh equivalent: partition use <partition> → bmcsettings → get username / get password | ||
| // Password stored by the cluster manager daemon at partition level. | ||
| let result = manager_rpc( | ||
| http, | ||
| json_rpc_url, | ||
| "cmpart", | ||
| MANAGER_PARTITION_CALL, | ||
| json!(cfg.cluster_manager_partition), | ||
| ) | ||
| .await; | ||
|
|
||
| let raw = match result { | ||
| Ok(v) => v, | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| error = %e, | ||
| partition = %cfg.cluster_manager_partition, | ||
| "Failed to fetch cluster manager partition credentials; using config fallback" | ||
| ); | ||
| return (cfg.default_username.clone(), cfg.default_password.clone()); | ||
| } | ||
| }; | ||
|
|
||
| tracing::debug!(response = ?raw, "Raw cluster manager partition response"); | ||
|
|
||
| // Navigate to bmcsettings — try likely paths. | ||
| // Update once live /api confirms the exact field layout. | ||
| let bmc_settings = raw | ||
| .pointer("/bmcSettings") | ||
| .or_else(|| raw.pointer("/result/bmcSettings")) | ||
| .or_else(|| raw.pointer("/data/bmcSettings")); | ||
|
|
||
| let username = bmc_settings | ||
| .and_then(|s| s.get("username").or_else(|| s.get("userName"))) | ||
| .and_then(|v| v.as_str()) | ||
| .map(str::to_string) | ||
| .unwrap_or_else(|| { | ||
| tracing::debug!( | ||
| "Cluster manager partition response missing bmcSettings.username; using config fallback. \ | ||
| Probe /api on head node to find correct field path." | ||
| ); | ||
| cfg.default_username.clone() | ||
| }); | ||
|
|
||
| let password = bmc_settings | ||
| .and_then(|s| s.get("password").or_else(|| s.get("bmcPassword"))) | ||
| .and_then(|v| v.as_str()) | ||
| .map(str::to_string) | ||
| .or_else(|| { | ||
| tracing::debug!( | ||
| "Cluster manager partition response missing bmcSettings.password; using config fallback." | ||
| ); | ||
| cfg.default_password.clone() | ||
| }); | ||
|
|
||
| (username, password) | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Cluster-manager credentials are logged in plaintext.
tracing::debug!(response = ?raw, "Raw cluster manager partition response") dumps the entire JSON-RPC response — which, per the surrounding logic, contains bmcSettings.password — straight into the log stream. Any operator running this service with debug logging enabled (a common troubleshooting step) will leak BMC passwords into logs/log aggregators. As per coding guidelines, "All services should emit logs in 'logfmt' syntax" with structured fields, but structured logging still must avoid emitting secrets; this is the same principle applied to the log_request_data filtering requirement for API handlers.
🔒 Suggested fix: redact before logging
- tracing::debug!(response = ?raw, "Raw cluster manager partition response");
+ tracing::debug!(
+ has_bmc_settings = raw.pointer("/bmcSettings").is_some()
+ || raw.pointer("/result/bmcSettings").is_some()
+ || raw.pointer("/data/bmcSettings").is_some(),
+ "Raw cluster manager partition response (redacted)"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async fn fetch_manager_credentials( | |
| http: &HttpClient, | |
| json_rpc_url: &Url, | |
| cfg: &ClusterEndpointSourceConfig, | |
| ) -> (String, Option<String>) { | |
| // cmsh equivalent: partition use <partition> → bmcsettings → get username / get password | |
| // Password stored by the cluster manager daemon at partition level. | |
| let result = manager_rpc( | |
| http, | |
| json_rpc_url, | |
| "cmpart", | |
| MANAGER_PARTITION_CALL, | |
| json!(cfg.cluster_manager_partition), | |
| ) | |
| .await; | |
| let raw = match result { | |
| Ok(v) => v, | |
| Err(e) => { | |
| tracing::warn!( | |
| error = %e, | |
| partition = %cfg.cluster_manager_partition, | |
| "Failed to fetch cluster manager partition credentials; using config fallback" | |
| ); | |
| return (cfg.default_username.clone(), cfg.default_password.clone()); | |
| } | |
| }; | |
| tracing::debug!(response = ?raw, "Raw cluster manager partition response"); | |
| // Navigate to bmcsettings — try likely paths. | |
| // Update once live /api confirms the exact field layout. | |
| let bmc_settings = raw | |
| .pointer("/bmcSettings") | |
| .or_else(|| raw.pointer("/result/bmcSettings")) | |
| .or_else(|| raw.pointer("/data/bmcSettings")); | |
| let username = bmc_settings | |
| .and_then(|s| s.get("username").or_else(|| s.get("userName"))) | |
| .and_then(|v| v.as_str()) | |
| .map(str::to_string) | |
| .unwrap_or_else(|| { | |
| tracing::debug!( | |
| "Cluster manager partition response missing bmcSettings.username; using config fallback. \ | |
| Probe /api on head node to find correct field path." | |
| ); | |
| cfg.default_username.clone() | |
| }); | |
| let password = bmc_settings | |
| .and_then(|s| s.get("password").or_else(|| s.get("bmcPassword"))) | |
| .and_then(|v| v.as_str()) | |
| .map(str::to_string) | |
| .or_else(|| { | |
| tracing::debug!( | |
| "Cluster manager partition response missing bmcSettings.password; using config fallback." | |
| ); | |
| cfg.default_password.clone() | |
| }); | |
| (username, password) | |
| } | |
| async fn fetch_manager_credentials( | |
| http: &HttpClient, | |
| json_rpc_url: &Url, | |
| cfg: &ClusterEndpointSourceConfig, | |
| ) -> (String, Option<String>) { | |
| // cmsh equivalent: partition use <partition> → bmcsettings → get username / get password | |
| // Password stored by the cluster manager daemon at partition level. | |
| let result = manager_rpc( | |
| http, | |
| json_rpc_url, | |
| "cmpart", | |
| MANAGER_PARTITION_CALL, | |
| json!(cfg.cluster_manager_partition), | |
| ) | |
| .await; | |
| let raw = match result { | |
| Ok(v) => v, | |
| Err(e) => { | |
| tracing::warn!( | |
| error = %e, | |
| partition = %cfg.cluster_manager_partition, | |
| "Failed to fetch cluster manager partition credentials; using config fallback" | |
| ); | |
| return (cfg.default_username.clone(), cfg.default_password.clone()); | |
| } | |
| }; | |
| tracing::debug!( | |
| has_bmc_settings = raw.pointer("/bmcSettings").is_some() | |
| || raw.pointer("/result/bmcSettings").is_some() | |
| || raw.pointer("/data/bmcSettings").is_some(), | |
| "Raw cluster manager partition response (redacted)" | |
| ); | |
| // Navigate to bmcsettings — try likely paths. | |
| // Update once live /api confirms the exact field layout. | |
| let bmc_settings = raw | |
| .pointer("/bmcSettings") | |
| .or_else(|| raw.pointer("/result/bmcSettings")) | |
| .or_else(|| raw.pointer("/data/bmcSettings")); | |
| let username = bmc_settings | |
| .and_then(|s| s.get("username").or_else(|| s.get("userName"))) | |
| .and_then(|v| v.as_str()) | |
| .map(str::to_string) | |
| .unwrap_or_else(|| { | |
| tracing::debug!( | |
| "Cluster manager partition response missing bmcSettings.username; using config fallback. \ | |
| Probe /api on head node to find correct field path." | |
| ); | |
| cfg.default_username.clone() | |
| }); | |
| let password = bmc_settings | |
| .and_then(|s| s.get("password").or_else(|| s.get("bmcPassword"))) | |
| .and_then(|v| v.as_str()) | |
| .map(str::to_string) | |
| .or_else(|| { | |
| tracing::debug!( | |
| "Cluster manager partition response missing bmcSettings.password; using config fallback." | |
| ); | |
| cfg.default_password.clone() | |
| }); | |
| (username, password) | |
| } |
🤖 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/cluster.rs` around lines 128 - 189, Remove the
plaintext response logging from fetch_manager_credentials by deleting the
tracing::debug! call that emits raw. If response diagnostics are required,
create a redacted representation that removes or masks bmcSettings.password and
any equivalent credential fields before logging it, while preserving structured
logfmt fields.
Comment on lines
+191
to
+270
| fn extract_manager_devices( | ||
| raw: &serde_json::Value, | ||
| username: String, | ||
| password: Option<String>, | ||
| ) -> Vec<ClusterNode> { | ||
| // The device list may be a top-level array or wrapped in result/data/items. | ||
| // Update once live /api confirms the exact response shape. | ||
| let items: Vec<&serde_json::Value> = if let Some(arr) = raw.as_array() { | ||
| arr.iter().collect() | ||
| } else { | ||
| ["result", "data", "items", "devices"] | ||
| .iter() | ||
| .find_map(|key| raw.get(key).and_then(|v| v.as_array())) | ||
| .map(|arr| arr.iter().collect()) | ||
| .unwrap_or_else(|| { | ||
| tracing::warn!( | ||
| response = ?raw, | ||
| "Cluster manager getDevices response has no recognized shape; \ | ||
| probe /api on head node and update extract_manager_devices in cluster.rs" | ||
| ); | ||
| vec![] | ||
| }) | ||
| }; | ||
|
|
||
| let mut nodes = Vec::new(); | ||
| for item in items { | ||
| let hostname = item | ||
| .get("hostname") | ||
| .or_else(|| item.get("name")) | ||
| .and_then(|v| v.as_str()) | ||
| .map(str::to_string); | ||
|
|
||
| // BMC IP lives in the BMC interface, not at the top level of the device. | ||
| // cmsh: device → interfaces → use ipmi0/ilo0/rf0 → get ip | ||
| // Try common field paths; update once confirmed. | ||
| let bmc_ip_str = item | ||
| .pointer("/interfaces/ipmi0/ip") | ||
| .or_else(|| item.pointer("/interfaces/bmc/ip")) | ||
| .or_else(|| item.pointer("/interfaces/ilo0/ip")) | ||
| .or_else(|| item.pointer("/interfaces/rf0/ip")) | ||
| .or_else(|| item.get("bmcAddress")) | ||
| .or_else(|| item.get("bmcIp")) | ||
| .or_else(|| item.get("ipmiAddress")) | ||
| .and_then(|v| v.as_str()); | ||
|
|
||
| // Cluster manager category maps to our rack identifier. | ||
| let rack = item | ||
| .get("category") | ||
| .or_else(|| item.get("partition")) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("") | ||
| .to_string(); | ||
|
|
||
| let (Some(hostname), Some(bmc_ip_str)) = (hostname, bmc_ip_str) else { | ||
| tracing::debug!( | ||
| item = ?item, | ||
| "Cluster manager device entry missing hostname or BMC IP; skipping. \ | ||
| Update field paths in extract_manager_devices once head node is probed." | ||
| ); | ||
| continue; | ||
| }; | ||
|
|
||
| let bmc_ip: IpAddr = match bmc_ip_str.parse() { | ||
| Ok(ip) => ip, | ||
| Err(e) => { | ||
| tracing::warn!(hostname, bmc_ip = bmc_ip_str, error = %e, "Invalid BMC IP; skipping"); | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| nodes.push(ClusterNode { | ||
| hostname, | ||
| bmc_ip, | ||
| rack, | ||
| username: username.clone(), | ||
| password: password.clone(), | ||
| }); | ||
| } | ||
| nodes | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files crates/health/src/endpoint/cluster.rs
printf '\nSymbols/search hits:\n'
rg -n "extract_manager_devices|fetch_manager_credentials|getDevices|getPartition|cmdevice|cmpart|interfaces/ipmi0|bmcAddress|bmcIp|ipmiAddress" crates/health/src/endpoint/cluster.rs crates/health -S || true
printf '\nOutline:\n'
ast-grep outline crates/health/src/endpoint/cluster.rs --view expanded || true
printf '\nRelevant source excerpt:\n'
nl -ba crates/health/src/endpoint/cluster.rs | sed -n '1,360p'Repository: NVIDIA/infra-controller
Length of output: 4759
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Source excerpt (1-360):\n'
sed -n '1,360p' crates/health/src/endpoint/cluster.rs
printf '\nTests/search in health crate:\n'
rg -n "extract_manager_devices|fetch_manager_credentials|ClusterEndpointSource|cmdevice|cmpart|bmcsettings|getDevices|getPartition" crates/health -g '*.{rs,md,toml}' -S || true
printf '\nAny dedicated tests for cluster.rs:\n'
fd -a 'cluster.rs' crates/health || true
fd -a '*test*' crates/health || trueRepository: NVIDIA/infra-controller
Length of output: 15630
Fail closed on unknown Cluster Manager payloads
fetch_manager_credentials and extract_manager_devices still parse provisional JSON paths, and an unrecognized getDevices payload is treated as success with an empty node list. That turns a response-shape mismatch into a silent empty inventory. Fail closed, or emit a countable error when a non-empty response yields zero nodes; the credential path should avoid silent fallback on missing fields as well.
🤖 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/cluster.rs` around lines 191 - 270, Make Cluster
Manager parsing fail closed: update fetch_manager_credentials to reject missing
or unrecognized credential fields instead of silently falling back, and update
extract_manager_devices to return or propagate a countable error when a
non-empty getDevices response has no recognized shape or produces zero valid
nodes. Preserve explicit handling for valid empty arrays, and ensure callers
surface the parsing failure rather than treating it as a successful empty
inventory.
Comment on lines
+236
to
+242
| // Cluster manager category maps to our rack identifier. | ||
| let rack = item | ||
| .get("category") | ||
| .or_else(|| item.get("partition")) | ||
| .and_then(|v| v.as_str()) | ||
| .unwrap_or("") | ||
| .to_string(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty rack falls back to Some(RackId::new("")) instead of None.
When a cluster-manager device entry has no category/partition field, rack defaults to "" (Line 241-242), and build_endpoints unconditionally wraps it as Some(RackId::new(&node.rack)) (Line 400). This groups all rack-less cluster endpoints under an empty-string rack id, which is then used as the hash_key() for sharding/grouping (see BmcEndpoint::hash_key in model.rs) — silently lumping unrelated endpoints together instead of falling back to the per-endpoint MAC key as None would.
🐛 Suggested fix
- endpoints.push(Arc::new(BmcEndpoint {
- addr,
- metadata: None,
- rack_id: Some(RackId::new(&node.rack)),
- bmc,
- }));
+ let rack_id = if node.rack.is_empty() {
+ None
+ } else {
+ Some(RackId::new(&node.rack))
+ };
+ endpoints.push(Arc::new(BmcEndpoint {
+ addr,
+ metadata: None,
+ rack_id,
+ bmc,
+ }));Also applies to: 397-402
🤖 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/cluster.rs` around lines 236 - 242, Preserve the
absence of a rack identifier in the cluster-manager parsing and endpoint
construction. Update the logic producing `rack` near the category/partition
lookup to return an optional value rather than defaulting to an empty string,
and update `build_endpoints` to pass `None` when no non-empty rack exists
instead of unconditionally constructing `Some(RackId::new(&node.rack))`; retain
`Some(RackId)` only for a valid rack so `BmcEndpoint::hash_key` falls back to
the endpoint MAC.
Comment on lines
+274
to
+295
| pub struct ClusterEndpointSource { | ||
| endpoints: Vec<Arc<BmcEndpoint>>, | ||
| } | ||
|
|
||
| impl ClusterEndpointSource { | ||
| pub async fn from_config( | ||
| cfg: &ClusterEndpointSourceConfig, | ||
| reqwest: &ReqwestClient, | ||
| proxy_url: Option<&Url>, | ||
| cache_size: usize, | ||
| ) -> Result<Self, HealthError> { | ||
| let nodes = if let Some(ref cluster_manager_url) = cfg.cluster_manager_url { | ||
| fetch_from_manager(cfg, cluster_manager_url).await? | ||
| } else { | ||
| read_from_file(cfg)? | ||
| }; | ||
|
|
||
| let endpoints = build_endpoints(nodes, cfg.port, reqwest, proxy_url, cache_size); | ||
| tracing::info!(count = endpoints.len(), "Loaded cluster endpoints"); | ||
| Ok(Self { endpoints }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cluster.rs outline =="
ast-grep outline crates/health/src/endpoint/cluster.rs --view expanded || true
echo
echo "== relevant slices from cluster.rs =="
sed -n '1,220p' crates/health/src/endpoint/cluster.rs
echo
sed -n '220,460p' crates/health/src/endpoint/cluster.rs
echo
echo "== discovery loop references =="
rg -n "endpoint_discovery_interval|fetch_bmc_hosts|ClusterEndpointSource|ApiEndpointSource|StaticEndpointSource" crates/health/src -n -A 3 -B 3
echo
echo "== lib.rs / caller context =="
sed -n '1,260p' crates/health/src/lib.rsRepository: NVIDIA/infra-controller
Length of output: 48064
Refresh cluster inventory on each discovery pass
ClusterEndpointSource snapshots the file/RPC inventory in from_config(), and fetch_bmc_hosts() only clones that cached vector. Added, removed, or re-IP’d nodes will never be picked up until the process restarts. Re-read the inventory on each fetch_bmc_hosts() call, or move this behind a shared refresh path.
🤖 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/cluster.rs` around lines 274 - 295, Refresh the
cluster inventory during every discovery pass instead of caching it only in
ClusterEndpointSource::from_config. Update ClusterEndpointSource and
fetch_bmc_hosts to re-read via fetch_from_manager or read_from_file, rebuild
endpoints with the configured port, clients, proxy, and cache size, and return
the refreshed set so additions, removals, and IP changes are observed without
restarting.
Comment on lines
+153
to
+162
| if let Configurable::Enabled(ref source_cfg) = config.endpoint_sources.cluster { | ||
| let cluster_source = ClusterEndpointSource::from_config( | ||
| source_cfg, | ||
| &reqwest, | ||
| config.bmc_proxy_url.as_ref(), | ||
| config.cache_size, | ||
| ) | ||
| .await?; | ||
| sources.push(Arc::new(cluster_source)); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby functions.
ast-grep outline crates/health/src/lib.rs --view expanded || true
# Read the relevant section with line numbers.
sed -n '120,230p' crates/health/src/lib.rs | cat -n
# Find ClusterEndpointSource and run_service references in the repo.
rg -n "ClusterEndpointSource::from_config|build_endpoint_wiring|run_service|static_bmc_endpoints|carbide_api|ApiClientWrapper::new|ApiEndpointSource::new" crates/health -SRepository: NVIDIA/infra-controller
Length of output: 14211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the health crate structure and the relevant function bodies.
ast-grep outline crates/health/src/lib.rs --view expanded
printf '\n--- relevant slice ---\n'
sed -n '140,340p' crates/health/src/lib.rs | cat -n
printf '\n--- call sites ---\n'
rg -n "ClusterEndpointSource::from_config|build_endpoint_wiring|run_service|static_bmc_endpoints|ApiClientWrapper::new|ApiEndpointSource::new" crates/ -SRepository: NVIDIA/infra-controller
Length of output: 15947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the cluster endpoint source implementation and inspect its constructor.
fd -a "cluster" crates/health/src || true
rg -n "struct ClusterEndpointSource|impl ClusterEndpointSource|from_config\(" crates/health/src -S
# Read the implementation file once identified.
file="$(rg -l "struct ClusterEndpointSource|impl ClusterEndpointSource" crates/health/src -S | head -n 1)"
echo "--- file: $file ---"
sed -n '1,260p' "$file" | cat -nRepository: NVIDIA/infra-controller
Length of output: 12309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '260,420p' crates/health/src/endpoint/cluster.rs | cat -n
printf '\n--- from_config signature and body ---\n'
rg -n "pub async fn from_config|fetch_manager_credentials|extract_manager_devices|manager_rpc" crates/health/src/endpoint/cluster.rs -n -A120 -B20Repository: NVIDIA/infra-controller
Length of output: 18990
Cluster inventory fetch still blocks startup. fetch_manager_credentials already falls back on RPC failures, but fetch_from_manager(...).await? still propagates device-inventory and URL/RPC errors, so a transient Cluster Manager outage aborts build_endpoint_wiring and prevents otherwise healthy sources from starting. Consider making this source best-effort with logging plus skip/retry-backoff.
🤖 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/lib.rs` around lines 153 - 162, Make cluster endpoint
initialization best-effort in ClusterEndpointSource::from_config: handle
device-inventory and URL/RPC failures locally instead of propagating them
through build_endpoint_wiring, log the failure, and skip the cluster source (or
apply the existing retry/backoff mechanism) so other sources can start normally.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
|
There are couple of coderabbit comments which are relevant. (most marked as Major). Two which required to be fixed are password debug and tls danger_accept_invalid_certs (this can be moved to config) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a new EndpointSource that ingests cluster rack inventory from either a local JSON file or Cluster manager's JSON RPC API.
File path: reads a JSON inventory file containing
hostname, BMC IP, rack, and default credentials for each node.
Cluster Manager's JSON RPC path (placeholder, pending head-node IP confirmation):
Both paths produce the same Vec fed into build_endpoints(). MAC addresses are derived deterministically from BMC IPv4 octets (02:00::::) as internal cache keys only.
config: cluster source is Disabled by default; enable with:
[endpoint_sources.cluster]
inventory_path = "/path/to/inventory.json"
or
cluster_manager_url = "https://:8081"
Related issues
Fixes #3358
Type of Change
Breaking Changes
Testing
Manual testing
Manual testing performed against a single BMC endpoint using the file-based
inventory path (
inventory_path). Verified that:ClusterEndpointSourcecorrectly loads rack/node hierarchy from JSON inventorydefault_credentials[endpoint_sources.cluster]is absent from configAdditional Notes