fix(nvswitch-redfish): bracket IPv6 BMC hosts in Redfish endpoint URLs by CTOmari360 · Pull Request #3013 · NVIDIA/infra-controller

CTOmari360 · 2026-06-30T06:09:28Z

Builds the gofish Redfish endpoint URL for NV-Switch BMCs with IPv6-safe host bracketing.

RedfishClient.New built the endpoint with a bare fmt.Sprintf("https://%s:%d", b.IP.String(), port). Because b.IP is a net.IP, an IPv6 BMC's String() is unbracketed, yielding an invalid URL authority:

custom port → https://2001:db8::1:8443
default port → https://2001:db8::1

gofish (and any URL parser) cannot resolve these, so the client cannot connect to IPv6 NV-Switch BMCs.

The endpoint logic is extracted into a pure buildEndpoint(ip net.IP, port int) string helper that brackets IPv6 literals — net.JoinHostPort for the port case (which also leaves IPv4/hostnames untouched), and an explicit bracket for the no-port (default 443) case. IPv4 and hostname endpoints are unchanged; the magic 443 is replaced with the existing bmc.DefaultBMCPort constant.

This is the same IPv6-URL-bracketing bug class previously fixed in the nv-redfish client (#3008) and bmc-proxy (#3010), here in the distinct rest-api/nvswitch-manager component.

Related issues

Part of #2237 (IPv6 Bug Scan Bucket).

Type of Change

Fix - Bug fixes

Breaking Changes

This PR contains breaking changes

Testing

Unit tests added/updated

Added a table-driven TestBuildEndpoint covering IPv4/IPv6 × default/custom port. The two IPv6 cases fail before the fix (https://2001:db8::1 / https://2001:db8::1:8443) and pass after. go build ./nvswitch-manager/..., go vet, and gofmt -l are clean.

Additional Notes

The endpoint string was previously inline inside New() (which performs network I/O via gofish.ConnectContext); extracting the pure helper makes the URL construction unit-testable without a live BMC.

RedfishClient.New built the gofish endpoint with a bare fmt.Sprintf("https://%s:%d", b.IP.String(), port). For an IPv6 BMC the unbracketed host yields an invalid URL authority -- e.g. https://2001:db8::1:8443 (custom port) or https://2001:db8::1 (default port) -- so gofish cannot connect to IPv6 NV-Switch BMCs. Extract a pure buildEndpoint helper that brackets IPv6 literals: net.JoinHostPort for the port case (which also leaves IPv4/hostnames untouched), and an explicit bracket for the no-port (default 443) case. IPv4 and hostname endpoints are unchanged. Also use the bmc.DefaultBMCPort constant in place of the magic 443. Add a table-driven TestBuildEndpoint; the two IPv6 cases fail pre-fix. Part of NVIDIA#2237 (IPv6 Bug Scan Bucket). Signed-off-by: Omar Refai <omar@refai.org>

copy-pr-bot · 2026-06-30T06:09:31Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

coderabbitai · 2026-06-30T06:09:50Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: fe6a8d72-327b-4afc-9e40-476779ba1a35

📥 Commits

Reviewing files that changed from the base of the PR and between 046a5b7 and 05f1817.

📒 Files selected for processing (2)

rest-api/nvswitch-manager/pkg/redfish/client.go
rest-api/nvswitch-manager/pkg/redfish/client_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

rest-api/nvswitch-manager/pkg/redfish/client_test.go
rest-api/nvswitch-manager/pkg/redfish/client.go

Summary by CodeRabbit

Bug Fixes
- Improved Redfish endpoint URL handling so IPv6 addresses format correctly, including when using custom ports.
- Made endpoint generation consistent across default and non-default port cases.
Tests
- Added unit tests for endpoint formatting with IPv4 and IPv6.
- Verified URL output for both default (443) and custom ports.

Walkthrough

The Redfish client now builds its HTTPS endpoint through an unexported buildEndpoint(ip net.IP, port int) string helper. New delegates to it, and a table-driven test covers IPv4 and IPv6 with default and custom ports.

Changes

IPv6-safe Redfish Endpoint Builder

Layer / File(s)	Summary
buildEndpoint helper and New wiring `rest-api/nvswitch-manager/pkg/redfish/client.go`	Adds networking and URL construction imports, introduces `buildEndpoint(ip net.IP, port int) string`, and updates `New` to call it.
Table-driven tests for buildEndpoint `rest-api/nvswitch-manager/pkg/redfish/client_test.go`	Adds `TestBuildEndpoint` with IPv4 and IPv6 cases for ports `443` and `8443`, asserting exact `https://` URL strings.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly states the core fix: IPv6-safe bracketing for Redfish BMC endpoint URLs.
Description check	✅ Passed	The description matches the changeset and explains the IPv6 endpoint fix, helper extraction, and tests.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

🧹 Nitpick comments (1)

rest-api/nvswitch-manager/pkg/redfish/client_test.go (1)
6-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use testify assertions here. Replace the t.Fatalf/t.Errorf checks in TestBuildEndpoint with require/assert to match the rest of rest-api tests and keep failures 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 `@rest-api/nvswitch-manager/pkg/redfish/client_test.go` around lines 6 - 9,
`TestBuildEndpoint` is using raw `t.Fatalf`/`t.Errorf` checks instead of the
test helpers used elsewhere in rest-api. Update the assertions in
`TestBuildEndpoint` to use testify’s `require` and `assert` packages, keeping
the existing test logic intact while making failures consistent; this change
should be localized in the test function that exercises `BuildEndpoint`.
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 `@rest-api/nvswitch-manager/pkg/redfish/client_test.go`:
- Around line 6-9: `TestBuildEndpoint` is using raw `t.Fatalf`/`t.Errorf` checks
instead of the test helpers used elsewhere in rest-api. Update the assertions in
`TestBuildEndpoint` to use testify’s `require` and `assert` packages, keeping
the existing test logic intact while making failures consistent; this change
should be localized in the test function that exercises `BuildEndpoint`.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cc4c7c5b-cbed-4ac1-992e-96635182e3f7

📥 Commits

Reviewing files that changed from the base of the PR and between 18af75a and 046a5b7.

📒 Files selected for processing (2)

rest-api/nvswitch-manager/pkg/redfish/client.go
rest-api/nvswitch-manager/pkg/redfish/client_test.go

ajf · 2026-06-30T17:33:00Z

/ok to test 046a5b7

ajf · 2026-06-30T17:33:17Z

@CTOmari360 isn't there a URI go library or something that'll do this for us?

github-actions · 2026-06-30T17:34:25Z

🔍 Container Scan Summary

Service	Total	Critical	High	Medium	Low	Other
nico-flow	43	3	21	9	2	8
nico-nsm	49	0	10	31	8	0
nico-psm	43	3	21	9	2	8
nico-rest-api	43	3	21	9	2	8
nico-rest-cert-manager	42	3	21	9	1	8
nico-rest-db	43	3	21	9	2	8
nico-rest-site-agent	42	3	21	9	1	8
nico-rest-site-manager	42	3	21	9	1	8
nico-rest-workflow	43	3	21	9	2	8
TOTAL	390	24	178	103	21	64

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

github-actions · 2026-06-30T17:39:14Z

🔐 TruffleHog Secret Scan

✅ No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

_{🕐 Last updated: 2026-06-30 17:39:13 UTC | Commit: 046a5b7}

…rl.URL Per review (ajf): collapse the endpoint construction to the canonical stdlib helpers instead of hand-bracketing. net.JoinHostPort brackets IPv6 literals (and leaves IPv4/hostnames untouched); url.URL assembles the string. Drops the manual strings.Contains bracketing and the default-port special case. Default-port endpoints are now explicit (https://host:443), which is equivalent to the bare-host form for HTTPS. Test expectations updated accordingly. Part of NVIDIA#2237 (IPv6 Bug Scan Bucket). Signed-off-by: Omar Refai <omar@refai.org>

CTOmari360 · 2026-06-30T21:57:56Z

Yep — the stdlib helper is net.JoinHostPort, which brackets IPv6 hosts correctly. I was already using it on the custom-port path; the only hand-rolled bracketing was the default-port branch. net/url on its own doesn't help here — it trusts URL.Host to already be well-formed and won't bracket a bare IPv6 host for you.

Simplified it to lean entirely on the stdlib — just pushed:

func buildEndpoint(ip net.IP, port int) string {
	return (&url.URL{
		Scheme: "https",
		Host:   net.JoinHostPort(ip.String(), strconv.Itoa(port)),
	}).String()
}

Only visible change is that default-port endpoints are now explicit (https://[2001:db8::1]:443), which is equivalent to the bare-host form for HTTPS. Thanks for the nudge.

CTOmari360 requested a review from a team as a code owner June 30, 2026 06:09

coderabbitai Bot reviewed Jun 30, 2026

View reviewed changes

ajf approved these changes Jul 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(nvswitch-redfish): bracket IPv6 BMC hosts in Redfish endpoint URLs#3013

fix(nvswitch-redfish): bracket IPv6 BMC hosts in Redfish endpoint URLs#3013
CTOmari360 wants to merge 2 commits into
NVIDIA:mainfrom
CTOmari360:fix/nvswitch-redfish-ipv6-endpoint

CTOmari360 commented Jun 30, 2026

Uh oh!

copy-pr-bot Bot commented Jun 30, 2026

Uh oh!

coderabbitai Bot commented Jun 30, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

ajf commented Jun 30, 2026

Uh oh!

ajf commented Jun 30, 2026

Uh oh!

github-actions Bot commented Jun 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jun 30, 2026

Uh oh!

CTOmari360 commented Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

CTOmari360 commented Jun 30, 2026

Related issues

Type of Change

Breaking Changes

Testing

Additional Notes

Uh oh!

copy-pr-bot Bot commented Jun 30, 2026

Uh oh!

coderabbitai Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ajf commented Jun 30, 2026

Uh oh!

ajf commented Jun 30, 2026

Uh oh!

github-actions Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Container Scan Summary

Uh oh!

github-actions Bot commented Jun 30, 2026

🔐 TruffleHog Secret Scan

Uh oh!

CTOmari360 commented Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jun 30, 2026 •

edited

Loading

github-actions Bot commented Jun 30, 2026 •

edited

Loading