OCPBUGS-43692: Fix LB Type Defaulting with ProviderParameters Set

In the case that `alreadyAdmitted` was true and the IngressController's
spec had `providerParameters` set, `setDefaultProviderParameters`
incorrectly forced the LB type to `Classic`.

This fix removes `alreadyAdmitted` as a parameter from
`setDefaultProviderParameters` and completely prevents it from being
invoked when `alreadyAdmitted`, preventing the update of defaulted
values after an IngressController has been admitted.

Previously, `setDefaultProviderParameters` always ensured that
`specLB.ProviderParameters.AWS` was initialized. Now that the function
is conditionally invoked, an additional nil check has been added to
handle cases when it's not called.

An unit test has been added for this scenario to prevent regression.

pkg/operator/controller/ingress/controller.go

@@ -462,8 +462,10 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
 			}
 		}
 
-		// Set provider parameters based on the cluster ingress config.
-		setDefaultProviderParameters(effectiveStrategy.LoadBalancer, ingressConfig, alreadyAdmitted)
+		// Set provider parameters based on the cluster ingress config if not already admitted.
+		if !alreadyAdmitted {
+			setDefaultProviderParameters(effectiveStrategy.LoadBalancer, ingressConfig)
+		}
 
 	case operatorv1.NodePortServiceStrategyType:
 		if effectiveStrategy.NodePort == nil {
@@ -543,7 +545,7 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
 				if statusLB.ProviderParameters.AWS == nil {
 					statusLB.ProviderParameters.AWS = &operatorv1.AWSLoadBalancerParameters{}
 				}
-				if specLB.ProviderParameters.AWS.Type != statusLB.ProviderParameters.AWS.Type {
+				if specLB.ProviderParameters.AWS != nil && specLB.ProviderParameters.AWS.Type != statusLB.ProviderParameters.AWS.Type {
 					statusLB.ProviderParameters.AWS.Type = specLB.ProviderParameters.AWS.Type
 					changed = true
 				}
@@ -681,12 +683,12 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat
 // setDefaultProviderParameters mutates the given LoadBalancerStrategy by
 // defaulting its ProviderParameters field based on the defaults in the provided
 // ingress config object.
-func setDefaultProviderParameters(lbs *operatorv1.LoadBalancerStrategy, ingressConfig *configv1.Ingress, alreadyAdmitted bool) {
+func setDefaultProviderParameters(lbs *operatorv1.LoadBalancerStrategy, ingressConfig *configv1.Ingress) {
 	var provider operatorv1.LoadBalancerProviderType
 	if lbs.ProviderParameters != nil {
 		provider = lbs.ProviderParameters.Type
 	}
-	if len(provider) == 0 && !alreadyAdmitted {
+	if len(provider) == 0 {
 		// Infer the LB type from the cluster ingress config, but only
 		// if the ingresscontroller isn't already admitted.
 		switch ingressConfig.Spec.LoadBalancer.Platform.Type {
@@ -701,7 +703,7 @@ func setDefaultProviderParameters(lbs *operatorv1.LoadBalancerStrategy, ingressC
 		}
 		lbs.ProviderParameters.Type = provider
 		defaultLBType := operatorv1.AWSClassicLoadBalancer
-		if p := ingressConfig.Spec.LoadBalancer.Platform; !alreadyAdmitted && p.Type == configv1.AWSPlatformType && p.AWS != nil {
+		if p := ingressConfig.Spec.LoadBalancer.Platform; p.Type == configv1.AWSPlatformType && p.AWS != nil {
 			if p.AWS.Type == configv1.NLB {
 				defaultLBType = operatorv1.AWSNetworkLoadBalancer
 			}

pkg/operator/controller/ingress/controller_test.go

@@ -413,6 +413,18 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
 			}
 			return lbs
 		}
+		lbsWithPPType = func(scope operatorv1.LoadBalancerScope, policy *operatorv1.LoadBalancerDNSManagementPolicy) *operatorv1.LoadBalancerStrategy {
+			lbs := &operatorv1.LoadBalancerStrategy{
+				Scope: scope,
+				ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{
+					Type: operatorv1.AWSLoadBalancerProvider,
+				},
+			}
+			if policy != nil {
+				lbs.DNSManagementPolicy = *policy
+			}
+			return lbs
+		}
 		eps = func(lbs *operatorv1.LoadBalancerStrategy) *operatorv1.EndpointPublishingStrategy {
 			return &operatorv1.EndpointPublishingStrategy{
 				Type:         operatorv1.LoadBalancerServiceStrategyType,
@@ -662,6 +674,14 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
 			expectedIC:              makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())),
 			domainMatchesBaseDomain: true,
 		},
+		{
+			name:                    "loadbalancer type changed from NLB to unset, with NLB as default, with providerParameters.type set (OCPBUGS-43692)",
+			ic:                      makeIC(spec(eps(lbsWithPPType(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())),
+			ingressConfig:           ingressConfigWithDefaultNLB,
+			expectedResult:          false,
+			expectedIC:              makeIC(spec(eps(lbsWithPPType(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlbWithNullParameters())),
+			domainMatchesBaseDomain: true,
+		},
 		{
 			name:                    "loadbalancer ELB connection idle timeout changed from unset with null provider parameters to 2m",
 			ic:                      makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute})), status(elbWithNullParameters())),