OCPBUGS-81761: ClusterHostedDNS: Don't update Infrastrcture CR when order of Ingress LB IPs changes#1417
OCPBUGS-81761: ClusterHostedDNS: Don't update Infrastrcture CR when order of Ingress LB IPs changes#1417sadasu wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@sadasu: This pull request references Jira Issue OCPBUGS-81761, 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ingress controller was modified to add a watch on the 'machine-config' ClusterOperator object to trigger ingress controller reconciliations. A new helper function was introduced to sort net.IP slices using bytes.Compare and convert them to configv1.IP. The computeUpdatedInfraFromService method was updated for ClusterHosted platforms: AWS now accumulates all resolved IPs before sorting and converting, while Azure and GCP parse IP strings with validation before applying the sort-and-convert operation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
openshift-ci
Bot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
sadasu
force-pushed
the
ingress-lb-ip-list
branch
from
32096fe to
7c8756c
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@sadasu: This pull request references Jira Issue OCPBUGS-81761, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
![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 the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/controller/ingress/controller.go`:
- Around line 1481-1486: The loop building ips should skip nil results from
net.ParseIP to avoid later panics in sortAndConvertIPs; update the loop in
controller.go that calls net.ParseIP(ingress.IP) to check the returned value is
non-nil before appending to ips (optionally log or record invalid ingress.IP
values for debugging), so only valid net.IP entries are passed into
sortAndConvertIPs.
- Around line 1500-1505: The loop building ips from ingresses uses
net.ParseIP(ingress.IP) but doesn't check for a nil return; update the loop in
the ingress processing code (the block that populates ips from ingresses) to
assign the result of net.ParseIP to a variable (e.g., parsed :=
net.ParseIP(ingress.IP)) and only append parsed to ips when parsed != nil so
invalid IP strings are skipped, mirroring the Azure-path defensive check.
🪄 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: Pro Plus
Run ID: b3d5c05c-2436-4e13-9550-d6af06e74cef
📒 Files selected for processing (2)
pkg/operator/controller/ingress/controller.gopkg/operator/controller/ingress/controller_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/operator/controller/ingress/controller.go (2)
1481-1487:⚠️ Potential issue | 🟡 MinorAdd nil check after
net.ParseIPto prevent invalid data.
net.ParseIPreturnsnilfor invalid IP strings. Appending nil to the slice will causesortAndConvertIPsto convert it to the string"<nil>", corrupting the Infrastructure CR data.🛡️ Proposed defensive fix
ips := []net.IP{} for _, ingress := range ingresses { if len(ingress.IP) > 0 { - ips = append(ips, net.ParseIP(ingress.IP)) + if ip := net.ParseIP(ingress.IP); ip != nil { + ips = append(ips, ip) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/ingress/controller.go` around lines 1481 - 1487, The loop collects IPs with net.ParseIP but doesn't check for nil, which lets invalid IPs become "<nil>" via sortAndConvertIPs; update the loop that builds ips (inside the ingresses iteration) to call net.ParseIP(ingress.IP), skip (do not append) when the result is nil, and optionally log or record a warning for the malformed ingress.IP value so ingressLBIPs is only derived from valid net.IP values before calling sortAndConvertIPs.
1500-1506:⚠️ Potential issue | 🟡 MinorAdd nil check after
net.ParseIPfor consistency with Azure fix.Same issue as the Azure path -
net.ParseIPcan returnnilfor invalid input, which would result in"<nil>"being stored in the Infrastructure CR.🛡️ Proposed defensive fix
ips := []net.IP{} for _, ingress := range ingresses { if len(ingress.IP) > 0 { - ips = append(ips, net.ParseIP(ingress.IP)) + if ip := net.ParseIP(ingress.IP); ip != nil { + ips = append(ips, ip) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/ingress/controller.go` around lines 1500 - 1506, The loop that builds ips calls net.ParseIP(ingress.IP) without checking for nil, so invalid IPs produce nil entries and later "<nil>" in the Infrastructure CR; update the loop that builds ips (iterating over ingresses and appending to ips) to check the result of net.ParseIP and only append non-nil IPs (skip or log invalid ingress.IP), ensuring sortAndConvertIPs receives only valid net.IP values and ingressLBIPs is thus correct.
🧹 Nitpick comments (2)
pkg/operator/controller/ingress/controller.go (2)
1427-1437: Consider filtering nil IPs to prevent invalid data.If a nil
net.IPis passed in the slice (e.g., fromnet.ParseIPreturning nil),ip.String()returns the string"<nil>", which would be stored as an invalid IP in the Infrastructure CR. While this won't panic, it will corrupt the data.Since the Azure and GCP paths use
net.ParseIPwhich can return nil, consider adding defensive filtering here or in the callers.♻️ Option A: Filter within this function
func sortAndConvertIPs(ips []net.IP) []configv1.IP { slices.SortFunc(ips, func(a, b net.IP) int { return bytes.Compare(a, b) }) result := make([]configv1.IP, 0, len(ips)) for _, ip := range ips { + if ip == nil { + continue + } result = append(result, configv1.IP(ip.String())) } return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/ingress/controller.go` around lines 1427 - 1437, The sortAndConvertIPs function should defensively skip nil net.IP entries to avoid storing "<nil>" into configv1.IP; update sortAndConvertIPs to first filter out nils (or skip nils during conversion) before calling slices.SortFunc and while building the result slice so only valid ip.String() values are appended; reference the function sortAndConvertIPs, the net.IP slice parameter, the comparator using bytes.Compare, and the conversion to configv1.IP(ip.String()) when implementing the nil-check and filtering.
1513-1517: Remove unreachable code.Line 1516 is unreachable because all switch cases (including
default) return before this point.🧹 Proposed fix
default: return false, nil } - return false, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/ingress/controller.go` around lines 1513 - 1517, The trailing "return false, nil" after the switch is unreachable because every switch branch (including default) already returns; remove the redundant return statement to eliminate dead code in the function (in controller.go around the switch in the ingress controller function), leaving only the existing returns inside each case/default so the function's control flow remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/operator/controller/ingress/controller.go`:
- Around line 1481-1487: The loop collects IPs with net.ParseIP but doesn't
check for nil, which lets invalid IPs become "<nil>" via sortAndConvertIPs;
update the loop that builds ips (inside the ingresses iteration) to call
net.ParseIP(ingress.IP), skip (do not append) when the result is nil, and
optionally log or record a warning for the malformed ingress.IP value so
ingressLBIPs is only derived from valid net.IP values before calling
sortAndConvertIPs.
- Around line 1500-1506: The loop that builds ips calls net.ParseIP(ingress.IP)
without checking for nil, so invalid IPs produce nil entries and later "<nil>"
in the Infrastructure CR; update the loop that builds ips (iterating over
ingresses and appending to ips) to check the result of net.ParseIP and only
append non-nil IPs (skip or log invalid ingress.IP), ensuring sortAndConvertIPs
receives only valid net.IP values and ingressLBIPs is thus correct.
---
Nitpick comments:
In `@pkg/operator/controller/ingress/controller.go`:
- Around line 1427-1437: The sortAndConvertIPs function should defensively skip
nil net.IP entries to avoid storing "<nil>" into configv1.IP; update
sortAndConvertIPs to first filter out nils (or skip nils during conversion)
before calling slices.SortFunc and while building the result slice so only valid
ip.String() values are appended; reference the function sortAndConvertIPs, the
net.IP slice parameter, the comparator using bytes.Compare, and the conversion
to configv1.IP(ip.String()) when implementing the nil-check and filtering.
- Around line 1513-1517: The trailing "return false, nil" after the switch is
unreachable because every switch branch (including default) already returns;
remove the redundant return statement to eliminate dead code in the function (in
controller.go around the switch in the ingress controller function), leaving
only the existing returns inside each case/default so the function's control
flow remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ed308e52-a902-4f09-aee8-718a4698a071
📒 Files selected for processing (2)
pkg/operator/controller/ingress/controller.gopkg/operator/controller/ingress/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/controller/ingress/controller_test.go
sadasu
force-pushed
the
ingress-lb-ip-list
branch
from
7c8756c to
6bba135
Compare
openshift-ci
Bot
removed
the
needs-rebase
sadasu
force-pushed
the
ingress-lb-ip-list
branch
from
6bba135 to
4651bb9
Compare
| { | ||
| description: "gcp platform with multiple IPs in different order should not update", | ||
| platform: &gcpPlatformWithMultipleLBIPs, | ||
| ingresses: ingressesWithMultipleIPsReversed, | ||
| expectedLBIPs: []configv1.IP{configv1.IP("10.10.10.4"), IngressLBIP}, | ||
| expectUpdated: false, | ||
| expectError: false, | ||
| }, |
There was a problem hiding this comment.
Do you have an equivalent test case with ingresses: ingressesWithMultipleIPs? Something like this:
{
description: "gcp platform with multiple IPs already in status should not update",
platform: &gcpPlatformWithMultipleLBIPs,
ingresses: ingressesWithMultipleIPs,
expectedLBIPs: []configv1.IP{configv1.IP("10.10.10.4"), IngressLBIP},
expectUpdated: false,
expectError: false,
},Similarly for Azure:
{
description: "azure platform with multiple IPs already in status should not update",
platform: &azurePlatformWithMultipleLBIPs,
ingresses: ingressesWithMultipleIPs,
expectedLBIPs: []configv1.IP{configv1.IP("10.10.10.4"), IngressLBIP},
expectUpdated: false,
expectError: false,
},Testing the AWS logic is a little more complicated as it uses net.LookupIP. What do you think of these changes?
diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go
index f8b2f9be8..8abd3becf 100644
--- a/pkg/operator/controller/ingress/controller.go
+++ b/pkg/operator/controller/ingress/controller.go
@@ -1507,6 +1507,9 @@ func sortAndConvertIPs(ips []net.IP) []configv1.IP {
return result
}
+// lookupIP is an alias for net.LookupIP that can be overridden in unit tests.
+var lookupIP = net.LookupIP
+
// computeUpdatedInfraFromService updates PlatformStatus for AWS, Azure and GCP with Ingress LB IPs when the DNSType is `ClusterHosted`.
func computeUpdatedInfraFromService(service *corev1.Service, infraConfig *configv1.Infrastructure) (bool, error) {
platformStatus := infraConfig.Status.PlatformStatus
@@ -1533,7 +1536,7 @@ func computeUpdatedInfraFromService(service *corev1.Service, infraConfig *config
latestIngressIPs := []net.IP{}
for _, ingress := range ingresses {
// Resolving the LoadBalancer's IPs is not ideal because they may change, but currently there is no better alternative.
- ingressIPs, err := net.LookupIP(ingress.Hostname)
+ ingressIPs, err := lookupIP(ingress.Hostname)
if err != nil {
return false, fmt.Errorf("failed to lookup IP addresses corresponding to AWS LB hostname: %w", err)
}
diff --git a/pkg/operator/controller/ingress/controller_test.go b/pkg/operator/controller/ingress/controller_test.go
index 878f04d53..aaad47bb7 100644
--- a/pkg/operator/controller/ingress/controller_test.go
+++ b/pkg/operator/controller/ingress/controller_test.go
@@ -1,6 +1,7 @@
package ingress
import (
+ "net"
"reflect"
"testing"
"time"
@@ -1799,26 +1800,22 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
awsPlatform = configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
}
- awsPlatformWithDNSType = configv1.PlatformStatus{
- Type: configv1.AWSPlatformType,
- AWS: &configv1.AWSPlatformStatus{
- CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
- DNSType: configv1.ClusterHostedDNSType,
- },
- },
- }
- awsPlatformWithLBIP = configv1.PlatformStatus{
- Type: configv1.AWSPlatformType,
- AWS: &configv1.AWSPlatformStatus{
- CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
- DNSType: configv1.ClusterHostedDNSType,
- ClusterHosted: &configv1.CloudLoadBalancerIPs{
- IngressLoadBalancerIPs: []configv1.IP{
- IngressLBIP,
+ awsPlatformWithLBIP = func(as ...string) *configv1.PlatformStatus {
+ addrs := []configv1.IP{}
+ for _, addr := range as {
+ addrs = append(addrs, configv1.IP(addr))
+ }
+ return &configv1.PlatformStatus{
+ Type: configv1.AWSPlatformType,
+ AWS: &configv1.AWSPlatformStatus{
+ CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
+ DNSType: configv1.ClusterHostedDNSType,
+ ClusterHosted: &configv1.CloudLoadBalancerIPs{
+ IngressLoadBalancerIPs: addrs,
},
},
},
- },
+ }
}
azurePlatform = configv1.PlatformStatus{
Type: configv1.AzurePlatformType,
@@ -1907,14 +1904,31 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
{IP: "10.10.10.4"},
{IP: "196.78.125.4"},
}
- // Hostname is intentionally assigned an IP address for unit testing purposes since net.LookupIP simply returns the provided IP.
- awsIngresses = []corev1.LoadBalancerIngress{
- {Hostname: "196.78.125.4"},
- }
- awsUpdatedIngresses = []corev1.LoadBalancerIngress{
- {Hostname: "10.10.10.4"},
- }
)
+
+ // On AWS, the service status has a host name rather than IP addresses,
+ // and so testing AWS requires temporarily overriding net.LookupIP with
+ // a mock.
+ defer func() { lookupIP = net.LookupIP }()
+ var (
+ addr1 = net.ParseIP("196.78.125.4")
+ addr2 = net.ParseIP("10.10.10.4")
+ addr3 = net.ParseIP("10.10.10.5")
+ )
+ lookupIP = func(host string) ([]net.IP, error) {
+ switch host {
+ case "domain1.tld":
+ return []net.IP{addr1, addr2}, nil
+ case "domain2.tld":
+ return []net.IP{addr2, addr1}, nil
+ case "domain3.tld":
+ return []net.IP{addr3}, nil
+ default:
+ t.Fatal("unexpected domain", host)
+ return []net.IP{}, nil
+ }
+ }
+
testCases := []struct {
description string
platform *configv1.PlatformStatus
@@ -1992,32 +2006,40 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
},
{
description: "aws platform with DNSType and no LB IP",
- platform: &awsPlatformWithDNSType,
+ platform: awsPlatformWithLBIP(),
ingresses: []corev1.LoadBalancerIngress{},
expectUpdated: false,
expectError: false,
},
{
- description: "aws platform with DNSType and service has 1 IP",
- platform: &awsPlatformWithDNSType,
- ingresses: awsIngresses,
- expectedLBIPs: []configv1.IP{IngressLBIP},
+ description: "aws platform with DNSType and service has a hostname",
+ platform: awsPlatformWithLBIP(),
+ ingresses: []corev1.LoadBalancerIngress{{Hostname: "domain1.tld"}},
+ expectedLBIPs: []configv1.IP{configv1.IP("10.10.10.4"), configv1.IP("196.78.125.4")},
expectUpdated: true,
expectError: false,
},
{
description: "aws platform with no change to LB IPs",
- platform: &awsPlatformWithLBIP,
- ingresses: awsIngresses,
- expectedLBIPs: []configv1.IP{IngressLBIP},
+ platform: awsPlatformWithLBIP("196.78.125.4", "10.10.10.4"),
+ ingresses: []corev1.LoadBalancerIngress{{Hostname: "domain1.tld"}},
+ expectedLBIPs: []configv1.IP{configv1.IP("10.10.10.4"), configv1.IP("196.78.125.4")},
+ expectUpdated: false,
+ expectError: false,
+ },
+ {
+ description: "aws platform with LB IP addresses reversed",
+ platform: awsPlatformWithLBIP("196.78.125.4", "10.10.10.4"),
+ ingresses: []corev1.LoadBalancerIngress{{Hostname: "domain2.tld"}},
+ expectedLBIPs: []configv1.IP{configv1.IP("10.10.10.4"), configv1.IP("196.78.125.4")},
expectUpdated: false,
expectError: false,
},
{
description: "aws platform with 1 LB IP with change in hostname",
- platform: &awsPlatformWithLBIP,
- ingresses: awsUpdatedIngresses,
- expectedLBIPs: []configv1.IP{configv1.IP("10.10.10.4")},
+ platform: awsPlatformWithLBIP("196.78.125.4", "10.10.10.4"),
+ ingresses: []corev1.LoadBalancerIngress{{Hostname: "domain3.tld"}},
+ expectedLBIPs: []configv1.IP{configv1.IP("10.10.10.5")},
expectUpdated: true,
expectError: false,
},As an aside, computeUpdatedInfraFromService has a lot of repeated logic, and one advantage of refactoring it to reduce the duplication would be that some of the corresponding test logic might go away.
This will allow the addition of Ingress LB IPs to the Infra PlatformStatus as soon as MCO becomes ready. This is a follow-up based on feedback received on PR 1411.
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-custom-dns-techpreview |
|
@sadasu: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/03649ad0-498c-11f1-9257-8e942a20a697-0 |
|
@sadasu: 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. |
|
/assign @gcs278 |
|
@sadasu Let me know if you are ready for a review on this one. If you aren't - it'd be best to mark it with a WIP. The PR description will need an update - it mentions 2 commits, but I only see 1 with a small change. Was there an IP-ordering fix that was accidentally removed? |
5 participants
This PR contains 2 commits:
On AWS, Azure and GCP platforms, do not update the Ingress LB IPs in the Infra CR's Platform Status when only the order of Ingress Load Balancer IPs have changed.
Fixes: https://redhat.atlassian.net/browse/OCPBUGS-81761
Follow-up to comment on OCPBUGS-81550: AWS, Azure and GCP ClusterHostedDNS: Add Ingress LB IPs to Infra CR after masters up #1411.