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

Open
gcs278 wants to merge 1 commit into
base: master
Choose a base branch
from
+24 −3
Open

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
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 3, 2026

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.

})
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