OCPBUGS-63543: fix: update redfish url validation

[eggfoobar]

update validation for redfish url to give better error message when port number is not provided
changed from using regex to using url.Parse to avoid any unforseen cases and support ipv6
added unit tests to cover changes

[coderabbitai]

Walkthrough

The changes refactor address parsing in fencing configuration from regex-based extraction to URL parsing via net/url. Default ports (443 for HTTPS, 80 for HTTP) are inferred when absent, with explicit error handling if port determination fails. Test coverage is expanded to validate default port inference and IPv6 address handling.

Changes

Cohort / File(s)	Summary
Fencing implementation pkg/tnf/pkg/pcs/fencing.go	Replaces regex-based address parsing with URL parsing; extracts hostname, path, and port; infers default ports (443/https, 80/http); updates fencing device options (Ip, IpPort, SystemsUri) to use parsed values; removes unused addressRegEx.
Fencing test coverage pkg/tnf/pkg/pcs/fencing_test.go	Adds test cases for Redfish configurations without explicit ports (HTTPS/HTTP), including IPv6 addresses; validates default port inference and IPv6 preservation; adds negative test for missing scheme/port.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Port inference logic must be validated for correctness (443 for HTTPS, 80 for HTTP)
• IPv6 address handling in URL parsing and field assignment requires verification
• Error handling path for indeterminate ports needs review
• Test coverage completeness for edge cases (malformed URLs, unusual port scenarios)

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	The description is directly related to the changeset and describes the key aspects of the changes. It explains the motivation (better error messages when port is missing), the implementation approach (switching from regex to url.Parse), the benefits (avoiding unforeseen cases and supporting IPv6), and confirms that unit tests were added. While concise, it conveys meaningful information about what was changed and why, meeting the lenient criteria for this check.
Title Check	✅ Passed	The title "OCPBUGS-63543: fix: update redfish url validation" directly aligns with the primary changes in this pull request. The changeset replaces regex-based Redfish address parsing with URL parsing via net/url, adds default port inference (443 for HTTPS, 80 for HTTP), improves error handling, and adds support for IPv6 addresses. The title accurately captures that this is an update to how Redfish URL validation is performed. The title is concise, specific, and clear enough that a developer scanning the history would understand this addresses Redfish URL handling improvements.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

pkg/tnf/pkg/pcs/fencing.go (1)

129-149: Solid refactor to URL parsing with proper default port inference.

The migration from regex to url.Parse improves robustness and adds native IPv6 support. The default port inference correctly distinguishes between HTTPS (443) and HTTP (80) schemes, and appropriately errors when the port cannot be determined.

For slightly more robust scheme matching, consider using strings.HasSuffix instead of strings.Contains:

 if redfishPort == "" {
     switch {
-    case strings.Contains(parsedUrl.Scheme, "https"):
+    case strings.HasSuffix(parsedUrl.Scheme, "https"):
         redfishPort = "443"
-    case strings.Contains(parsedUrl.Scheme, "http"):
+    case strings.HasSuffix(parsedUrl.Scheme, "http"):
         redfishPort = "80"
     default:

This prevents potential false matches with schemes like "myhttps" or "httpspecial", though given the Redfish context this is unlikely to occur.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between f93a306 and 219c11f.

📒 Files selected for processing (2)

• pkg/tnf/pkg/pcs/fencing.go (3 hunks)
• pkg/tnf/pkg/pcs/fencing_test.go (2 hunks)

🔇 Additional comments (5)

pkg/tnf/pkg/pcs/fencing_test.go (3)

68-117: Excellent test coverage for default port inference.

These test cases properly validate the default port behavior for HTTPS (443) and HTTP (80) when no explicit port is provided in the Redfish URL.

---

118-142: Strong IPv6 test coverage.

This test case properly validates IPv6 address handling, ensuring the brackets are correctly stripped from the hostname while maintaining the full IPv6 address in the Ip field.

---

180-191: Good negative test case.

This test appropriately validates that addresses without a proper scheme (missing "redfish+http" or "redfish+https" prefix) produce an error, as the port cannot be inferred.

pkg/tnf/pkg/pcs/fencing.go (2)

6-6: Appropriate import addition.

The net/url import is correctly added to support URL parsing functionality.

---

174-176: Correct mapping of parsed URL components.

The extracted URL components (hostname, port, path) are properly mapped to the fencing device options. The use of Hostname() correctly strips IPv6 brackets for the Ip field.

[slintes]

Nice 👍🏼 Are we fine with not looking for "redfish" substring in the path anymore?

[eggfoobar]

Nice 👍🏼 Are we fine with not looking for "redfish" substring in the path anymore?

So that part still happens just above this line of code, https://github.com/openshift/cluster-etcd-operator/pull/1504/files#diff-026cdeecf29c80e2bda9316cc9f2d8c1433a3a05af902af1b20c2d13154dab54R122 so if we want to check for redfish that will trigger the failure there, otherwise here we just do the parsing. WDYT?

[slintes]

Nice 👍🏼 Are we fine with not looking for "redfish" substring in the path anymore?

So that part still happens just above this line of code, https://github.com/openshift/cluster-etcd-operator/pull/1504/files#diff-026cdeecf29c80e2bda9316cc9f2d8c1433a3a05af902af1b20c2d13154dab54R122 so if we want to check for redfish that will trigger the failure there, otherwise here we just do the parsing. WDYT?

oh sorry, I missed that check outside of the diff. So it was duplicate before (once in that mentioned check, once in the regex) 😁

/retest

[eggfoobar]

Oh no worries, as long as we don't care to enforce the schema pattern then we're good, personally I think we shouldn't since most people won't think to include it and we only pass along the IP address.

/retest-required

[openshift-ci]

@eggfoobar: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[openshift-ci-robot]

@eggfoobar: This pull request references Jira Issue OCPBUGS-63543, which is invalid:

• expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

update validation for redfish url to give better error message when port number is not provided
changed from using regex to using url.Parse to avoid any unforseen cases and support ipv6
added unit tests to cover changes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[eggfoobar]

/jira refresh

[openshift-ci-robot]

@eggfoobar: This pull request references Jira Issue OCPBUGS-63543, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.21.0) matches configured target version for branch (4.21.0)
• bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (dhensel@redhat.com), skipping review request.

Details

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[tjungblu]

/approve

[fonta-rh]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eggfoobar, fonta-rh, tjungblu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• pkg/tnf/OWNERS [tjungblu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[eggfoobar]

/verified by unit tests

[openshift-ci-robot]

@eggfoobar: This PR has been marked as verified by unit tests.

Details

In response to this:

/verified by unit tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci-robot]

@eggfoobar: Jira Issue Verification Checks: Jira Issue OCPBUGS-63543
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-63543 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

Details

In response to this:

update validation for redfish url to give better error message when port number is not provided
changed from using regex to using url.Parse to avoid any unforseen cases and support ipv6
added unit tests to cover changes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[eggfoobar]

/cherry-pick release-4.20

[openshift-cherrypick-robot]

@eggfoobar: new pull request created: #1506

Details

In response to this:

/cherry-pick release-4.20

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[openshift-merge-robot]

Fix included in accepted release 4.21.0-0.nightly-2025-10-30-060549