OCPBUGS-38411: Not allow eipAllocations in CLB while changing the LBType of the Ingress Controller from NLB to Classic.

After changing the LBType of the IngressController from `NLB` to `Classic`, the eipAllocations still were attached with controller and the LB service.
Howver, eipAllocations are  not supported in Classic.

This commit fixes service annotation `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` value to a blank value. The CCM then removes this annotation from the service.
It also sets the IngressController in Progressing Status to True with a status message to delete the load balancer service so that a new lb service is recreated for Classic LB.

Author: miheer

pkg/operator/controller/ingress/load_balancer_service.go

@@ -441,6 +441,13 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 								service.Annotations[awsLBSubnetsAnnotation] = JoinAWSSubnets(clbParams.Subnets, ",")
 							}
 						}
+
+						if eipAllocationsAWSEnabled {
+							nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ci)
+							if nlbParams != nil && awsEIPAllocationsExist(nlbParams.EIPAllocations) {
+								service.Annotations[awsEIPAllocationsAnnotation] = ""
+							}
+						}
 					}
 				}
 			}
@@ -675,7 +682,16 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev
 	// avoid problems, make sure the previous release blocks upgrades when
 	// the user has modified an annotation or spec field that the new
 	// release manages.
-	changed, updated := loadBalancerServiceAnnotationsChanged(current, expected, managedLoadBalancerServiceAnnotations)
+
+	// Make eipAllocation annotation managed ONLY when LB type annotation changes.
+	// Ordinarily, the eipAllocation annotation is not managed, so they are not immediately effectuated.
+	// However, since eipAllocations are specific to NLB type, when LB type is changed from NLB to Classic, we MUST not use
+	// eipAllocations associated with the NLB for Classic.
+	managedAnnotationsUpdated := managedLoadBalancerServiceAnnotations
+	if current.Annotations[AWSLBTypeAnnotation] != expected.Annotations[AWSLBTypeAnnotation] {
+		managedAnnotationsUpdated = managedLoadBalancerServiceAnnotations.Union(sets.NewString(awsEIPAllocationsAnnotation))
+	}
+	changed, updated := loadBalancerServiceAnnotationsChanged(current, expected, managedAnnotationsUpdated)
 
 	// If spec.loadBalancerSourceRanges is nonempty on the service, that
 	// means that allowedSourceRanges is nonempty on the ingresscontroller,
@@ -846,6 +862,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service
 		if nlbParams := getAWSNetworkLoadBalancerParametersInStatus(ic); nlbParams != nil {
 			haveEIPAllocations = nlbParams.EIPAllocations
 		}
+
 		if !awsEIPAllocationsEqual(wantEIPAllocations, haveEIPAllocations) {
 			// Generate JSON for the oc patch command as well as "pretty" json
 			// that will be used for a more human-readable error message.
@@ -856,6 +873,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service
 			ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"type\":\"LoadBalancerService\",\"loadBalancer\":{\"providerParameters\":{\"type\":\"AWS\",\"aws\":{\"type\":\"%[2]s\",\"%[3]s\":{\"eipAllocations\":%[4]s}}}}}}}'", ic.Name, getAWSLoadBalancerTypeInStatus(ic), "networkLoadBalancer", haveEIPAllocationsPatchJson)
 			err := fmt.Errorf("%[1]s  To effectuate this change, you must delete the service: `oc -n %[2]s delete svc/%[3]s`; the service load-balancer will then be deprovisioned and a new one created. This will most likely cause the new load-balancer to have a different host name and IP address and cause disruption. To return to the previous state, you can revert the change to the IngressController: `%[4]s`", changedMsg, service.Namespace, service.Name, ocPatchRevertCmd)
 			errs = append(errs, err)
+
 		}
 	}
 

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -61,6 +61,34 @@ func Test_desiredLoadBalancerService(t *testing.T) {
 			}
 			return eps
 		}
