Fix connectivity check for nodes named with IP address - #3083
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 |
WalkthroughConnectivity check generation now uses a shared helper to normalize node names for source and target labels. IP addresses replace dots and colons with hyphens, while non-IP names retain the segment before the first dot. ChangesConnectivity check label normalization
Estimated code review effort: 2 (Simple) | ~10 minutes 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bradbehle The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
| for _, template := range templates { | ||
| check := copySpecFields(template) | ||
| WithSource("network-check-source-" + strings.Split(pod.Spec.NodeName, ".")[0])(check) | ||
| WithSource("network-check-source-" + nodeNameForLabel(pod.Spec.NodeName))(check) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Ensure normalized node identifiers remain collision-free.
192.168.1.1 and 192-168-1-1 both normalize to 192-168-1-1. If both nodes exist, the source/target values used at Lines 179 and 380 can identify different nodes identically, causing generated PodNetworkConnectivityCheck resources to collide or overwrite one another. Use an encoding that preserves separator distinctions or explicitly detect and resolve collisions.
Also applies to: 380-380, 441-450
🤖 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` at 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.
| // 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
1 participant
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.