Skip to content

CORS-4174: Azure: Add Ingress LB IPs to Infra CR when in-cluster DNS is enabled #1256

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 2 commits into from
Nov 22, 2025
+123 −3
Merged

CORS-4174: Azure: Add Ingress LB IPs to Infra CR when in-cluster DNS is enabled #1256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b99000f
Update AzurePlatformStatus with Ingress LB IPs
sadasu Nov 21, 2025
10842ab
Add unit tests to verify update of Infra CR with Ingress LB IPs
sadasu Nov 21, 2025
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
25 changes: 24 additions & 1 deletion pkg/operator/controller/ingress/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,11 @@ func computeEffectivePublishingStrategy(ic *operatorv1.IngressController, platfo
platformStatus.AWS.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
effectiveStrategy.LoadBalancer.DNSManagementPolicy = operatorv1.UnmanagedLoadBalancerDNS
}
case configv1.AzurePlatformType:
if platformStatus.Azure != nil && platformStatus.Azure.CloudLoadBalancerConfig != nil &&
platformStatus.Azure.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
effectiveStrategy.LoadBalancer.DNSManagementPolicy = operatorv1.UnmanagedLoadBalancerDNS
}
case configv1.GCPPlatformType:
if platformStatus.GCP != nil && platformStatus.GCP.CloudLoadBalancerConfig != nil &&
platformStatus.GCP.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
Expand Down Expand Up @@ -1330,7 +1335,7 @@ func (r *reconciler) allRouterPodsDeleted(ingress *operatorv1.IngressController)
return true, nil
}

// computeUpdatedInfraFromService updates PlatformStatus for GCP and AWS with Ingress LB IPs when the DNSType is `ClusterHosted`.
// 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
if platformStatus == nil {
Expand Down Expand Up @@ -1373,6 +1378,24 @@ func computeUpdatedInfraFromService(service *corev1.Service, infraConfig *config
}
}
return false, nil
case configv1.AzurePlatformType:
if platformStatus.Azure != nil && platformStatus.Azure.CloudLoadBalancerConfig != nil && platformStatus.Azure.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
if platformStatus.Azure.CloudLoadBalancerConfig.ClusterHosted == nil {
platformStatus.Azure.CloudLoadBalancerConfig.ClusterHosted = &configv1.CloudLoadBalancerIPs{}
}
ingresses := service.Status.LoadBalancer.Ingress
ingressLBIPs := []configv1.IP{}
for _, ingress := range ingresses {
if len(ingress.IP) > 0 {
ingressLBIPs = append(ingressLBIPs, configv1.IP(ingress.IP))
}
}
if !cmp.Equal(platformStatus.Azure.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, ingressLBIPs, ipCmpOpts...) {
platformStatus.Azure.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs = ingressLBIPs
return true, nil
}
}
return false, nil
Comment on lines +1381 to +1398
Copy link
Contributor

@Miciah Miciah Nov 19, 2025

Choose a reason for hiding this comment

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

What do you think about refactoring the logic for updating the CloudLoadBalancerConfig?

diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go
index 0a208cfd3..83d627bd1 100644
--- a/pkg/operator/controller/ingress/controller.go
+++ b/pkg/operator/controller/ingress/controller.go
@@ -1336,63 +1336,86 @@ func computeUpdatedInfraFromService(service *corev1.Service, infraConfig *config
 	if platformStatus == nil {
 		return false, fmt.Errorf("invalid PlatformStatus within Infrastructure config")
 	}
-	ipCmpOpts := []cmp.Option{
-		cmpopts.EquateEmpty(),
-		cmpopts.SortSlices(func(a, b configv1.IP) bool {
-			return a < b
-		}),
-	}
 	// The cluster has to run its own CoreDNS pod for DNS. Update Infra CR with the Ingress LB IPs.
 	// These values are used to configure the in-cluster DNS to provide resolution for *.apps.
 	switch platformStatus.Type {
 	case configv1.AWSPlatformType:
 		// On the AWS platform, only the Ingress LB's Hostname is available.
 		// Resolve this Hostname to its IPs before adding to the Infra CR.
-		if platformStatus.AWS != nil && platformStatus.AWS.CloudLoadBalancerConfig != nil && platformStatus.AWS.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
-			if platformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted == nil {
-				platformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted = &configv1.CloudLoadBalancerIPs{}
-			}
-			ingresses := service.Status.LoadBalancer.Ingress
-			ingressLBIPs := []configv1.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)
-				if err != nil {
-					return false, fmt.Errorf("failed to lookup IP addresses corresponding to AWS LB hostname: %w", err)
-				}
+		if platformStatus.AWS != nil {
+			updated, err := copyServiceIngressLBIPsToCloudLoadBalancerConfig(service, platformStatus.AWS.CloudLoadBalancerConfig)
 
-				if len(ingressIPs) > 0 {
-					for _, ingressIP := range ingressIPs {
-						ingressLBIPs = append(ingressLBIPs, configv1.IP(ingressIP.String()))
-					}
-				}
-			}
-			if !cmp.Equal(platformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, ingressLBIPs, ipCmpOpts...) {
-				platformStatus.AWS.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs = ingressLBIPs
-				return true, nil
-			}
+			return updated, err
+		}
+		return false, nil
+	case configv1.AzurePlatformType:
+		if platformStatus.GCP != nil {
+			updated, err := copyServiceIngressLBIPsToCloudLoadBalancerConfig(service, platformStatus.Azure.CloudLoadBalancerConfig)
+
+			return updated, err
 		}
 		return false, nil
 	case configv1.GCPPlatformType:
-		if platformStatus.GCP != nil && platformStatus.GCP.CloudLoadBalancerConfig != nil && platformStatus.GCP.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
-			if platformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted == nil {
-				platformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted = &configv1.CloudLoadBalancerIPs{}
-			}
-			ingresses := service.Status.LoadBalancer.Ingress
-			ingressLBIPs := []configv1.IP{}
-			for _, ingress := range ingresses {
-				if len(ingress.IP) > 0 {
-					ingressLBIPs = append(ingressLBIPs, configv1.IP(ingress.IP))
-				}
-			}
-			if !cmp.Equal(platformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, ingressLBIPs, ipCmpOpts...) {
-				platformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs = ingressLBIPs
-				return true, nil
-			}
+		if platformStatus.GCP != nil {
+			updated, err := copyServiceIngressLBIPsToCloudLoadBalancerConfig(service, platformStatus.GCP.CloudLoadBalancerConfig)
+
+			return updated, err
 		}
 		return false, nil
 	default:
 		return false, nil
 	}
+}
+
+// copyServiceIngressLBIPsToCloudLoadBalancerConfig updates the given
+// cloudLoadBalancerConfig's ingressLoadBalancerIPs field with any ingress IP
+// addresses from the given service object's status.  If cloudLoadBalancerConfig
+// is nil or does not specify the "ClusterHosted" DNS type, this function does
+// not update the cloudLoadBalancerConfig object.
+func copyServiceIngressLBIPsToCloudLoadBalancerConfig(service *corev1.Service, cloudLoadBalancerConfig *configv1.CloudLoadBalancerConfig) (bool, error) {
+	if cloudLoadBalancerConfig == nil {
+		return false, nil
+	}
+
+	if cloudLoadBalancerConfig.DNSType != configv1.ClusterHostedDNSType {
+		return false, nil
+	}
+
+	if cloudLoadBalancerConfig.ClusterHosted == nil {
+		cloudLoadBalancerConfig.ClusterHosted = &configv1.CloudLoadBalancerIPs{}
+	}
+
+	ingresses := service.Status.LoadBalancer.Ingress
+	ingressLBIPs := []configv1.IP{}
+	for _, ingress := range ingresses {
+		if len(ingress.IP) > 0 {
+			ingressLBIPs = append(ingressLBIPs, configv1.IP(ingress.IP))
+		} else if len(ingress.Hostname) != 0 {
+			// 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)
+			if err != nil {
+				return false, fmt.Errorf("failed to lookup IP addresses corresponding to LB hostname: %w", err)
+			}
+
+			if len(ingressIPs) > 0 {
+				for _, ingressIP := range ingressIPs {
+					ingressLBIPs = append(ingressLBIPs, configv1.IP(ingressIP.String()))
+				}
+			}
+		}
+	}
+
+	ipCmpOpts := []cmp.Option{
+		cmpopts.EquateEmpty(),
+		cmpopts.SortSlices(func(a, b configv1.IP) bool {
+			return a < b
+		}),
+	}
+	if !cmp.Equal(cloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs, ingressLBIPs, ipCmpOpts...) {
+		cloudLoadBalancerConfig.ClusterHosted.IngressLoadBalancerIPs = ingressLBIPs
+
+		return true, nil
+	}
+
 	return false, nil
 }

