From 8a299a91cbb93ee1f6bbf3ec35c9cd2c14e6318b 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 | 266 ++++++++++++------ .../ingress/load_balancer_service_test.go | 55 ++-- .../controller/ingress/status_test.go | 70 +++-- test/e2e/operator_test.go | 112 +++++--- 4 files changed, 338 insertions(+), 165 deletions(-) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 7cef35dda3..23845cbc57 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,7 +216,7 @@ var ( configv1.GCPPlatformType: {}, } - // managedLoadBalancerServiceAnnotations is a set of annotation keys for + // autoEffectuatedLoadBalancerServiceAnnotations is a set of annotation keys for // annotations that the operator manages for LoadBalancer-type services. // The operator preserves all other annotations. // @@ -229,32 +227,30 @@ 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( + autoEffectuatedLoadBalancerServiceAnnotations = func() []string { + result := []string{ // AWS LB health check interval annotation (see // ). awsLBHealthCheckIntervalAnnotation, // AWS connection idle timeout annotation. awsELBConnectionIdleTimeoutAnnotation, + // 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, // GCP Global Access internal Load Balancer annotation // (see ). GCPGlobalAccessAnnotation, // 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, - ) + } // Azure and GCP support switching between internal and external // scope by changing the annotation, so the operator manages the @@ -265,15 +261,128 @@ 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 }() + + manualEffectuatedLoadBalancerAnnotations = func() map[configv1.PlatformType][]string { + result := map[configv1.PlatformType][]string{ + configv1.AWSPlatformType: { + // AWS load balancer type annotation to set either CLB/ELB or NLB. + // LB Type is not auto effectuated, but is managed. + AWSLBTypeAnnotation, + + awsLBSubnetsAnnotation, + + awsEIPAllocationsAnnotation, + }, + } + 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 + }() + + //// autoEffectuatedLoadBalancerServiceAnnotations is a set of annotation keys for + //// annotations that the operator manages (reconciles) for LoadBalancer-type services. + //// The operator preserves all other annotations. + //// + //// autoEffectuatedLoadBalancerServiceAnnotations MUST be a superset of autoEffectuatedLoadBalancerAnnotations. + //// Annotations included in autoEffectuatedLoadBalancerServiceAnnotations, but not in + //// autoEffectuatedLoadBalancerAnnotations indicate annotations that will ALWAYS be + //// reconciled by the operator, but will NOT be auto effectuated (requiring manual action + //// from the user). This is crucial when the operator needs to implement a desired change, + //// but hasn't applied it yet. During this time, the operator will STILL reconcile + //// the annotation to reflect the un-applied (previous) value. + //// + //// Be careful when adding annotation keys to this set or autoEffectuatedLoadBalancerAnnotations. + //// If a new release of the operator starts managing an annotation that it previously + //// ignored, it could stomp annotations that the user has set when the + //// user upgrades the operator to the new release (see + //// ). In order to + //// avoid problems, make sure the previous release blocks upgrades when + //// the user has modified an annotation that the new release manages. + //autoEffectuatedLoadBalancerServiceAnnotations = func() sets.String { + // result := autoEffectuatedLoadBalancerAnnotations.Clone() + // result.Insert( + // // AWS load balancer type annotation to set either CLB/ELB or NLB. + // // LB Type is not auto effectuated, but is managed. + // AWSLBTypeAnnotation, + // ) + // + // return result + //}() + // + //// autoEffectuatedLoadBalancerAnnotations is a set of annotation keys for + //// annotations that the operator will auto effectuate. "Auto effectuation" + //// means the operator will immediately update the annotation when it is required. + //// + //// In contrast, "manual effectuation" means the operator will not make the change + //// right away. Instead, it will set a Progressing=True status condition with a + //// message explaining how to apply the change manually (e.g., deleting the service). + //// + //// Annotations in this set are also added to autoEffectuatedLoadBalancerServiceAnnotations + //// because an annotation cannot be auto effectuated unless it is managed. + //autoEffectuatedLoadBalancerAnnotations = func() sets.String { + // result := sets.NewString( + // // AWS LB health check interval annotation (see + // // ). + // awsLBHealthCheckIntervalAnnotation, + // // AWS connection idle timeout annotation. + // awsELBConnectionIdleTimeoutAnnotation, + // // GCP Global Access internal Load Balancer annotation + // // (see ). + // GCPGlobalAccessAnnotation, + // // local-with-fallback annotation for kube-proxy (see + // // ). + // localWithFallbackAnnotation, + // // 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, + // ) + // + // // Azure and GCP support switching between internal and external + // // scope by changing the annotation, so the operator manages the + // // corresponding load-balancer scope annotations for these + // // platforms. Other platforms require deleting and recreating + // // the service, so the operator doesn't update the annotations + // // that specify load-balancer scope for those platforms. See + // // . + // for platform := range platformsWithMutableScope { + // for name := range InternalLBAnnotations[platform] { + // result.Insert(name) + // } + // for name := range externalLBAnnotations[platform] { + // result.Insert(name) + // } + // } + // + // return result + //}() ) // ensureLoadBalancerService creates an LB service if one is desired but absent. @@ -598,8 +707,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, manualEffectuatedLoadBalancerAnnotations[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 { @@ -624,46 +733,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) { @@ -675,7 +744,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, autoEffectuatedLoadBalancerServiceAnnotations) // If spec.loadBalancerSourceRanges is nonempty on the service, that // means that allowedSourceRanges is nonempty on the ingresscontroller, @@ -703,18 +772,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() @@ -723,7 +792,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) { @@ -733,7 +802,7 @@ func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, an } } - return true, updated + return true, changedAnnotations, updated } // IsServiceInternal returns a Boolean indicating whether the provided service @@ -752,8 +821,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(autoEffectuatedLoadBalancerServiceAnnotations, awsLBAdditionalResourceTags) return loadBalancerServiceAnnotationsChanged(current, expected, ignoredAnnotations) } @@ -773,7 +842,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) @@ -799,12 +868,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 +916,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 +1047,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. @@ -1025,16 +1121,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 { @@ -1180,6 +1266,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 cb0f61ea47..fe390e6214 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 25abb25ff8..3ce193f187 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 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) } }