From c6fd8b6ec5585de9b379d23a3fe4964c6154dc67 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 added to the `managedAtServiceCreationLBServiceAnnotations` map along with other annotations that require service deletion. --- .../ingress/load_balancer_service.go | 199 ++++++++++-------- .../ingress/load_balancer_service_test.go | 55 +++-- .../controller/ingress/status_test.go | 41 +++- test/e2e/operator_test.go | 112 ++++++---- 4 files changed, 249 insertions(+), 158 deletions(-) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 9b71c18b2d..a9dc5b6448 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -23,8 +23,6 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kerrors "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/apimachinery/pkg/util/sets" - crclient "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -33,19 +31,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). @@ -218,9 +216,11 @@ var ( configv1.GCPPlatformType: {}, } - // managedLoadBalancerServiceAnnotations is a set of annotation keys for - // annotations that the operator manages for LoadBalancer-type services. - // The operator preserves all other annotations. + // managedLBServiceAnnotations is a set of annotation keys for + // annotations that the operator fully manages for LoadBalancer-type services. + // The operator preserves all other annotations. Any desired updates to + // these annotations are immediately applied to the service without + // requiring service deletion. // // Be careful when adding annotation keys to this set. If a new release // of the operator starts managing an annotation that it previously @@ -229,8 +229,8 @@ var ( // ). In order to // avoid problems, make sure the previous release blocks upgrades when // the user has modified an annotation that the new release manages. - managedLoadBalancerServiceAnnotations = func() sets.String { - result := sets.NewString( + managedLBServiceAnnotations = func() []string { + result := []string{ // AWS LB health check interval annotation (see // ). awsLBHealthCheckIntervalAnnotation, @@ -242,19 +242,17 @@ 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. - // - // https://kubernetes.io/docs/concepts/services-networking/service/#proxy-protocol-support-on-aws - awsLBProxyProtocolAnnotation, // iksLBEnableFeaturesAnnotation annotation used on a service to enable features // on the load balancer. // // https://cloud.ibm.com/docs/containers?topic=containers-vpc-lbaas iksLBEnableFeaturesAnnotation, - ) + // 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 + awsLBProxyProtocolAnnotation, + } // Azure and GCP support switching between internal and external // scope by changing the annotation, so the operator manages the @@ -265,15 +263,63 @@ var ( // . for platform := range platformsWithMutableScope { for name := range InternalLBAnnotations[platform] { - result.Insert(name) + result = append(result, name) } for name := range externalLBAnnotations[platform] { - result.Insert(name) + result = append(result, name) } } return result }() + + // managedAtServiceCreationLBServiceAnnotations maps platform to annotation keys + // that the operator manages, but only during service creation. This means that + // the operator will not apply a desired annotation update right away. + // Instead, it will set a Progressing=True status condition with a message explaining + // how to apply the change manually (e.g., by deleting the service so that it can be + // recreated). + // + // Note that the ingress.operator.openshift.io/auto-delete-load-balancer annotation enables + // these annotations to be applied immediately by automatically deleting and recreating the + // service. + managedAtServiceCreationLBServiceAnnotations = func() map[configv1.PlatformType][]string { + result := map[configv1.PlatformType][]string{ + configv1.AWSPlatformType: { + // Annotation to set either CLB/ELB or NLB. + // This annotation was previously fully managed (https://issues.redhat.com/browse/NE-865), + // but now required service recreation due to issues with AWS leaking resources when updating + // LB Type without recreating the service (see https://issues.redhat.com/browse/OCPBUGS-16728). + AWSLBTypeAnnotation, + // Annotation for configuring load balancer subnets. + // This requires service recreation because NLBs do not support updates to subnets, + // the operator cannot detect errors if the subnets are invalid, and to prevent + // compatibility issues during upgrades since this annotation was previously unmanaged. + awsLBSubnetsAnnotation, + // Annotation for configuring load balancer EIPs. + // This requires service recreation to prevent compatibility issues during upgrades since + // this annotation was previously unmanaged. + awsEIPAllocationsAnnotation, + }, + } + + // Add the annotations that do NOT support mutable scope. + for platform, annotation := range InternalLBAnnotations { + if _, ok := platformsWithMutableScope[platform]; !ok { + for name := range annotation { + result[platform] = append(result[platform], name) + } + } + } + for platform, annotation := range externalLBAnnotations { + if _, ok := platformsWithMutableScope[platform]; !ok { + for name := range annotation { + result[platform] = append(result[platform], name) + } + } + } + return result + }() ) // ensureLoadBalancerService creates an LB service if one is desired but absent. @@ -593,8 +639,8 @@ func (r *reconciler) deleteLoadBalancerService(service *corev1.Service, options // updateLoadBalancerService updates a load balancer service. Returns a Boolean // 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) + if shouldRecreateLB, changedAnnotations, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedAtServiceCreationLBServiceAnnotations[platform.Type]); shouldRecreateLB && autoDeleteLB { + log.Info("deleting and recreating the load balancer", "annotations", changedAnnotations, "namespace", desired.Namespace, "name", desired.Name) foreground := metav1.DeletePropagationForeground deleteOptions := crclient.DeleteOptions{PropagationPolicy: &foreground} if err := r.deleteLoadBalancerService(current, &deleteOptions); err != nil { @@ -619,46 +665,6 @@ func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service, return true, nil } -// scopeEqual returns true if the scope is the same between the two given -// services and false if the scope is different. -func scopeEqual(a, b *corev1.Service, platform *configv1.PlatformStatus) bool { - aAnnotations := a.Annotations - if aAnnotations == nil { - aAnnotations = map[string]string{} - } - bAnnotations := b.Annotations - if bAnnotations == nil { - bAnnotations = map[string]string{} - } - for name := range InternalLBAnnotations[platform.Type] { - if aAnnotations[name] != bAnnotations[name] { - return false - } - } - for name := range externalLBAnnotations[platform.Type] { - if aAnnotations[name] != bAnnotations[name] { - return false - } - } - return true -} - -// shouldRecreateLoadBalancer determines whether a load balancer needs to be -// recreated and returns the reason for its recreation. -func shouldRecreateLoadBalancer(current, desired *corev1.Service, platform *configv1.PlatformStatus) (bool, string) { - _, platformHasMutableScope := platformsWithMutableScope[platform.Type] - if !platformHasMutableScope && !scopeEqual(current, desired, platform) { - return true, "its scope changed" - } - if platform.Type == configv1.AWSPlatformType && !serviceSubnetsEqual(current, desired) { - return true, "its subnets changed" - } - if platform.Type == configv1.AWSPlatformType && !serviceEIPAllocationsEqual(current, desired) { - return true, "its eipAllocations changed" - } - return false, "" -} - // loadBalancerServiceChanged checks if the current load balancer service // matches the expected and if not returns an updated one. func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev1.Service) { @@ -670,7 +676,7 @@ 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) + changed, _, updated := loadBalancerServiceAnnotationsChanged(current, expected, managedLBServiceAnnotations) // If spec.loadBalancerSourceRanges is nonempty on the service, that // means that allowedSourceRanges is nonempty on the ingresscontroller, @@ -698,18 +704,18 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev // loadBalancerServiceAnnotationsChanged checks if the annotations on the expected Service // match the ones on the current Service. -func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, annotations sets.String) (bool, *corev1.Service) { - changed := false - for annotation := range annotations { +func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, annotations []string) (bool, []string, *corev1.Service) { + var changedAnnotations []string + for _, annotation := range annotations { currentVal, have := current.Annotations[annotation] expectedVal, want := expected.Annotations[annotation] if (want && (!have || currentVal != expectedVal)) || (have && !want) { - changed = true + changedAnnotations = append(changedAnnotations, annotation) break } } - if !changed { - return false, nil + if len(changedAnnotations) == 0 { + return false, nil, nil } updated := current.DeepCopy() @@ -718,7 +724,7 @@ func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, an updated.Annotations = map[string]string{} } - for annotation := range annotations { + for _, annotation := range annotations { currentVal, have := current.Annotations[annotation] expectedVal, want := expected.Annotations[annotation] if want && (!have || currentVal != expectedVal) { @@ -728,7 +734,7 @@ func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, an } } - return true, updated + return true, changedAnnotations, updated } // IsServiceInternal returns a Boolean indicating whether the provided service @@ -747,8 +753,8 @@ func IsServiceInternal(service *corev1.Service) bool { } // loadBalancerServiceTagsModified verifies that none of the managedAnnotations have been changed and also the AWS tags annotation -func loadBalancerServiceTagsModified(current, expected *corev1.Service) (bool, *corev1.Service) { - ignoredAnnotations := managedLoadBalancerServiceAnnotations.Union(sets.NewString(awsLBAdditionalResourceTags)) +func loadBalancerServiceTagsModified(current, expected *corev1.Service) (bool, []string, *corev1.Service) { + ignoredAnnotations := append(managedLBServiceAnnotations, awsLBAdditionalResourceTags) return loadBalancerServiceAnnotationsChanged(current, expected, ignoredAnnotations) } @@ -772,7 +778,7 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme return nil } - changed, updated := loadBalancerServiceTagsModified(current, desired) + changed, _, updated := loadBalancerServiceTagsModified(current, desired) if changed { diff := cmp.Diff(current, updated, cmpopts.EquateEmpty()) return fmt.Errorf("load balancer service has been modified; changes must be reverted before upgrading: %s", diff) @@ -781,6 +787,19 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme return nil } +// effectuateMessage returns a message describing how to effectuate a +// change that requires the service to be deleted. +func effectuateMessage(changedMsg, ocPatchRevertCmd string, service *corev1.Service) string { + return fmt.Sprintf( + "%[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`. "+ + "Direct updates to the service annotations are not supported.", + changedMsg, service.Namespace, service.Name, ocPatchRevertCmd, + ) +} + // loadBalancerServiceIsProgressing returns an error value indicating if the // load balancer service is in progressing status. func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) error { @@ -791,13 +810,27 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service haveScope = operatorv1.InternalLoadBalancer } if wantScope != haveScope { - err := fmt.Errorf("The IngressController scope was changed from %q to %q.", haveScope, wantScope) + changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope) + err := fmt.Errorf(changedMsg) if _, ok := platformsWithMutableScope[platform.Type]; !ok { - 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 from the old one's. Alternatively, you can revert the change to the IngressController: `oc -n openshift-ingress-operator patch ingresscontrollers/%[4]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[5]s\"}}}}'`", err.Error(), service.Namespace, service.Name, ic.Name, haveScope) + ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[2]s\"}}}}'", ic.Name, haveScope) + err = fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) } errs = append(errs, err) } + if platform.Type == configv1.AWSPlatformType { + wantLBType := getAWSLoadBalancerTypeInSpec(ic) + haveLBType := getAWSLoadBalancerTypeFromServiceAnnotation(service) + + if wantLBType != haveLBType { + changedMsg := fmt.Sprintf("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, haveLBType) + err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) + errs = append(errs, err) + } + } + if platform.Type == configv1.AWSPlatformType && subnetsAWSEnabled { var ( wantSubnets, haveSubnets *operatorv1.AWSSubnets @@ -829,7 +862,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service wantSubnetsPrettyJson := convertAWSSubnetListToPatchJson(wantSubnets, "{}", "[]") changedMsg := fmt.Sprintf("The IngressController subnets were changed from %q to %q.", haveSubnetsPrettyJson, wantSubnetsPrettyJson) 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\":{\"subnets\":%[4]s}}}}}}}'", ic.Name, getAWSLoadBalancerTypeInStatus(ic), paramsFieldName, haveSubnetsPatchJson) - 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) + err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) errs = append(errs, err) } } @@ -852,7 +885,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service wantEIPAllocationsPrettyJson := convertAWSEIPAllocationsListToPatchJson(wantEIPAllocations, "[]") changedMsg := fmt.Sprintf("The IngressController eipAllocations were changed from %q to %q.", haveEIPAllocationsPrettyJson, wantEIPAllocationsPrettyJson) 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) + err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) errs = append(errs, err) } } @@ -1039,16 +1072,6 @@ func getEIPAllocationsFromServiceAnnotation(service *corev1.Service) []operatorv return awsEIPAllocations } -// serviceSubnetsEqual compares the subnet annotations on two services to determine if they are equivalent, -// ignoring the order of the subnets. -func serviceSubnetsEqual(a, b *corev1.Service) bool { - return awsSubnetsEqual(getSubnetsFromServiceAnnotation(a), getSubnetsFromServiceAnnotation(b)) -} - -func serviceEIPAllocationsEqual(a, b *corev1.Service) bool { - return awsEIPAllocationsEqual(getEIPAllocationsFromServiceAnnotation(a), getEIPAllocationsFromServiceAnnotation(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 { diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index e3467a86b4..7ec2127947 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -16,7 +16,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" - "k8s.io/apimachinery/pkg/util/sets" ) func Test_desiredLoadBalancerService(t *testing.T) { @@ -1170,7 +1169,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", @@ -1240,32 +1246,33 @@ func Test_loadBalancerServiceChanged(t *testing.T) { // loadBalancerServiceAnnotationsChanged behaves correctly. func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { testCases := []struct { - description string - mutate func(*corev1.Service) - currentAnnotations map[string]string - expectedAnnotations map[string]string - managedAnnotations sets.String - expect bool + description string + mutate func(*corev1.Service) + currentAnnotations map[string]string + expectedAnnotations map[string]string + managedAnnotations []string + expect bool + expectChangedAnnotations []string }{ { description: "if current and expected annotations are both empty", currentAnnotations: map[string]string{}, expectedAnnotations: map[string]string{}, - managedAnnotations: sets.NewString("foo"), + managedAnnotations: []string{"foo"}, expect: false, }, { description: "if current annotations is nil and expected annotations is empty", currentAnnotations: nil, expectedAnnotations: map[string]string{}, - managedAnnotations: sets.NewString("foo"), + managedAnnotations: []string{"foo"}, expect: false, }, { description: "if current annotations is empty and expected annotations is nil", currentAnnotations: map[string]string{}, expectedAnnotations: nil, - managedAnnotations: sets.NewString("foo"), + managedAnnotations: []string{"foo"}, expect: false, }, { @@ -1277,7 +1284,7 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { "foo": "bar", "baz": "quux", }, - managedAnnotations: sets.NewString("foo"), + managedAnnotations: []string{"foo"}, expect: false, }, { @@ -1286,8 +1293,9 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { expectedAnnotations: map[string]string{ "foo": "bar", }, - managedAnnotations: sets.NewString("foo"), - expect: true, + managedAnnotations: []string{"foo"}, + expect: true, + expectChangedAnnotations: []string{"foo"}, }, { description: "if a managed annotation is updated", @@ -1297,17 +1305,19 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { expectedAnnotations: map[string]string{ "foo": "baz", }, - managedAnnotations: sets.NewString("foo"), - expect: true, + managedAnnotations: []string{"foo"}, + expect: true, + expectChangedAnnotations: []string{"foo"}, }, { description: "if a managed annotation is deleted", currentAnnotations: map[string]string{ "foo": "bar", }, - expectedAnnotations: map[string]string{}, - managedAnnotations: sets.NewString("foo"), - expect: true, + expectedAnnotations: map[string]string{}, + managedAnnotations: []string{"foo"}, + expect: true, + expectChangedAnnotations: []string{"foo"}, }, } @@ -1323,13 +1333,14 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { Annotations: tc.expectedAnnotations, }, } - if changed, updated := loadBalancerServiceAnnotationsChanged(¤t, &expected, tc.managedAnnotations); changed != tc.expect { + if changed, changedAnnotations, updated := loadBalancerServiceAnnotationsChanged(¤t, &expected, tc.managedAnnotations); changed != tc.expect { t.Errorf("expected loadBalancerServiceAnnotationsChanged to be %t, got %t", tc.expect, changed) } else if changed { - if updatedChanged, _ := loadBalancerServiceAnnotationsChanged(¤t, updated, tc.managedAnnotations); !updatedChanged { + assert.Equal(t, tc.expectChangedAnnotations, changedAnnotations) + if updatedChanged, _, _ := loadBalancerServiceAnnotationsChanged(¤t, updated, tc.managedAnnotations); !updatedChanged { t.Error("loadBalancerServiceAnnotationsChanged reported changes but did not make any update") } - if changedAgain, _ := loadBalancerServiceAnnotationsChanged(&expected, updated, tc.managedAnnotations); changedAgain { + if changedAgain, _, _ := loadBalancerServiceAnnotationsChanged(&expected, updated, tc.managedAnnotations); changedAgain { t.Error("loadBalancerServiceAnnotationsChanged does not behave as a fixed point function") } } diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 1457141efb..7e9e29957d 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{ @@ -891,7 +895,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -903,7 +907,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, &operatorv1.AWSSubnets{}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -917,7 +921,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: false, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1188,7 +1192,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1199,7 +1203,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, []operatorv1.EIPAllocation{}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1210,7 +1214,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, @@ -1303,6 +1307,27 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, }, + { + 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 08cd3a06d2..3e7421ba03 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) } }