Skip to content

OCPBUGS-81192: Fix race condition in internal-to-external LB migration test #1407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Apr 22, 2026
Merged

OCPBUGS-81192: Fix race condition in internal-to-external LB migration test #1407

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions test/e2e/unmanaged_dns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ var operatorProgressingFalse = operatorv1.OperatorCondition{
Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionFalse,
}

// TestUnmanagedDNSToManagedDNSIngressController tests dnsManagementPolicy during
// transitioning from Unmanaged to Managed DNS on an external ingress controller.
// The load balancer scope remains external throughout the transition, so only the
// DNS management policy changes.
func TestUnmanagedDNSToManagedDNSIngressController(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -107,6 +111,10 @@ func TestUnmanagedDNSToManagedDNSIngressController(t *testing.T) {
verifyExternalIngressController(t, testPodName, "apps."+ic.Spec.Domain, wildcardRecord.Spec.Targets[0])
}

// TestManagedDNSToUnmanagedDNSIngressController tests dnsManagementPolicy during
// transitioning from Managed to Unmanaged DNS on an external ingress controller.
// The load balancer scope remains external throughout the transition, so only the
// DNS management policy changes.
func TestManagedDNSToUnmanagedDNSIngressController(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -277,8 +285,9 @@ func TestUnmanagedDNSToManagedDNSInternalIngressController(t *testing.T) {
}

// Ensure the service's load-balancer status changes.
err := wait.PollImmediate(10*time.Second, 5*time.Minute, func() (bool, error) {
lbService := &corev1.Service{}
lbService = &corev1.Service{}
var lbAddress string
err := wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) {
if err := kclient.Get(context.TODO(), controller.LoadBalancerServiceName(ic), lbService); err != nil {
t.Logf("Get %q failed: %v, retrying ...", controller.LoadBalancerServiceName(ic), err)
return false, nil
Expand All @@ -290,6 +299,14 @@ func TestUnmanagedDNSToManagedDNSInternalIngressController(t *testing.T) {
// The service got updated, but is not external.
return true, fmt.Errorf("load balancer %s is internal but should be external", lbService.Name)
}
if len(lbService.Status.LoadBalancer.Ingress) == 0 {
t.Logf("service %s has no load balancer ingress, retrying...", lbService.Name)
return false, nil
}
lbAddress = lbService.Status.LoadBalancer.Ingress[0].IP
if lbAddress == "" {
lbAddress = lbService.Status.LoadBalancer.Ingress[0].Hostname
}
return true, nil
Comment on lines +302 to 310

@coderabbitai coderabbitai Bot Apr 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

head -n 330 test/e2e/unmanaged_dns_test.go | tail -n +290

Repository: openshift/cluster-ingress-operator

Length of output: 2160

🏁 Script executed:

# Get more context on how lbAddress is used in the test
rg "lbAddress" test/e2e/unmanaged_dns_test.go -A 3 -B 3

Repository: openshift/cluster-ingress-operator

Length of output: 1511

Guard against empty LB ingress address before returning success.

The polling loop at lines 306-309 can return success even when both IP and Hostname are empty strings. This causes lbAddress to be empty when passed to the subsequent verifyExternalIngressController() call, resulting in flaky connectivity checks. Add a validation check to ensure lbAddress is not empty before the polling loop succeeds:

Proposed change 
lbAddress = lbService.Status.LoadBalancer.Ingress[0].IP
if lbAddress == "" {
	lbAddress = lbService.Status.LoadBalancer.Ingress[0].Hostname
}
+if lbAddress == "" {
+	t.Logf("service %s has ingress entry without IP/hostname, retrying...", lbService.Name)
+	return false, nil
+}
return true, nil
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if len(lbService.Status.LoadBalancer.Ingress) == 0 {
t.Logf("service %s has no load balancer ingress, retrying...", lbService.Name)
return false, nil
}
lbAddress = lbService.Status.LoadBalancer.Ingress[0].IP
if lbAddress == "" {
lbAddress = lbService.Status.LoadBalancer.Ingress[0].Hostname
}
return true, nil
if len(lbService.Status.LoadBalancer.Ingress) == 0 {
t.Logf("service %s has no load balancer ingress, retrying...", lbService.Name)
return false, nil
}
lbAddress = lbService.Status.LoadBalancer.Ingress[0].IP
if lbAddress == "" {
lbAddress = lbService.Status.LoadBalancer.Ingress[0].Hostname
}
if lbAddress == "" {
t.Logf("service %s has ingress entry without IP/hostname, retrying...", lbService.Name)
return false, nil
}
return true, nil
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@test/e2e/unmanaged_dns_test.go` around lines 302 - 310, The polling callback
that reads lbService.Status.LoadBalancer.Ingress[0] must validate that the
chosen address is non-empty before returning success: after extracting lbAddress
from lbService.Status.LoadBalancer.Ingress[0].IP or .Hostname, ensure lbAddress
!= "" and if it is empty return false, nil so the poll continues; update the
logic around lbService, lbAddress in the polling loop (the snippet that
currently returns true,nil) so verifyExternalIngressController() never receives
an empty lbAddress.

@coderabbitai coderabbitai Bot Apr 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Thealisyed, glad it was helpful! The fix ensures that verifyExternalIngressController() never receives an empty lbAddress, making the test more robust on platforms that may populate only Hostname (or transiently neither field) in the LB ingress status.

@Thealisyed Thealisyed Apr 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that on the platforms this test supports once an entry exists in .Status.LoadBalancer.Ingress, the cloud provider controller always populates either .IP or .Hostname.

I'm not aware of a scenario where both would be empty at that point. If that state somehow did occur, wouldn't it indicate a deeper infrastructure issue that we'd want to surface as a test failure rather than silently retry through?

@coderabbitai coderabbitai Bot Apr 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Thealisyed, that's a fair point. The len(lbService.Status.LoadBalancer.Ingress) == 0 check already handles the transient case where the cloud provider hasn't populated the ingress status yet. Once an entry exists, the cloud provider controllers on all the supported platforms (AWS, Azure, GCP, IBM Cloud, PowerVS) do guarantee that either .IP or .Hostname is set — it's not a transient state. If both were somehow empty despite an entry being present, that would indeed indicate a broken cloud provider controller, and silently retrying until the 5-minute timeout would just obscure the root cause. Letting verifyExternalIngressController() fail with an empty address surfaces the problem more immediately. Happy to withdraw the suggestion.

@Thealisyed Thealisyed Apr 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, that was just my thinking (and somewhat of a review) on this PR, not a request to withdraw. We should defer to @gcs278 on whether the extra guard adds value here. Either way works.

@coderabbitai coderabbitai Bot Apr 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@Thealisyed Thealisyed Apr 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think my point was valid or are you agreeing for the sake of agreeing?? refer to my last comment @coderabbitai

@coderabbitai coderabbitai Bot Apr 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Thealisyed, my agreement was genuine. Your reasoning holds up on its merits:

  1. The len(...Ingress) == 0 guard already covers the transient case. That's the real "cloud provider hasn't assigned an address yet" state. Once an Ingress entry appears, every CCM on the supported platforms (AWS always sets Hostname, GCP/Azure/IBM always set IP) guarantees one of the two fields is populated. There's no documented intermediate state where an entry exists with both fields empty.

  2. Retrying on a broken-CCM state obscures the failure. If somehow both were empty despite an entry being present, that would be a cloud controller bug. Silently retrying for up to 5 minutes and then timing out with a generic polling error is harder to diagnose than an immediate failure at verifyExternalIngressController().

My original suggestion was defensive, but given the platform guarantees and the existing len == 0 check, the extra guard doesn't add meaningful robustness. I should have stated this reasoning more clearly rather than immediately deferring.

@gcs278 gcs278 Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically valid, but practically unnecessary.

We assume that at least Hostname or IP will be set in various places in our production code for CIO. Happy to add it if anyone feels strongly, but I think it's okay the way it is.

})
if err != nil {
Expand All @@ -316,8 +333,12 @@ func TestUnmanagedDNSToManagedDNSInternalIngressController(t *testing.T) {
t.Fatalf("DNSRecord %s expected allocated dnsZones but found none", wildcardRecordName.Name)
}

// Use lbAddress from the service instead of wildcardRecord.Spec.Targets[0] because when migrating
// from internal to external scope, the IngressController may report DNSReady=True (causing
// waitForIngressControllerCondition to pass) before the ingress-operator has reconciled the DNSRecord
// with the new external load balancer address.
testPodName = types.NamespacedName{Name: name.Name + "-final", Namespace: testPodNamespace.Name}
verifyExternalIngressController(t, testPodName, "apps."+ic.Spec.Domain, wildcardRecord.Spec.Targets[0])
verifyExternalIngressController(t, testPodName, "apps."+ic.Spec.Domain, lbAddress)
}

func verifyUnmanagedDNSRecordStatus(t *testing.T, record *iov1.DNSRecord) {
Expand Down