fix(api,redfish): format ipv6 bmc addresses at protocol boundaries#3749
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (10)
Summary by CodeRabbit
WalkthroughBMC address resolution is centralized, while Redfish URI, endpoint, browser, and ChangesBMC and Redfish networking
Estimated code review effort: 4 (Complex) | ~45 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/api-core/src/handlers/utils.rs (1)
186-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
check_cases_asyncfor this async Result table.
This matches the repository’s table-test pattern and keeps scenario naming and failure reporting consistent.🤖 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/api-core/src/handlers/utils.rs` around lines 186 - 197, Update resolves_hostname_forms_with_the_expected_port to use the repository’s check_cases_async helper for the async Result table, preserving both hostname scenarios, expected ports, loopback validation, and scenario labels through the helper’s standard failure reporting.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.
Inline comments:
In `@crates/bmc-mock/src/combined_service.rs`:
- Around line 86-95: Update the Forwarded-parameter parsing in the shown
combined-service logic and its corresponding occurrence to recognize the `host`
parameter case-insensitively, while preserving trimming, quote removal,
bracketed IPv6 handling, and existing fallback behavior.
---
Nitpick comments:
In `@crates/api-core/src/handlers/utils.rs`:
- Around line 186-197: Update resolves_hostname_forms_with_the_expected_port to
use the repository’s check_cases_async helper for the async Result table,
preserving both hostname scenarios, expected ports, loopback validation, and
scenario labels through the helper’s standard failure reporting.
🪄 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: a063f4da-c4d5-4b33-ad73-c5169f29f4d3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/api-core/src/handlers/bmc_endpoint_explorer.rscrates/api-core/src/handlers/redfish.rscrates/api-core/src/handlers/site_explorer.rscrates/api-core/src/handlers/utils.rscrates/api-web/src/redfish_browser.rscrates/bmc-mock/src/combined_service.rscrates/health/src/bmc.rscrates/redfish/Cargo.tomlcrates/redfish/src/libredfish/implementation.rscrates/utils/src/redfish.rs
chet
changed the title
poroh
approved these changes
Jul 21, 2026
A BMC address stays bare while NICo stores or resolves it, but URI authorities and RFC 7239 `Forwarded` values need IPv6-specific brackets and quoting. Several paths were carrying the bare form across those serialization boundaries. Centralize resolution and boundary formatting for endpoint discovery, the admin browser, Redfish actions, `libredfish`, and the health proxy. IPv4, hostname, and resolver-error behavior stay unchanged; the mock proxy covers quoted IPv6 and case-insensitive `host` parameters. `nv_redfish` URL construction remains in NVIDIA#3008 and `HostPortPair` parsing remains in NVIDIA#3012. This supports NVIDIA#2240 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
enabled auto-merge (squash)
July 21, 2026 17:48
nick-vetro
approved these changes
Jul 21, 2026
3 tasks
CTOmari360
added a commit
to CTOmari360/infra-controller
that referenced
this pull request
Jul 21, 2026
… header NvRedfishClientPool::create_bmc built the BMC base URL and the Forwarded `host` parameter from a bare IpAddr, leaving IPv6 literals unbracketed: the URL (https://2001:db8::1:8443) fails Url::parse, and the header (host=2001:db8::1) violates RFC 7239. Build the URL with the url crate (which brackets IPv6) via a build_bmc_url helper returning Result<Url, _>, propagated as BmcError::InvalidRequest -- no expect on the production path -- mirroring health::BmcAddr::to_url(). IPv6-vs-hostname classification for proxy overrides lives on HostPortPair::url_host(). For the Forwarded header, use the shared carbide_utils::redfish::format_forwarded_host_parameter helper (NVIDIA#3749). Adds unit coverage for URL building (IPv4/IPv6, proxy/no-proxy) and the Forwarded host parameter. Part of NVIDIA#2237; addresses the Redfish-bracket item of NVIDIA#2406. Signed-off-by: Omar Refai <omar@refai.org>
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
3 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.
A BMC IP needs three representations as it crosses NICo: a bare value for storage and resolution, a bracketed URI authority, and a quoted RFC 7239
Forwardedhost parameter.Several paths carried the bare address one boundary too far. Endpoint exploration treated IPv6 colons like an explicit port and skipped the default
:443; the admin browser, Redfish actions, andlibredfishcould build ambiguoushost:portauthorities; and proxy paths serialized invalid RFC 7239 values.This change keeps the address bare for storage and metadata lookup, then formats it at the protocol boundary that needs a URI authority or header value.
resolve_bmc_addresshandles hostnames, IPv4, bare or bracketed IPv6, and explicit or default ports through one path, while shared helpers cover URI host parsing andForwardedserialization for the API, browser,libredfish, and health proxy.IPv4 and hostname behavior remains unchanged, including resolver error mapping. The mock proxy also accepts quoted IPv6 values and case-insensitive
hostparameter names so its routing matches the real proxy's RFC 7239 handling.Related issues
This supports #2240
Related work:
nv_redfishURL construction.bmc-proxyupstream URI authority.HostPortPair.Type of Change
Breaking Changes
Testing
Focused coverage includes bare and bracketed IPv6, explicit and default ports, browser and
libredfishURI construction, RFC 7239 header formatting, case-insensitive mock routing, and the existing IPv4 and hostname paths.Local verification passes
cargo make format-nightly,cargo make clippy,cargo make carbide-lints,cargo make check-workspace-deps, and the affected Rust tests.Additional Notes
BmcAccessInfo.hostintentionally remains bare because brackets belong to a URI authority, not the stored IP value.crates/redfish/src/nv_redfish/mod.rsremains in #3008, thebmc-proxyupstream authority remains in #3010, andHostPortPairremains in #3012. This PR supports #2240 without closing it while those pieces remain open.Closes #2240