fix(redfish): bracket IPv6 BMC hosts in nv-redfish URLs#3008
fix(redfish): bracket IPv6 BMC hosts in nv-redfish URLs#3008CTOmari360 wants to merge 2 commits into
Conversation
NvRedfishClientPool::create_bmc built the BMC base URL with bare format!s and then .expect()ed Url::parse. IPv6 hosts were left unbracketed, so an IPv6 BMC produced an invalid authority such as https://2001:db8::1:8443 -- which Url::parse rejects, panicking the caller. This mirrors the bracketing the health crate's BmcAddr::to_url already applies on its own path. Extract a testable build_bmc_url helper that brackets IPv6 literals in every arm: the port-only proxy path uses the BMC's own IpAddr, and the host / host+port proxy paths bracket config-supplied IPv6 literals. The no-override arm is unchanged -- SocketAddr's Display already brackets. Add unit tests covering each arm; the port-only IPv6 case fails before this change. Part of the NVIDIA#2237 IPv6 bug bucket; addresses the Redfish-bracket item of NVIDIA#2406. Signed-off-by: Omar Refai <[email protected]>
| fn url_host(host: &str) -> Cow<'_, str> { | ||
| if host.parse::<Ipv6Addr>().is_ok() { | ||
| Cow::Owned(format!("[{host}]")) | ||
| } else { | ||
| Cow::Borrowed(host) | ||
| } | ||
| } |
There was a problem hiding this comment.
This code doesn't look right. Probably HostPortPair should be able to destinguish IPv6 address from Hostname or return url_host(). This code here looks like patch on wrong / missing behavior of other object.
Could you please move this function there to make incoherent design of HostAndPort more local?
|
Same question as on the golang code, can we use the url library to do this IPv6 bracketing for us? If we're dealing with URLs we should probably use the url crate. |
Per review (ajf): build the BMC URL with the `url` crate instead of hand-formatting the authority. build_bmc_url now returns a `url::Url` assembled via `Url::set_ip_host` / `set_host` / `set_port` -- `set_ip_host` brackets IPv6 literals for us, so the manual `[..]` formatting (and the separate `url_host` helper) is gone, along with the string-parse-then-`.expect()` at the call site. Behavior is unchanged: the returned Url is identical to what the old string produced once parsed (the url crate canonicalizes the https default port either way). Proxy hosts that are IP literals go through set_ip_host; hostnames through set_host. Tests updated to assert on the Url (host_str/port) instead of the raw string; the IPv6 bracketing cases still guard the original panic. Part of NVIDIA#2237 (IPv6 Bug Scan Bucket). Signed-off-by: Omar Refai <[email protected]>
|
Done — reworked it to let the Proxy hosts that come in as IP literals go through |
Summary by CodeRabbit
WalkthroughThe BMC base URL construction in ChangesBMC URL Authority Construction
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant create_bmc
participant build_bmc_url
participant set_host
participant Url
create_bmc->>build_bmc_url: proxy_address, bmc_address
build_bmc_url->>set_host: host string
set_host->>set_host: detect IP literal vs hostname
alt IP literal
set_host->>Url: set_ip_host
else hostname
set_host->>Url: set_host
end
Url-->>build_bmc_url: constructed Url
build_bmc_url-->>create_bmc: Url
Possibly related issues
🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/redfish/src/nv_redfish/mod.rs (1)
141-148: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConfig-supplied proxy host can now panic the service instead of returning an
Error.
create_bmcreturnsResult<Arc<RedfishBmc>, Error>, butbuild_bmc_urlbypasses that contract: if the configured proxy host inHostPortPair::HostAndPort/HostOnlyfailsUrl::set_hostvalidation (malformed hostname, stray whitespace, an already-bracketed IPv6 literal, etc.),set_host(Lines 219-226) panics via.expect("proxy host is expected to be a valid URL host")instead of surfacing a recoverable error tocreate_bmc's caller. Sinceproxy_addressis loaded from anArcSwap(dynamic/admin-configurable), a bad config value would crash the process rather than degrade gracefully.Consider having
build_bmc_urlreturn aResult<Url, Error>(or a dedicated error type) and threading it throughcreate_bmcwith?, so invalid proxy configuration surfaces as a normalErrorinstead of a panic.🛡️ Sketch of a non-panicking alternative
-fn set_host(url: &mut Url, host: &str) { +fn set_host(url: &mut Url, host: &str) -> Result<(), Error> { match host.parse::<IpAddr>() { - Ok(ip) => set_ip_host(url, ip), - Err(_) => url - .set_host(Some(host)) - .expect("proxy host is expected to be a valid URL host"), + Ok(ip) => set_ip_host(url, ip), + Err(_) => url + .set_host(Some(host)) + .map_err(|e| Error::Bmc(format!("invalid proxy host {host:?}: {e}").into())), } }Also applies to: 219-226
🤖 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/redfish/src/nv_redfish/mod.rs` around lines 141 - 148, `create_bmc` should not be able to panic from invalid proxy configuration; `build_bmc_url` currently uses `set_host` with `expect`, which breaks the `Result<Arc<RedfishBmc>, Error>` contract. Change `build_bmc_url` to return `Result<Url, Error>` (or a dedicated error type), propagate the host-validation failure instead of calling `expect`, and update `create_bmc` to use `?` when building the URL so bad values from `proxy_address` surface as a normal error.
🧹 Nitpick comments (1)
crates/redfish/src/nv_redfish/mod.rs (1)
228-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid regression coverage; consider adding a malformed-host case.
The IPv6 bracketing, IPv4 passthrough, and hostname-passthrough cases are all covered well. Given the panic-on-
.expect()concern above, a test asserting the current (undesirable) panic behavior for a malformed proxy host would document the gap and make a future fix easy to verify.🤖 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/redfish/src/nv_redfish/mod.rs` around lines 228 - 305, Add a regression test in the nv_redfish tests module for a malformed proxy host to document the current panic path through build_bmc_url and create_bmc’s Url::parse/.expect flow. Use the existing helpers and symbols like build_bmc_url and HostPortPair to construct an invalid host case, then assert the undesired panic behavior so the gap is covered and a future fix can be verified.
🤖 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.
Outside diff comments:
In `@crates/redfish/src/nv_redfish/mod.rs`:
- Around line 141-148: `create_bmc` should not be able to panic from invalid
proxy configuration; `build_bmc_url` currently uses `set_host` with `expect`,
which breaks the `Result<Arc<RedfishBmc>, Error>` contract. Change
`build_bmc_url` to return `Result<Url, Error>` (or a dedicated error type),
propagate the host-validation failure instead of calling `expect`, and update
`create_bmc` to use `?` when building the URL so bad values from `proxy_address`
surface as a normal error.
---
Nitpick comments:
In `@crates/redfish/src/nv_redfish/mod.rs`:
- Around line 228-305: Add a regression test in the nv_redfish tests module for
a malformed proxy host to document the current panic path through build_bmc_url
and create_bmc’s Url::parse/.expect flow. Use the existing helpers and symbols
like build_bmc_url and HostPortPair to construct an invalid host case, then
assert the undesired panic behavior so the gap is covered and a future fix can
be verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81ad9da2-6e9f-4eb0-a82f-24ccbf3c8343
📒 Files selected for processing (1)
crates/redfish/src/nv_redfish/mod.rs
|
/ok to test 21f7ed8 |
| /// for us, so an IPv6 BMC (or proxy host) yields a valid URL. A bare, unbracketed | ||
| /// `2001:db8::1` authority would otherwise be rejected by the parser. | ||
| fn build_bmc_url(proxy_address: &Option<HostPortPair>, bmc_address: SocketAddr) -> Url { | ||
| let mut url = Url::parse("https://placeholder.invalid").expect("static base URL is valid"); |
There was a problem hiding this comment.
It is better to return Result<Url,...> here to avoid panicking
| } | ||
|
|
||
| /// Sets an IP host, letting the `url` crate bracket IPv6 literals. | ||
| fn set_ip_host(url: &mut Url, ip: IpAddr) { |
There was a problem hiding this comment.
This helper is trivial, so it is better just inline it. Also, please avoid using expect in production code.
| Ok(ip) => set_ip_host(url, ip), | ||
| Err(_) => url | ||
| .set_host(Some(host)) | ||
| .expect("proxy host is expected to be a valid URL host"), |
There was a problem hiding this comment.
Same here. Please avoid using expect.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
3 participants
NvRedfishClientPool::create_bmcbuilds the BMC base URL with bareformat!s and then.expect()sUrl::parse. IPv6 hosts were left unbracketed, so an IPv6 BMC produced an invalid authority such ashttps://2001:db8::1:8443— whichUrl::parserejects, panicking the caller.The codebase already brackets IPv6 on the sibling path (
crates/health/src/endpoint/model.rs::BmcAddr::to_url); thenv_redfishpool was missing the same handling.This extracts a small, testable
build_bmc_urlhelper that brackets IPv6 literals in every arm:IpAddr(the panicking case),SocketAddr'sDisplayalready brackets.Hostnames, IPv4 literals, and already-bracketed hosts are passed through untouched.
Related issues
Part of the #2237 IPv6 bug bucket; addresses the Redfish-bracket item of #2406. (Scoped to just this fix, so it does not close the multi-item #2406.)
Type of Change
Breaking Changes
Testing
cargo test -p carbide-redfish --features test-support nv_redfish::tests— 5 passing, covering each arm. The newport_only_proxy_brackets_ipv6_bmctest fails before this change (produceshttps://2001:db8::1:8443, asserted against the bracketedhttps://[2001:db8::1]:8443).cargo clippy -- -D warningsandcargo fmt --checkboth clean.Additional Notes
The
libredfishproxy path (crates/redfish/src/libredfish/implementation.rs) constructs its endpoint inside the externallibredfishcrate and is out of scope here.