From 65eea907aab3dbd1777ad5acc8a297717edc75d5 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Mon, 9 Sep 2024 18:55:43 -0400 Subject: [PATCH] OCPBUGS-16728: Require Service Deletion for LB Type Updates Due to the AWS CCM leaking resources when the load balancer type is changed on a service, the cloud team is now blocking updates to the load balancer type. As a result, the Ingress Operator will require the service to be deleted and recreated when the Ingress Controller's load balancer type changes. This change introduces a Progressing=True status message when the load balancer type is modified, instructing the user on how to effectuate the change. Additionally, the `service.beta.kubernetes.io/aws-load-balancer-type` annotation is now unmanaged. --- .../ingress/load_balancer_service.go | 62 ++++++++-- .../ingress/load_balancer_service_test.go | 9 +- .../controller/ingress/status_test.go | 70 ++++++++--- test/e2e/operator_test.go | 112 +++++++++++------- 4 files changed, 185 insertions(+), 68 deletions(-) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 7cef35dda..f1240911e 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -33,19 +33,19 @@ const ( // Key=Value pairs that are additionally recorded on // load balancer resources and security groups. // - // https://kubernetes.io/docs/concepts/services-networking/service/#aws-load-balancer-additional-resource-tags + // https://kubernetes.io/docs/reference/labels-annotations-taints/#service-beta-kubernetes-io-aws-load-balancer-additional-resource-tags awsLBAdditionalResourceTags = "service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags" // awsLBProxyProtocolAnnotation is used to enable the PROXY protocol on any // AWS load balancer services created. // - // https://kubernetes.io/docs/concepts/services-networking/service/#proxy-protocol-support-on-aws + // https://kubernetes.io/docs/reference/labels-annotations-taints/#service-beta-kubernetes-io-aws-load-balancer-proxy-protocol awsLBProxyProtocolAnnotation = "service.beta.kubernetes.io/aws-load-balancer-proxy-protocol" // AWSLBTypeAnnotation is a Service annotation used to specify an AWS load // balancer type. See the following for additional details: // - // https://kubernetes.io/docs/concepts/services-networking/service/#aws-nlb-support + // https://kubernetes.io/docs/reference/labels-annotations-taints/#service-beta-kubernetes-io-aws-load-balancer-type AWSLBTypeAnnotation = "service.beta.kubernetes.io/aws-load-balancer-type" // AWSNLBAnnotation is the annotation value of an AWS Network Load Balancer (NLB). @@ -242,8 +242,6 @@ var ( // local-with-fallback annotation for kube-proxy (see // ). localWithFallbackAnnotation, - // AWS load balancer type annotation to set either CLB/ELB or NLB - AWSLBTypeAnnotation, // awsLBProxyProtocolAnnotation is used to enable the PROXY protocol on any // AWS load balancer services created. // @@ -599,7 +597,7 @@ func (r *reconciler) deleteLoadBalancerService(service *corev1.Service, options // indicating whether the service was updated, and an error value. func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service, platform *configv1.PlatformStatus, autoDeleteLB bool) (bool, error) { if shouldRecreateLB, reason := shouldRecreateLoadBalancer(current, desired, platform); shouldRecreateLB && autoDeleteLB { - log.Info("deleting and recreating the load balancer because "+reason, "namespace", desired.Namespace, "name", desired.Name) + log.Info("deleting and recreating the load balancer", "reason", reason, "namespace", desired.Namespace, "name", desired.Name) foreground := metav1.DeletePropagationForeground deleteOptions := crclient.DeleteOptions{PropagationPolicy: &foreground} if err := r.deleteLoadBalancerService(current, &deleteOptions); err != nil { @@ -661,6 +659,9 @@ func shouldRecreateLoadBalancer(current, desired *corev1.Service, platform *conf if platform.Type == configv1.AWSPlatformType && !serviceEIPAllocationsEqual(current, desired) { return true, "its eipAllocations changed" } + if platform.Type == configv1.AWSPlatformType && !serviceAWSLBTypeEqual(current, desired) { + return true, "its load balancer type changed" + } return false, "" } @@ -799,12 +800,24 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service errs = append(errs, err) } + if platform.Type == configv1.AWSPlatformType { + wantLBType := getAWSLoadBalancerTypeInSpec(ic) + haveLBType := getAWSLBTypeFromServiceAnnotation(service) + + if wantLBType != haveLBType { + changedMsg := fmt.Errorf("The IngressController load balancer type was changed from %q to %q.", haveLBType, wantLBType) + 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\"}}}}}}'", ic.Name, wantLBType) + 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) + } + } + if platform.Type == configv1.AWSPlatformType && subnetsAWSEnabled { var ( wantSubnets, haveSubnets *operatorv1.AWSSubnets paramsFieldName string ) - switch getAWSLoadBalancerTypeInStatus(ic) { + switch getAWSLBTypeFromServiceAnnotation(service) { case operatorv1.AWSNetworkLoadBalancer: if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil { wantSubnets = nlbParams.Subnets @@ -835,7 +848,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service } } - if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeInStatus(ic) == operatorv1.AWSNetworkLoadBalancer { + if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLBTypeFromServiceAnnotation(service) == operatorv1.AWSNetworkLoadBalancer { var ( wantEIPAllocations, haveEIPAllocations []operatorv1.EIPAllocation ) @@ -966,6 +979,21 @@ func loadBalancerSourceRangesMatch(ic *operatorv1.IngressController, current *co return fmt.Errorf("You have manually edited an operator-managed object. You must revert your modifications by removing the Spec.LoadBalancerSourceRanges field of LoadBalancer-typed service %q. You can use the new AllowedSourceRanges API field on the ingresscontroller to configure this setting instead.", current.Name) } +// getAWSLBTypeFromServiceAnnotation gets the effective load balancer type by looking at the +// service.beta.kubernetes.io/aws-load-balancer-type annotation of the LoadBalancer-type Service. +// If the annotation isn't specified, the function returns the default of Classic. +func getAWSLBTypeFromServiceAnnotation(service *corev1.Service) operatorv1.AWSLoadBalancerType { + if service == nil { + return "" + } + + if a, ok := service.Annotations[AWSLBTypeAnnotation]; ok && a == AWSNLBAnnotation { + return operatorv1.AWSNetworkLoadBalancer + } + + return operatorv1.AWSClassicLoadBalancer +} + // getSubnetsFromServiceAnnotation gets the effective subnets by looking at the // service.beta.kubernetes.io/aws-load-balancer-subnets annotation of the LoadBalancer-type Service. // If no subnets are specified in the annotation, this function returns nil. @@ -1035,6 +1063,12 @@ func serviceEIPAllocationsEqual(a, b *corev1.Service) bool { return awsEIPAllocationsEqual(getEIPAllocationsFromServiceAnnotation(a), getEIPAllocationsFromServiceAnnotation(b)) } +// serviceAWSLBTypeEqual compares the AWS load balancer type annotations +// on the two services to determine if they are equivalent. +func serviceAWSLBTypeEqual(a, b *corev1.Service) bool { + return getAWSLBTypeFromServiceAnnotation(a) == getAWSLBTypeFromServiceAnnotation(b) +} + // awsEIPAllocationsEqual compares two AWSEIPAllocation slices and returns a boolean // whether they are equal are not. The order of the EIP Allocations are ignored. func awsEIPAllocationsEqual(eipAllocations1, eipAllocations2 []operatorv1.EIPAllocation) bool { @@ -1180,6 +1214,18 @@ func JoinAWSEIPAllocations(eipAllocations []operatorv1.EIPAllocation, sep string return buffer.String() } +// getAWSLoadBalancerTypeInSpec gets the AWS Load Balancer Type reported in the status. +// If nothing is configured, then it returns the default of Classic. +func getAWSLoadBalancerTypeInSpec(ic *operatorv1.IngressController) operatorv1.AWSLoadBalancerType { + if ic.Spec.EndpointPublishingStrategy != nil && + ic.Spec.EndpointPublishingStrategy.LoadBalancer != nil && + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil && + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil { + return ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type + } + return operatorv1.AWSClassicLoadBalancer +} + // getAWSLoadBalancerTypeInStatus gets the AWS Load Balancer Type reported in the status. func getAWSLoadBalancerTypeInStatus(ic *operatorv1.IngressController) operatorv1.AWSLoadBalancerType { if ic.Status.EndpointPublishingStrategy != nil && diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index cb0f61ea4..14868cb42 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -1170,7 +1170,14 @@ func Test_loadBalancerServiceChanged(t *testing.T) { svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-subnets"] = "foo-subnet" svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-type"] = "NLB" }, - expect: true, + expect: false, + }, + { + description: "if the service.beta.kubernetes.io/aws-load-balancer-type is added", + mutate: func(svc *corev1.Service) { + svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-type"] = "NLB" + }, + expect: false, }, { description: "if the service.beta.kubernetes.io/aws-load-balancer-eip-allocations annotation added", diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 25abb25ff..3ce193f18 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -655,8 +655,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, }, } - - loadBalancerIngressControllerWithAWSSubnets := func(lbType operatorv1.AWSLoadBalancerType, subnetSpec *operatorv1.AWSSubnets, subnetStatus *operatorv1.AWSSubnets) *operatorv1.IngressController { + loadBalancerIngressControllerWithLBType := func(lbType operatorv1.AWSLoadBalancerType) *operatorv1.IngressController { eps := &operatorv1.EndpointPublishingStrategy{ Type: operatorv1.LoadBalancerServiceStrategyType, LoadBalancer: &operatorv1.LoadBalancerStrategy{ @@ -677,6 +676,11 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { EndpointPublishingStrategy: eps.DeepCopy(), }, } + return ic + } + loadBalancerIngressControllerWithAWSSubnets := func(lbType operatorv1.AWSLoadBalancerType, subnetSpec *operatorv1.AWSSubnets, subnetStatus *operatorv1.AWSSubnets) *operatorv1.IngressController { + ic := loadBalancerIngressControllerWithLBType(lbType) + switch lbType { case operatorv1.AWSNetworkLoadBalancer: ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{ @@ -763,6 +767,13 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, } lbService := &corev1.Service{} + lbServiceWithNLB := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + AWSLBTypeAnnotation: AWSNLBAnnotation, + }, + }, + } lbServiceWithInternalScopeOnAWS := &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ @@ -884,7 +895,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -896,7 +907,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, &operatorv1.AWSSubnets{}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -910,7 +921,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: false, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -924,7 +935,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -938,7 +949,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { IDs: []operatorv1.AWSSubnetID{"subnet-12345"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -956,7 +967,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Names: []operatorv1.AWSSubnetName{"name-12345"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -992,7 +1003,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Names: []operatorv1.AWSSubnetName{"name-67890", "name-12345"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1010,7 +1021,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Names: []operatorv1.AWSSubnetName{"name-67890", "name-12345", "name-54321"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -1145,7 +1156,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1156,7 +1167,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, []operatorv1.EIPAllocation{}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1167,7 +1178,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: false, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1178,7 +1189,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -1189,7 +1200,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -1200,7 +1211,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1211,7 +1222,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, []operatorv1.EIPAllocation{"eipalloc-aaaaaaaaaaaaaaaaa", "eipalloc-bbbbbbbbbbbbbbbbb"}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -1222,7 +1233,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, []operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-xxxxxxxxxxxxxxxxx"}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1233,11 +1244,32 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-zzzzzzzzzzzzz"}, []operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-xxxxxxxxxxxxxxxxx"}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, }, + { + name: "LBType Empty LoadBalancerService (default Classic), AWS LBType Classic", + ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSClassicLoadBalancer), + service: lbService, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "LBType Classic LoadBalancerService, AWS LBType NLB", + ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSNetworkLoadBalancer), + service: lbService, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionTrue, + }, + { + name: "LBType NLB LoadBalancerService, AWS LBType Classic", + ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSClassicLoadBalancer), + service: lbServiceWithNLB, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionTrue, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 08cd3a06d..3e7421ba0 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -1291,19 +1291,23 @@ func TestInternalLoadBalancerGlobalAccessGCP(t *testing.T) { } } +// TestAWSLBTypeChange verifies that process of changing the IngressController's +// load balancer type, ensuring that it requires the associated service to be deleted +// and recreated for the change to take effect. func TestAWSLBTypeChange(t *testing.T) { t.Parallel() - if infraConfig.Status.Platform != "AWS" { - t.Skipf("test skipped on platform %q", infraConfig.Status.Platform) + if infraConfig.Status.PlatformStatus.Type != configv1.AWSPlatformType { + t.Skipf("test skipped on platform %q", infraConfig.Status.PlatformStatus.Type) } - name := types.NamespacedName{Namespace: operatorNamespace, Name: "awslb"} + name := types.NamespacedName{Namespace: operatorNamespace, Name: "aws-lb-type-change"} ic := newLoadBalancerController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) ic.Spec.EndpointPublishingStrategy.LoadBalancer = &operatorv1.LoadBalancerStrategy{ Scope: operatorv1.ExternalLoadBalancer, } - if err := kclient.Create(context.TODO(), ic); err != nil { + t.Logf("creating ingresscontroller %q without specifying LB type", ic.Name) + if err := kclient.Create(context.Background(), ic); err != nil { t.Fatalf("failed to create ingresscontroller: %v", err) } defer assertIngressControllerDeleted(t, kclient, ic) @@ -1313,53 +1317,81 @@ func TestAWSLBTypeChange(t *testing.T) { t.Fatalf("failed to observe expected conditions: %v", err) } - lbService := &corev1.Service{} - if err := kclient.Get(context.TODO(), controller.LoadBalancerServiceName(ic), lbService); err != nil { - t.Fatalf("failed to get LoadBalancer service: %v", err) + // LB Annotation should be empty by default (meaning CLB). + waitForLBAnnotation(t, ic, ingresscontroller.AWSLBTypeAnnotation, false, "") + + // Update IngressController to use NLB. + t.Logf("updating ingresscontroller %q to change the LB type to NLB", ic.Name) + if err := updateIngressControllerWithRetryOnConflict(t, name, 5*time.Minute, func(ic *operatorv1.IngressController) { + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{ + Type: operatorv1.AWSLoadBalancerProvider, + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: operatorv1.AWSNetworkLoadBalancer, + }, + } + }); err != nil { + t.Fatalf("failed to update ingresscontroller: %v", err) } - if v := lbService.Annotations[ingresscontroller.AWSLBTypeAnnotation]; len(v) != 0 { - t.Fatalf("load balancer service has unexpected %s=%s annotation", ingresscontroller.AWSLBTypeAnnotation, v) + + // Effectuate the LB Type change. + effectuateIngressControllerLBType(t, ic, operatorv1.AWSNetworkLoadBalancer, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...) + + // Now, update the IngressController switch back to CLB, but let's use the + // auto-delete-load-balancer annotation, so we don't have to manually delete the service. + t.Logf("updating ingresscontroller %q to use CLB while using the auto-delete-load-balancer annotation", ic.Name) + if err := updateIngressControllerWithRetryOnConflict(t, name, 5*time.Minute, func(ic *operatorv1.IngressController) { + if ic.Annotations == nil { + ic.Annotations = map[string]string{} + } + ic.Annotations["ingress.operator.openshift.io/auto-delete-load-balancer"] = "" + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type = operatorv1.AWSClassicLoadBalancer + }); err != nil { + t.Fatalf("failed to update ingresscontroller: %v", err) } - if err := kclient.Get(context.TODO(), name, ic); err != nil { - t.Fatalf("failed to get ingresscontroller %s: %v", name, err) + // Verify the LB type annotation is removed on the service (default to CLB). + waitForLBAnnotation(t, ic, ingresscontroller.AWSLBTypeAnnotation, false, "") + + // Expect the load balancer to provision successfully. + if err := waitForIngressControllerCondition(t, kclient, 10*time.Minute, name, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) } +} - pp := &operatorv1.ProviderLoadBalancerParameters{ - Type: operatorv1.AWSLoadBalancerProvider, - AWS: &operatorv1.AWSLoadBalancerParameters{ - Type: operatorv1.AWSNetworkLoadBalancer, - }, +// effectuateIngressControllerLBType manually effectuates updated IngressController LB type by +// confirming IngressController is in a progressing state, deleting the service, and waiting for +// the expected LB type annotation to appear on the service. It waits for the provided operator +// conditions after the LB type has been effectuated. +func effectuateIngressControllerLBType(t *testing.T, ic *operatorv1.IngressController, expectedLBType operatorv1.AWSLoadBalancerType, expectedOperatorConditions ...operatorv1.OperatorCondition) { + t.Helper() + t.Logf("effectuating LB type for IngressController %s", ic.Name) + icName := types.NamespacedName{Name: ic.Name, Namespace: ic.Namespace} + progressingTrue := operatorv1.OperatorCondition{ + Type: ingresscontroller.IngressControllerLoadBalancerProgressingConditionType, + Status: operatorv1.ConditionTrue, + } + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, progressingTrue); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) } - ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters = pp - if err := kclient.Update(context.TODO(), ic); err != nil { - t.Fatalf("failed to update ingresscontroller: %v", err) + // Delete and recreate the IngressController service to effectuate. + t.Logf("recreating the service to effectuate the LB type: %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) } - // Wait for the load balancer and DNS to be ready. - if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, availableConditionsForIngressControllerWithLoadBalancer...); err != nil { - t.Fatalf("failed to observe expected conditions: %v", err) + // Verify we get the expected LB type annotation on the service. + if expectedLBType == operatorv1.AWSClassicLoadBalancer { + waitForLBAnnotation(t, ic, ingresscontroller.AWSLBTypeAnnotation, false, "") + } else if expectedLBType == operatorv1.AWSNetworkLoadBalancer { + waitForLBAnnotation(t, ic, ingresscontroller.AWSLBTypeAnnotation, true, ingresscontroller.AWSNLBAnnotation) + } else { + t.Fatalf("unsupported LB type: %s", expectedLBType) } - err := wait.PollImmediate(5*time.Second, 5*time.Minute, func() (bool, error) { - service := &corev1.Service{} - if err := kclient.Get(context.TODO(), controller.LoadBalancerServiceName(ic), service); err != nil { - t.Logf("failed to get service %s: %v", controller.LoadBalancerServiceName(ic), err) - return false, nil - } - if actual, ok := service.Annotations[ingresscontroller.AWSLBTypeAnnotation]; !ok { - t.Logf("load balancer has no %q annotation: %v", ingresscontroller.AWSLBTypeAnnotation, service.Annotations) - return false, nil - } else if actual != ingresscontroller.AWSNLBAnnotation { - t.Logf("expected %s=%s, found %s=%s", ingresscontroller.AWSLBTypeAnnotation, ingresscontroller.AWSNLBAnnotation, - ingresscontroller.AWSLBTypeAnnotation, actual) - return false, nil - } - return true, nil - }) - if err != nil { - t.Fatalf("timed out waiting for the service LB type annotation to be updated: %v", err) + // Expect the load balancer to provision successfully with the new LB type. + if err := waitForIngressControllerCondition(t, kclient, 10*time.Minute, icName, expectedOperatorConditions...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) } }