OCPBUGS-99460: Fix connectivity check for nodes named with IP address - #3083
OCPBUGS-99460: Fix connectivity check for nodes named with IP address#3083bradbehle wants to merge 2 commits into
Conversation
The code that creates the PodNetworkConnectivityCheck resources to allow connection checks from network-check-source to network-check-target uses only the first part of the node name up to the first '.' This means this function is completely broken for clusters that use the IPv4 IP address as the start of the node name (like Managed OpenShift on IBM Cloud) since only one resource is created (since all the node names start with 10. or 192.) This fix identifies these types of node names and uses the full IP address (replacing the '.' with '-') so that each resource name is unique. Its focus is narrow on purpose, so the behavior is only changed (fixed) for node names that are IP addresses and that are broken currently.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Summary by CodeRabbit
WalkthroughConnectivity check generation now uses ChangesConnectivity check label normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors)
✅ Passed checks (21 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @bradbehle. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pkg/controller/connectivitycheck/connectivity_check_controller.go`:
- Around line 441-452: Add Go regression tests for nodeNameForLabel covering
bare IPv4 and IPv6 normalization, FQDN truncation to the first segment, and
distinct outputs for IP-derived versus hostname-derived identifiers. Place the
cases in the existing connectivity-check controller test suite and use
table-driven assertions; if testing is infeasible, document the justification
under “How to verify it.”
- Line 179: Update node identifier generation used by WithSource, the target
assignment near line 380, and the source/target construction around lines
441-450 so normalization remains collision-free for dotted and hyphenated
addresses such as 192.168.1.1 and 192-168-1-1. Use an encoding that preserves
separator distinctions, or explicitly detect and resolve collisions consistently
across all generated PodNetworkConnectivityCheck resources.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c236ccbf-00a0-4951-8088-87f5e0e8f5bf
📒 Files selected for processing (1)
pkg/controller/connectivitycheck/connectivity_check_controller.go
| // nodeNameForLabel returns a string derived from a node name that is safe to embed | ||
| // in a Kubernetes resource name. For bare IP addresses (IPv4 or IPv6), dots and | ||
| // colons are replaced with dashes so the full address is preserved and collisions | ||
| // between different IPs are avoided. For all other names (e.g. FQDNs), only the | ||
| // segment before the first dot is returned, preserving the existing behaviour. | ||
| func nodeNameForLabel(nodeName string) string { | ||
| if _, err := netip.ParseAddr(nodeName); err == nil { | ||
| return strings.NewReplacer(".", "-", ":", "-").Replace(nodeName) | ||
| } | ||
| return strings.Split(nodeName, ".")[0] | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add regression tests for the normalization contract.
Cover bare IPv4, IPv6, FQDN truncation, and collisions between IP-derived and hostname-derived identifiers. No corresponding Go test change is included here; add one or document the applicable exception.
As per coding guidelines, “When production Go files under pkg/ or cmd/ change, corresponding Go test files must also be added or modified, unless the change is trivial, already covered, infeasible to test, or the justification is documented under How to verify it.”
🤖 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 `@pkg/controller/connectivitycheck/connectivity_check_controller.go` around
lines 441 - 452, Add Go regression tests for nodeNameForLabel covering bare IPv4
and IPv6 normalization, FQDN truncation to the first segment, and distinct
outputs for IP-derived versus hostname-derived identifiers. Place the cases in
the existing connectivity-check controller test suite and use table-driven
assertions; if testing is infeasible, document the justification under “How to
verify it.”
Source: Coding guidelines
There was a problem hiding this comment.
There are currently no unit checks of this code... it would be good to have a test that various configurations result in various PodNetworkConnectivityCheck objects being created, but the code doesn't seem set up well for unit tests right now. But you should at least add a unit test for nodeNameForLabel
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd --type f --extension go . pkg/controller/connectivitycheck | sort
ast-grep outline pkg/controller/connectivitycheck --items all --type function --match 'Test|nodeNameForLabel'Length of output: 571
@danwinship, the package has no existing Go test suite. I will add connectivity_check_controller_test.go with table-driven tests for nodeNameForLabel.
One detail: the current helper can collide with a hostname-derived value. For example, both 192.0.2.10 and 192-0-2-10.example.com produce 192-0-2-10. A test that requires those outputs to differ would require a behavior change for non-IP node names. This PR states that non-IP behavior remains unchanged, so the focused test should not require that distinction.
- Add a new focused unit-test file in
pkg/controller/connectivitycheck/. - Test IPv4 normalization.
- Test IPv6 normalization.
- Test the unchanged FQDN truncation behavior.
- 📌 Create a pull request with these changes
You are interacting with an AI system.
danwinship
changed the title
openshift-ci-robot
added
jira/severity-important
|
@bradbehle: This pull request references Jira Issue OCPBUGS-99460, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@danwinship: This pull request references Jira Issue OCPBUGS-99460, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (core-networking-bot@redhat.com), skipping review request. DetailsIn response to this:
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. |
|
/lgtm |
|
Scheduling required tests: Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bradbehle, danwinship The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@bradbehle: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/retest |
3 participants
The code that creates the PodNetworkConnectivityCheck resources to allow connection checks from network-check-source to network-check-target uses only the first part of the node name up to the first '.' This means this function is completely broken for clusters that use the IPv4 IP address as the start of the node name (like Managed OpenShift on IBM Cloud) since only one resource is created (since all the node names start with 10. or 192.)
This fix identifies these types of node names and uses the full IP address (replacing the '.' with '-') so that each resource name is unique. Its focus is narrow on purpose, so the behavior is only changed (fixed) for node names that are IP addresses and that are broken currently.