+
+		clb = func(scope operatorv1.LoadBalancerScope) *operatorv1.EndpointPublishingStrategy {
+			eps := lbs(scope)
+			eps.LoadBalancer.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{
+				Type: operatorv1.AWSLoadBalancerProvider,
+				AWS: &operatorv1.AWSLoadBalancerParameters{
+					Type:                          operatorv1.AWSClassicLoadBalancer,
+					ClassicLoadBalancerParameters: &operatorv1.AWSClassicLoadBalancerParameters{},
+				},
+			}
+			return eps
+		}
+
+		clbWithEIPAllocations = func(scope operatorv1.LoadBalancerScope, eipAllocations []operatorv1.EIPAllocation) *operatorv1.EndpointPublishingStrategy {
+			eps := lbs(scope)
+			eps.LoadBalancer.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{
+				Type: operatorv1.AWSLoadBalancerProvider,
+				AWS: &operatorv1.AWSLoadBalancerParameters{
+					Type: operatorv1.AWSClassicLoadBalancer,
+					NetworkLoadBalancerParameters: &operatorv1.AWSNetworkLoadBalancerParameters{
+						EIPAllocations: eipAllocations,
+					},
+				},
+			}
+			eps.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations = eipAllocations
+			return eps
+		}
+
 		awsLbWithSubnets = func(lbType operatorv1.AWSLoadBalancerType, subnets *operatorv1.AWSSubnets) *operatorv1.EndpointPublishingStrategy {
 			eps := lbs(operatorv1.ExternalLoadBalancer)
 			eps.LoadBalancer.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{
@@ -454,6 +482,33 @@ func Test_desiredLoadBalancerService(t *testing.T) {
 				awsLBSubnetsAnnotation:                       {true, "subnet-00000000000000001,subnet-00000000000000002,subnetA,subnetB"},
 			},
 		},
+		{
+			description:    "change from network load balancer with eipAllocations to classic load balancer type for aws platform when feature gate is enabled",
+			platformStatus: platformStatus(configv1.AWSPlatformType),
+			strategySpec: clbWithEIPAllocations(operatorv1.ExternalLoadBalancer,
+				[]operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"},
+			),
+			// setDefaultPublishingStrategy sets CLB empty provider parameters in the status of IC once the LB Type is changed from NLB to CLB.
+			strategyStatus:                  clb(operatorv1.ExternalLoadBalancer),
+			proxyNeeded:                     true,
+			expectService:                   true,
+			eipAllocationsAWSFeatureEnabled: true,
+			expectedExternalTrafficPolicy:   corev1.ServiceExternalTrafficPolicyLocal,
+			expectedServiceAnnotations: map[string]annotationExpectation{
+				awsInternalLBAnnotation:                      {false, ""},
+				awsLBAdditionalResourceTags:                  {false, ""},
+				awsLBHealthCheckHealthyThresholdAnnotation:   {true, awsLBHealthCheckHealthyThresholdDefault},
+				awsLBHealthCheckIntervalAnnotation:           {true, "5"},
+				awsLBHealthCheckTimeoutAnnotation:            {true, awsLBHealthCheckTimeoutDefault},
+				awsLBHealthCheckUnhealthyThresholdAnnotation: {true, awsLBHealthCheckUnhealthyThresholdDefault},
+				awsLBProxyProtocolAnnotation:                 {true, "*"},
+				AWSLBTypeAnnotation:                          {false, AWSNLBAnnotation},
+				localWithFallbackAnnotation:                  {true, ""},
+				awsLBSubnetsAnnotation:                       {false, ""},
+				// The CIO sets the annotation value to blank however, the CCM however removes the `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` from service.
+				awsEIPAllocationsAnnotation: {true, ""},
+			},
+		},
 		{
 			description:    "network load balancer with eipAllocations for aws platform when feature gate is enabled",
 			platformStatus: platformStatus(configv1.AWSPlatformType),

test/e2e/all_test.go

@@ -88,6 +88,7 @@ func TestAll(t *testing.T) {
 		t.Run("TestUnmanagedAWSLBSubnets", TestUnmanagedAWSLBSubnets)
 		t.Run("TestAWSEIPAllocationsForNLB", TestAWSEIPAllocationsForNLB)
 		t.Run("TestUnmanagedAWSEIPAllocations", TestUnmanagedAWSEIPAllocations)
+		t.Run("TestAWSEIPAllocationsLBTypeChangeFromNLBToClassic", TestAWSEIPAllocationsLBTypeChangeFromNLBToClassic)
 	})
 
 	t.Run("serial", func(t *testing.T) {

test/e2e/lb_eip_test.go

@@ -180,6 +180,121 @@ func TestAWSEIPAllocationsForNLB(t *testing.T) {
 	verifyExternalIngressController(t, externalTestPodName, testHostname, elbHostname)
 }
 
+func TestAWSEIPAllocationsLBTypeChangeFromNLBToClassic(t *testing.T) {
+	t.Parallel()
+	if infraConfig.Status.PlatformStatus == nil {
+		t.Skip("test skipped on nil platform")
+	}
+	if infraConfig.Status.PlatformStatus.Type != configv1.AWSPlatformType {
+		t.Skipf("test skipped on platform %q", infraConfig.Status.PlatformStatus.Type)
+	}
+	if enabled, err := isFeatureGateEnabled(features.FeatureGateSetEIPForNLBIngressController); err != nil {
+		t.Fatalf("failed to get feature gate: %v", err)
+	} else if !enabled {
+		t.Skipf("test skipped because %q feature gate is not enabled", features.FeatureGateSetEIPForNLBIngressController)
+	}
+
+	// Create an ingress controller with EIPs mentioned in the Ingress Controller CR.
+	var eipAllocations []operatorv1.EIPAllocation
+
+	ec2ServiceClient := createEC2ServiceClient(t, infraConfig)
+	clusterName, err := getClusterName(infraConfig)
+	if err != nil {
+		t.Fatal(err)
+	}
+	vpcID, err := getVPCId(ec2ServiceClient, clusterName)
+	if err != nil {
+		t.Fatalf("failed to get VPC ID due to error: %v", err)
+	}
+	validEIPAllocations, err := createAWSEIPs(t, ec2ServiceClient, clusterName, vpcID)
+	if err != nil {
+		t.Fatalf("failed to create EIPs due to error: %v", err)
+	}
+	t.Cleanup(func() { assertEIPAllocationDeleted(t, ec2ServiceClient, 5*time.Minute, clusterName) })
+
+	for _, validEIPAllocation := range validEIPAllocations {
+		eipAllocations = append(eipAllocations, operatorv1.EIPAllocation(validEIPAllocation))
+	}
+
+	t.Logf("creating ingresscontroller with valid EIPs: %s", validEIPAllocations)
+	name := types.NamespacedName{Namespace: operatorNamespace, Name: "lbtypechange"}
+	domain := name.Name + "." + dnsConfig.Spec.BaseDomain
+	ic := newLoadBalancerController(name, domain)
+	ic.Spec.EndpointPublishingStrategy.LoadBalancer = &operatorv1.LoadBalancerStrategy{
+		Scope:               operatorv1.ExternalLoadBalancer,
+		DNSManagementPolicy: operatorv1.ManagedLoadBalancerDNS,
+		ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{
+			Type: operatorv1.AWSLoadBalancerProvider,
+			AWS: &operatorv1.AWSLoadBalancerParameters{
+				Type: operatorv1.AWSNetworkLoadBalancer,
+				NetworkLoadBalancerParameters: &operatorv1.AWSNetworkLoadBalancerParameters{
+					EIPAllocations: eipAllocations,
+				},
+			},
+		},
+	}
+
+	if err := kclient.Create(context.TODO(), ic); err != nil {
+		t.Fatalf("failed to create ingresscontroller: %v", err)
+	}
+	t.Cleanup(func() {
+		assertIngressControllerDeleted(t, kclient, ic)
+		serviceName := controller.LoadBalancerServiceName(ic)
+		// Waits for the service to clean up so EIPs can be released in the next t.Cleanup.
+		if err := waitForIngressControllerServiceDeleted(t, ic, 4*time.Minute); err != nil {
+			t.Errorf("failed to delete IngressController service %s/%s due to error: %v", serviceName.Namespace, serviceName.Name, err)
+		}
+	})
+
+	// Wait for the load balancer and DNS to be ready.
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	// Get ELB host name from the service status for use with verifyExternalIngressController.
+	// Using the ELB host name (not the IngressController wildcard domain) results in much quicker DNS resolution.
+	elbHostname := getIngressControllerLBAddress(t, ic)
+
+	// Ensure the expected eipAllocation annotation is on the service.
+	waitForLBAnnotation(t, ic, awsLBEIPAllocationAnnotation, true, ingress.JoinAWSEIPAllocations(eipAllocations, ","))
+
+	// Verify the eipAllocations status field is configured to what we expect.
+	verifyIngressControllerEIPAllocationStatus(t, name)
+
+	// Verify we can reach the NLB with the provided eipAllocations.
+	externalTestPodName := types.NamespacedName{Name: name.Name + "-external-verify", Namespace: name.Namespace}
+	testHostname := "apps." + ic.Spec.Domain
+	t.Logf("verifying external connectivity for ingresscontroller %q using an NLB with specified eipAllocations", ic.Name)
+	verifyExternalIngressController(t, externalTestPodName, testHostname, elbHostname)
+
+	// Now, update the IngressController to change from NLB to Classic LBType.
+	t.Logf("updating ingresscontroller %q to Classic LBType", ic.Name)
+	if err := updateIngressControllerWithRetryOnConflict(t, name, 5*time.Minute, func(ic *operatorv1.IngressController) {
+		ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type = operatorv1.AWSClassicLoadBalancer
+	}); err != nil {
+		t.Fatalf("failed to update ingresscontroller: %v", err)
+	}
+
+	//effectuateIngressControllerEIPAllocations(t, ic, nil, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...)
+	// Wait for the load balancer and DNS to be ready.
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	// Ensure the expected eipAllocation annotation is on the service.
+	waitForLBAnnotation(t, ic, awsLBEIPAllocationAnnotation, false, "")
+
+	// Get ELB host name from the service status for use with verifyExternalIngressController.
+	// Using the ELB host name (not the IngressController wildcard domain) results in much quicker DNS resolution.
+	elbHostname = getIngressControllerLBAddress(t, ic)
+
+	// Verify we can reach the CLB with the no eipAllocations.
+	externalTestPodName = types.NamespacedName{Name: name.Name + "-external-verify", Namespace: name.Namespace}
+	testHostname = "apps." + ic.Spec.Domain
+	t.Logf("verifying external connectivity for ingresscontroller %q using an NLB with specified eipAllocations", ic.Name)
+	verifyExternalIngressController(t, externalTestPodName, testHostname, elbHostname)
+}
+
 // TestUnmanagedAWSEIPAllocations tests compatibility for unmanaged service.beta.kubernetes.io/aws-load-balancer-eip-allocations
 // annotations on the IngressController service. This is done by directly configuring the annotation on the service
 // and then updating the IngressController to match the unmanaged eipAllocation annotation.
@@ -357,7 +472,7 @@ func effectuateIngressControllerEIPAllocations(t *testing.T, ic *operatorv1.Ingr
 	}
 
 	// Delete and recreate the IngressController service to effectuate.
-	t.Logf("recreating the service to effectuate the subnets: %s/%s", controller.LoadBalancerServiceName(ic).Namespace, controller.LoadBalancerServiceName(ic).Namespace)
+	t.Logf("recreating the service to effectuate the eipAllocations: %s/%s", controller.LoadBalancerServiceName(ic).Namespace, controller.LoadBalancerServiceName(ic).Namespace)
 	if err := recreateIngressControllerService(t, ic); err != nil {
 		t.Fatalf("failed to delete and recreate service: %v", err)
 	}