From e9357ff0e4daba6f45b48137c4f9f6e57c41d69d Mon Sep 17 00:00:00 2001 From: miheer Date: Mon, 19 Aug 2024 17:48:01 +1000 Subject: [PATCH] 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. --- .../ingress/load_balancer_service.go | 46 ++++++- .../ingress/load_balancer_service_test.go | 55 ++++++++ test/e2e/all_test.go | 1 + test/e2e/lb_eip_test.go | 117 +++++++++++++++++- 4 files changed, 213 insertions(+), 6 deletions(-) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index d45af7fe90..35b46329db 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -441,6 +441,16 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef service.Annotations[awsLBSubnetsAnnotation] = JoinAWSSubnets(clbParams.Subnets, ",") } } + + if eipAllocationsAWSEnabled { + nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ci) + if nlbParams != nil && awsEIPAllocationsExist(nlbParams.EIPAllocations) { + // Remove the annotation + if _, exists := service.Annotations[awsEIPAllocationsAnnotation]; exists { + delete(service.Annotations, awsEIPAllocationsAnnotation) + } + } + } } } } @@ -617,11 +627,28 @@ func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service, } // Diff before updating because the client may mutate the object. diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) - if err := r.client.Update(context.TODO(), updated); err != nil { - return false, err + // Recreate lb service in AWS if the lbType has changed and if the current service has `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` annotation + if platform.Type == configv1.AWSPlatformType && current.Annotations[AWSLBTypeAnnotation] != desired.Annotations[AWSLBTypeAnnotation] { + if _, exists := current.Annotations[awsEIPAllocationsAnnotation]; exists { + log.Info("deleting and recreating the load balancer ", "namespace", updated.Namespace, "name", updated.Name) + foreground := metav1.DeletePropagationForeground + deleteOptions := crclient.DeleteOptions{PropagationPolicy: &foreground} + if err := r.deleteLoadBalancerService(current, &deleteOptions); err != nil { + return false, err + } + if err := r.createLoadBalancerService(updated); err != nil { + return false, err + } + return true, nil + } + } else { + if err := r.client.Update(context.TODO(), updated); err != nil { + return false, err + } + log.Info("updated load balancer service", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) + return true, nil } - log.Info("updated load balancer service", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) - return true, nil + return false, nil } // scopeEqual returns true if the scope is the same between the two given @@ -675,7 +702,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, diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index cb0f61ea47..54c7ed24ce 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/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 removes the `service.beta.kubernetes.io/aws-load-balancer-eip-allocations` from service. + awsEIPAllocationsAnnotation: {false, ""}, + }, + }, { description: "network load balancer with eipAllocations for aws platform when feature gate is enabled", platformStatus: platformStatus(configv1.AWSPlatformType), diff --git a/test/e2e/all_test.go b/test/e2e/all_test.go index f4ed961f57..6d0960da02 100644 --- a/test/e2e/all_test.go +++ b/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) { diff --git a/test/e2e/lb_eip_test.go b/test/e2e/lb_eip_test.go index b4b4d01a20..45b6ffda2a 100644 --- a/test/e2e/lb_eip_test.go +++ b/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) }