We can leave that for a follow-up PR if you prefer.

case configv1.GCPPlatformType:
if platformStatus.GCP != nil && platformStatus.GCP.CloudLoadBalancerConfig != nil && platformStatus.GCP.CloudLoadBalancerConfig.DNSType == configv1.ClusterHostedDNSType {
if platformStatus.GCP.CloudLoadBalancerConfig.ClusterHosted == nil {
Expand Down
101 changes: 99 additions & 2 deletions pkg/operator/controller/ingress/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,29 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
}
}

makeDefaultAzurePlatformStatus = func(platform configv1.PlatformType) *configv1.PlatformStatus {
return &configv1.PlatformStatus{
Type: platform,
Azure: &configv1.AzurePlatformStatus{
CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
DNSType: configv1.PlatformDefaultDNSType,
},
},
}
}

makeBYODNSAzurePlatformStatus = func(platform configv1.PlatformType) *configv1.PlatformStatus {
return &configv1.PlatformStatus{
Type: platform,
Azure: &configv1.AzurePlatformStatus{
CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
DNSType: configv1.ClusterHostedDNSType,
ClusterHosted: &configv1.CloudLoadBalancerIPs{},
},
},
}
}

ingressConfigWithDefaultClassicLB = &configv1.Ingress{
Spec: configv1.IngressSpec{
LoadBalancer: configv1.LoadBalancer{
Expand Down Expand Up @@ -320,6 +343,18 @@ func TestSetDefaultPublishingStrategySetsPlatformDefaults(t *testing.T) {
expectedIC: ingressControllerWithLoadBalancerUnmanagedDNS,
domainMatchesBaseDomain: true,
},
{
name: "Azure",
platformStatus: makeDefaultAzurePlatformStatus(configv1.AzurePlatformType),
expectedIC: ingressControllerWithLoadBalancer,
domainMatchesBaseDomain: true,
},
{
name: "Azure With BYO DNS",
platformStatus: makeBYODNSAzurePlatformStatus(configv1.AzurePlatformType),
expectedIC: ingressControllerWithLoadBalancerUnmanagedDNS,
domainMatchesBaseDomain: true,
},
{
name: "GCP",
platformStatus: makeDefaultGCPPlatformStatus(configv1.GCPPlatformType),
Expand Down Expand Up @@ -1617,7 +1652,10 @@ func Test_IsProxyProtocolNeeded(t *testing.T) {

func Test_computeUpdatedInfraFromService(t *testing.T) {
var (
IngressLBIP = configv1.IP("196.78.125.4")
IngressLBIP = configv1.IP("196.78.125.4")
nonePlatform = configv1.PlatformStatus{
Type: configv1.NonePlatformType,
}
awsPlatform = configv1.PlatformStatus{
Type: configv1.AWSPlatformType,
}
Expand Down Expand Up @@ -1645,6 +1683,27 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
azurePlatform = configv1.PlatformStatus{
Type: configv1.AzurePlatformType,
}
azurePlatformWithDNSType = configv1.PlatformStatus{
Type: configv1.AzurePlatformType,
Azure: &configv1.AzurePlatformStatus{
CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
DNSType: configv1.ClusterHostedDNSType,
},
},
}
azurePlatformWithLBIP = configv1.PlatformStatus{
Type: configv1.AzurePlatformType,
Azure: &configv1.AzurePlatformStatus{
CloudLoadBalancerConfig: &configv1.CloudLoadBalancerConfig{
DNSType: configv1.ClusterHostedDNSType,
ClusterHosted: &configv1.CloudLoadBalancerIPs{
IngressLoadBalancerIPs: []configv1.IP{
IngressLBIP,
},
},
},
},
}
gcpPlatform = configv1.PlatformStatus{
Type: configv1.GCPPlatformType,
}
Expand Down Expand Up @@ -1701,7 +1760,7 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
},
{
description: "unsupported platform should not cause an error",
platform: &azurePlatform,
platform: &nonePlatform,
ingresses: []corev1.LoadBalancerIngress{},
expectUpdated: false,
expectError: false,
Expand Down Expand Up @@ -1782,6 +1841,44 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
expectUpdated: true,
expectError: false,
},
{
description: "azure platform without DNSType set",
platform: &azurePlatform,
ingresses: []corev1.LoadBalancerIngress{},
expectUpdated: false,
expectError: false,
},
{
description: "azure platform with DNSType and no LB IP",
platform: &azurePlatformWithDNSType,
ingresses: []corev1.LoadBalancerIngress{},
expectUpdated: false,
expectError: false,
},
{
description: "azure platform with DNSType and no LB IP in infra config, service has 1 IP",
platform: &azurePlatformWithDNSType,
ingresses: ingresses,
expectedLBIPs: []configv1.IP{IngressLBIP},
expectUpdated: true,
expectError: false,
},
{
description: "azure platform with no change to LB IPs",
platform: &azurePlatformWithLBIP,
ingresses: ingresses,
expectedLBIPs: []configv1.IP{IngressLBIP},
expectUpdated: false,
expectError: false,
},
{
description: "azure platform with DNSType and LB IP",
platform: &azurePlatformWithLBIP,
ingresses: ingressesWithMultipleIPs,
expectedLBIPs: []configv1.IP{IngressLBIP, configv1.IP("10.10.10.4")},
expectUpdated: true,
expectError: false,
},
Comment on lines +1852 to +1881
Copy link
Contributor

@Miciah Miciah Nov 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Another case to consider is where the infra config status has an IP address and the service status does not—the expected behavior is that the infra config status be updated with an empty list of IP addresses, right?

		{
			description:   "azure platform with DNSType and 1 LB IP in infra config, service has none",
			platform:      &azurePlatformWithLBIP,
			ingresses:     []corev1.LoadBalancerIngress{},
			expectedLBIPs: []configv1.IP{},
			expectUpdated: true,
			expectError:   false,
		},

By the way, the test runner should make a copy of the test input in case the test mutates it:

diff --git a/pkg/operator/controller/ingress/controller_test.go b/pkg/operator/controller/ingress/controller_test.go
index 434f7509c..c4364846a 100644
--- a/pkg/operator/controller/ingress/controller_test.go
+++ b/pkg/operator/controller/ingress/controller_test.go
@@ -1787,7 +1787,7 @@ func Test_computeUpdatedInfraFromService(t *testing.T) {
 		t.Run(tc.description, func(t *testing.T) {
 			infraConfig := &configv1.Infrastructure{
 				Status: configv1.InfrastructureStatus{
-					PlatformStatus: tc.platform,
+					PlatformStatus: tc.platform.DeepCopy(),
 				},
 			}
 			service := &corev1.Service{}

If you add the DeepCopy fix as part of this PR, please put it as a separate commit as it is logically a separate fix from the feature itself or the new test cases.

}
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
Expand Down