diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 7cef35dda3..b328c42a2f 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. @@ -598,8 +644,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 { @@ -624,46 +670,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 +681,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, @@ -703,18 +709,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 +729,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 +739,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 +758,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) } @@ -773,7 +779,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 +805,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, haveLBType) + 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 +853,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 +984,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 +1058,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 +1203,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) } }