diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 9e67518f2..71f4ac4bd 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -440,6 +440,10 @@ func setDefaultDomain(ic *operatorv1.IngressController, ingressConfig *configv1. } func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStatus *configv1.PlatformStatus, domainMatchesBaseDomain bool, ingressConfig *configv1.Ingress, alreadyAdmitted bool) bool { + // WARNING: setDefaultPublishingStrategy follows an outdated pattern of using spec and status. It uses the status + // to represent the *desired* state (spec + defaulting), but per Kubernetes standards, the status should reflect + // the *actual* state. As a result, this function is considered legacy. For new API fields, use spec as the desired + // state and syncIngressControllerStatus to populate the status with the actual state. effectiveStrategy := ic.Spec.EndpointPublishingStrategy.DeepCopy() if effectiveStrategy == nil { var strategyType operatorv1.EndpointPublishingStrategyType @@ -577,7 +581,6 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat changed = true } if statusLB.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer { - statusLB.ProviderParameters.AWS.NetworkLoadBalancerParameters = nil if statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters == nil { statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{} } @@ -599,7 +602,6 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat } } if statusLB.ProviderParameters.AWS.Type == operatorv1.AWSNetworkLoadBalancer { - statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters = nil if statusLB.ProviderParameters.AWS.NetworkLoadBalancerParameters == nil { statusLB.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{} } @@ -1106,7 +1108,19 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d haveClientCAConfigmap = true } - haveDepl, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig, haveClientCAConfigmap, clientCAConfigmap, platformStatus, clusterProxyConfig) + _, currentLBService, err := r.currentLoadBalancerService(ci) + if err != nil { + errs = append(errs, fmt.Errorf("failed to get load balancer service for %s: %v", ci.Name, err)) + return utilerrors.NewAggregate(errs) + } + + proxyNeeded, err := IsProxyProtocolNeeded(ci, platformStatus, currentLBService) + if err != nil { + errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err)) + return utilerrors.NewAggregate(errs) + } + + haveDepl, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig, haveClientCAConfigmap, clientCAConfigmap, platformStatus, clusterProxyConfig, proxyNeeded) if err != nil { errs = append(errs, fmt.Errorf("failed to ensure deployment: %v", err)) return utilerrors.NewAggregate(errs) @@ -1125,7 +1139,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d } var wildcardRecord *iov1.DNSRecord - haveLB, lbService, err := r.ensureLoadBalancerService(ci, deploymentRef, platformStatus) + haveLB, lbService, err := r.ensureLoadBalancerService(ci, deploymentRef, platformStatus, proxyNeeded) if err != nil { errs = append(errs, fmt.Errorf("failed to ensure load balancer service for %s: %v", ci.Name, err)) } else { @@ -1220,23 +1234,26 @@ func IsStatusDomainSet(ingress *operatorv1.IngressController) bool { // IsProxyProtocolNeeded checks whether proxy protocol is needed based // upon the given ic and platform. -func IsProxyProtocolNeeded(ic *operatorv1.IngressController, platform *configv1.PlatformStatus) (bool, error) { +func IsProxyProtocolNeeded(ic *operatorv1.IngressController, platform *configv1.PlatformStatus, service *corev1.Service) (bool, error) { if platform == nil { return false, fmt.Errorf("platform status is missing; failed to determine if proxy protocol is needed for %s/%s", ic.Namespace, ic.Name) } switch ic.Status.EndpointPublishingStrategy.Type { case operatorv1.LoadBalancerServiceStrategyType: - // This can really be done for for any external [cloud] LBs that support the proxy protocol. + // This can really be done for any external [cloud] LBs that support the proxy protocol. switch platform.Type { case configv1.AWSPlatformType: - if ic.Status.EndpointPublishingStrategy.LoadBalancer == nil || - ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters == nil || - ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS == nil || - ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.Type == operatorv1.AWSLoadBalancerProvider && - ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer { - return true, nil + // TRICKY: If the service exists, use the LB Type annotation from the service, + // as status will be inaccurate if a LB Type update is pending. + // If the service does not exist, the status WILL be what determines the next LB Type. + var lbType operatorv1.AWSLoadBalancerType + if service != nil { + lbType = getAWSLoadBalancerTypeFromServiceAnnotation(service) + } else { + lbType = getAWSLoadBalancerTypeInStatus(ic) } + return lbType == operatorv1.AWSClassicLoadBalancer, nil case configv1.IBMCloudPlatformType: if ic.Status.EndpointPublishingStrategy.LoadBalancer != nil && ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil && diff --git a/pkg/operator/controller/ingress/controller_test.go b/pkg/operator/controller/ingress/controller_test.go index de931a014..30f460843 100644 --- a/pkg/operator/controller/ingress/controller_test.go +++ b/pkg/operator/controller/ingress/controller_test.go @@ -658,20 +658,6 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { expectedIC: makeIC(spec(elb()), status(elbWithNullParameters())), domainMatchesBaseDomain: true, }, - { - name: "loadbalancer type changed from ELB to NLB, with old ELB parameters removal", - ic: makeIC(spec(nlb()), status(elbWithNullParameters())), - expectedResult: true, - expectedIC: makeIC(spec(nlb()), status(nlbWithNullParameters())), - domainMatchesBaseDomain: true, - }, - { - name: "loadbalancer type changed from NLB to ELB, with old NLB parameters removal", - ic: makeIC(spec(elb()), status(nlbWithNullParameters())), - expectedResult: true, - expectedIC: makeIC(spec(elb()), status(elbWithNullParameters())), - domainMatchesBaseDomain: true, - }, { name: "loadbalancer type changed from NLB to unset, with Classic LB as default", ic: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())), @@ -1471,11 +1457,20 @@ func Test_IsProxyProtocolNeeded(t *testing.T) { Protocol: operatorv1.ProxyProtocol, }, } + serviceWithELB = corev1.Service{} + serviceWithNLB = corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + AWSLBTypeAnnotation: AWSNLBAnnotation, + }, + }, + } ) testCases := []struct { description string strategy *operatorv1.EndpointPublishingStrategy platform *configv1.PlatformStatus + service *corev1.Service expect bool expectError bool }{ @@ -1521,6 +1516,20 @@ func Test_IsProxyProtocolNeeded(t *testing.T) { platform: &awsPlatform, expect: false, }, + { + description: "loadbalancer strategy with NLB, but a service with ELB should use PROXY", + strategy: &loadBalancerStrategyWithNLB, + platform: &awsPlatform, + service: &serviceWithELB, + expect: true, + }, + { + description: "loadbalancer strategy with ELB, but a service with NLB shouldn't use PROXY", + strategy: &loadBalancerStrategyWithNLB, + platform: &awsPlatform, + service: &serviceWithNLB, + expect: false, + }, { description: "loadbalancer strategy shouldn't use PROXY on Azure", strategy: &loadBalancerStrategy, @@ -1602,7 +1611,7 @@ func Test_IsProxyProtocolNeeded(t *testing.T) { EndpointPublishingStrategy: tc.strategy, }, } - switch actual, err := IsProxyProtocolNeeded(ic, tc.platform); { + switch actual, err := IsProxyProtocolNeeded(ic, tc.platform, tc.service); { case tc.expectError && err == nil: t.Error("expected error, got nil") case !tc.expectError && err != nil: diff --git a/pkg/operator/controller/ingress/deployment.go b/pkg/operator/controller/ingress/deployment.go index 9bfcdf729..d5a238d2e 100644 --- a/pkg/operator/controller/ingress/deployment.go +++ b/pkg/operator/controller/ingress/deployment.go @@ -117,15 +117,11 @@ const ( // ensureRouterDeployment ensures the router deployment exists for a given // ingresscontroller. -func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network, haveClientCAConfigmap bool, clientCAConfigmap *corev1.ConfigMap, platformStatus *configv1.PlatformStatus, clusterProxyConfig *configv1.Proxy) (bool, *appsv1.Deployment, error) { +func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network, haveClientCAConfigmap bool, clientCAConfigmap *corev1.ConfigMap, platformStatus *configv1.PlatformStatus, clusterProxyConfig *configv1.Proxy, proxyNeeded bool) (bool, *appsv1.Deployment, error) { haveDepl, current, err := r.currentRouterDeployment(ci) if err != nil { return false, nil, err } - proxyNeeded, err := IsProxyProtocolNeeded(ci, platformStatus) - if err != nil { - return false, nil, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err) - } desired, err := desiredRouterDeployment(ci, r.config.IngressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, haveClientCAConfigmap, clientCAConfigmap, clusterProxyConfig, r.config.RouteExternalCertificateEnabled, r.config.IngressControllerDCMEnabled) if err != nil { return haveDepl, current, fmt.Errorf("failed to build router deployment: %v", err) diff --git a/pkg/operator/controller/ingress/deployment_test.go b/pkg/operator/controller/ingress/deployment_test.go index c192bab92..ba3245e3b 100644 --- a/pkg/operator/controller/ingress/deployment_test.go +++ b/pkg/operator/controller/ingress/deployment_test.go @@ -379,7 +379,7 @@ func getRouterDeploymentComponents(t *testing.T) (*operatorv1.IngressController, }, }, } - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) if err != nil { t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) } @@ -683,7 +683,7 @@ func TestDesiredRouterDeploymentSpecAndNetwork(t *testing.T) { ic.Status.Domain = "example.com" ic.Status.EndpointPublishingStrategy.Type = operatorv1.LoadBalancerServiceStrategyType - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) if err != nil { t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) } @@ -781,7 +781,7 @@ func TestDesiredRouterDeploymentSpecAndNetwork(t *testing.T) { }, }, } - proxyNeeded, err = IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err = IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) if err != nil { t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) } @@ -891,7 +891,7 @@ func TestDesiredRouterDeploymentVariety(t *testing.T) { networkConfig.Status.ClusterNetwork = []configv1.ClusterNetworkEntry{ {CIDR: "2620:0:2d0:200::7/32"}, } - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) if err != nil { t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) } @@ -1009,10 +1009,6 @@ func TestDesiredRouterDeploymentVariety(t *testing.T) { func TestDesiredRouterDeploymentHostNetworkNil(t *testing.T) { ic, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, clusterProxyConfig := getRouterDeploymentComponents(t) ic.Status.EndpointPublishingStrategy.Type = operatorv1.HostNetworkStrategyType - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) - if err != nil { - t.Fatal(err) - } deployment, err := desiredRouterDeployment(ic, ingressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, false, nil, clusterProxyConfig, false, false) if err != nil { t.Fatal(err) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 562068fa7..a41f0d40a 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "k8s.io/apimachinery/pkg/util/sets" "reflect" "strconv" "strings" @@ -23,8 +24,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 +32,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 +217,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 +230,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() sets.Set[string] { + result := sets.New[string]( // AWS LB health check interval annotation (see // ). awsLBHealthCheckIntervalAnnotation, @@ -242,8 +243,6 @@ var ( // local-with-fallback annotation for kube-proxy (see // ). localWithFallbackAnnotation, - // AWS load balancer type annotation to set either CLB/ELB or NLB - AWSLBTypeAnnotation, // awsLBProxyProtocolAnnotation is used to enable the PROXY protocol on any // AWS load balancer services created. // @@ -279,13 +278,67 @@ var ( 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]sets.Set[string] { + result := map[configv1.PlatformType]sets.Set[string]{ + configv1.AWSPlatformType: sets.New[string]( + // 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 { + if result[platform] == nil { + result[platform] = sets.New[string]() + } + result[platform].Insert(name) + } + } + } + for platform, annotation := range externalLBAnnotations { + if _, ok := platformsWithMutableScope[platform]; !ok { + for name := range annotation { + if result[platform] == nil { + result[platform] = sets.New[string]() + } + result[platform].Insert(name) + } + } + } + return result + }() ) // ensureLoadBalancerService creates an LB service if one is desired but absent. // Always returns the current LB service if one exists (whether it already // existed or was created during the course of the function). -func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platformStatus *configv1.PlatformStatus) (bool, *corev1.Service, error) { - wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled) +func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platformStatus *configv1.PlatformStatus, proxyNeeded bool) (bool, *corev1.Service, error) { + wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled, proxyNeeded) if err != nil { return false, nil, err } @@ -351,7 +404,7 @@ func isServiceOwnedByIngressController(service *corev1.Service, ic *operatorv1.I // ingresscontroller, or nil if an LB service isn't desired. An LB service is // desired if the high availability type is Cloud. An LB service will declare an // owner reference to the given deployment. -func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) (bool, *corev1.Service, error) { +func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool, proxyNeeded bool) (bool, *corev1.Service, error) { if ci.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType { return false, nil, nil } @@ -381,11 +434,6 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef service.Annotations = map[string]string{} } - proxyNeeded, err := IsProxyProtocolNeeded(ci, platform) - if err != nil { - return false, nil, fmt.Errorf("failed to determine if proxy protocol is proxyNeeded for ingresscontroller %q: %v", ci.Name, err) - } - if platform != nil { if isInternal { annotation := InternalLBAnnotations[platform.Type] @@ -618,8 +666,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, changedFields := shouldRecreateLoadBalancer(current, desired, platform); shouldRecreateLB && autoDeleteLB { + log.Info("deleting and recreating the load balancer", "annotations", changedAnnotations, "fields", changedFields, "namespace", desired.Namespace, "name", desired.Name) foreground := metav1.DeletePropagationForeground deleteOptions := crclient.DeleteOptions{PropagationPolicy: &foreground} if err := r.deleteLoadBalancerService(current, &deleteOptions); err != nil { @@ -644,47 +692,16 @@ 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" - } +// recreated and returns the changed annotations and fields that require recreation. +func shouldRecreateLoadBalancer(current, desired *corev1.Service, platform *configv1.PlatformStatus) (bool, []string, []string) { + shouldRecreateLB, changedAnnotations, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedAtServiceCreationLBServiceAnnotations[platform.Type]) + var changedFields []string if platform.Type == configv1.OpenStackPlatformType && current.Spec.LoadBalancerIP != desired.Spec.LoadBalancerIP { - return true, "its load balancer IP changed" + shouldRecreateLB = true + changedFields = append(changedFields, "spec.loadBalancerIP") } - return false, "" + return shouldRecreateLB, changedAnnotations, changedFields } // loadBalancerServiceChanged checks if the current load balancer service @@ -698,7 +715,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, @@ -726,18 +743,17 @@ 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 +func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, annotations sets.Set[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 - break + changedAnnotations = append(changedAnnotations, annotation) } } - if !changed { - return false, nil + if len(changedAnnotations) == 0 { + return false, nil, nil } updated := current.DeepCopy() @@ -756,7 +772,7 @@ func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, an } } - return true, updated + return true, changedAnnotations, updated } // IsServiceInternal returns a Boolean indicating whether the provided service @@ -781,7 +797,11 @@ func IsServiceInternal(service *corev1.Service) bool { // the service, then the return value is a non-nil error indicating that the // modification must be reverted before upgrading is allowed. func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, current *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) error { - want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled) + proxyNeeded, err := IsProxyProtocolNeeded(ic, platform, current) + if err != nil { + return fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) + } + want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled, proxyNeeded) if err != nil { return err } @@ -794,7 +814,7 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme // Since the status logic runs after the controller sets the annotations, it checks for // any discrepancy (in case modified) between the desired annotation values of the controller // and the current annotation values. - changed, updated := loadBalancerServiceAnnotationsChanged(current, desired, managedLoadBalancerServiceAnnotations) + changed, updated, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedLBServiceAnnotations) 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) @@ -803,29 +823,61 @@ 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. +// Tricky: Status fields have mixed functions. Older status fields often represent the +// *desired* state, while some newer fields reflect the *actual* state of the object. func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) error { var errs []error + // Tricky: Scope in the status indicates the *desired* scope (user desire + defaulting). wantScope := ic.Status.EndpointPublishingStrategy.LoadBalancer.Scope haveScope := operatorv1.ExternalLoadBalancer if IsServiceInternal(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) } + haveLBType := getAWSLoadBalancerTypeFromServiceAnnotation(service) + if platform.Type == configv1.AWSPlatformType { + // Tricky: LB Type in the status indicates the *desired* scope (user desire + defaulting). + wantLBType := getAWSLoadBalancerTypeInStatus(ic) + + 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 ( + // Tricky: Subnets in the status indicates the *actual* scope. wantSubnets, haveSubnets *operatorv1.AWSSubnets paramsFieldName string ) - switch getAWSLoadBalancerTypeInStatus(ic) { + switch haveLBType { case operatorv1.AWSNetworkLoadBalancer: if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil { wantSubnets = nlbParams.Subnets @@ -850,14 +902,15 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service haveSubnetsPrettyJson := convertAWSSubnetListToPatchJson(haveSubnets, "{}", "[]") 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) + 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, haveLBType, paramsFieldName, haveSubnetsPatchJson) + err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) errs = append(errs, err) } } - if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeInStatus(ic) == operatorv1.AWSNetworkLoadBalancer { + if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && haveLBType == operatorv1.AWSNetworkLoadBalancer { var ( + // Tricky: EIP Allocations in the status indicate the *actual* scope. wantEIPAllocations, haveEIPAllocations []operatorv1.EIPAllocation ) if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil { @@ -873,20 +926,21 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service haveEIPAllocationsPrettyJson := convertAWSEIPAllocationsListToPatchJson(haveEIPAllocations, "[]") 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) + 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, haveLBType, "networkLoadBalancer", haveEIPAllocationsPatchJson) + err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) errs = append(errs, err) } } if platform.Type == configv1.OpenStackPlatformType { + // Tricky: FloatingIP in the status indicate the *actual* scope. wantFloatingIP := getOpenStackFloatingIPInSpec(ic) haveFloatingIP := getOpenStackFloatingIPInStatus(ic) // OpenStack CCM does not support updating Service.Spec.LoadBalancerIP after creation, the load balancer will never be updated. if wantFloatingIP != haveFloatingIP { - changeMsg := fmt.Sprintf("The IngressController floatingIP was changed from %q to %q.", haveFloatingIP, wantFloatingIP) + changedMsg := fmt.Sprintf("The IngressController floatingIP was changed from %q to %q.", haveFloatingIP, wantFloatingIP) ocPatchRevertCmd := fmt.Sprintf(`oc -n %[1]s patch ingresscontrollers/%[2]s --type=merge --patch='{"spec":{"endpointPublishingStrategy":{"type":"LoadBalancerService","loadBalancer":{"providerParameters":{"type":"OpenStack","openstack":{"floatingIP":"%[3]s"}}}}}}'`, ic.Namespace, ic.Name, haveFloatingIP) - 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`", changeMsg, service.Namespace, service.Name, ocPatchRevertCmd) + err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) errs = append(errs, err) } } @@ -999,6 +1053,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) } +// getAWSLoadBalancerTypeFromServiceAnnotation 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 getAWSLoadBalancerTypeFromServiceAnnotation(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. @@ -1067,16 +1136,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 { @@ -1230,7 +1289,7 @@ func getAWSLoadBalancerTypeInStatus(ic *operatorv1.IngressController) operatorv1 ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil { return ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type } - return "" + return operatorv1.AWSClassicLoadBalancer } // getAWSClassicLoadBalancerParametersInSpec gets the ClassicLoadBalancerParameter struct diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index 44e779861..0abd8f49e 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -5,6 +5,8 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" @@ -767,7 +769,7 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, } - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) switch { case err != nil: t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) @@ -775,7 +777,7 @@ func Test_desiredLoadBalancerService(t *testing.T) { t.Errorf("expected IsProxyProtocolNeeded to return %v, got %v", tc.proxyNeeded, proxyNeeded) } - haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, tc.subnetsAWSFeatureEnabled, tc.eipAllocationsAWSFeatureEnabled) + haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, tc.subnetsAWSFeatureEnabled, tc.eipAllocationsAWSFeatureEnabled, proxyNeeded) switch { case err != nil: t.Error(err) @@ -960,7 +962,7 @@ func TestDesiredLoadBalancerServiceAWSIdleTimeout(t *testing.T) { }, }, } - haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true) + haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true, false) if err != nil { t.Fatal(err) } @@ -1220,7 +1222,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", @@ -1290,32 +1299,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 sets.Set[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: sets.New[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: sets.New[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: sets.New[string]("foo"), expect: false, }, { @@ -1327,7 +1337,7 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { "foo": "bar", "baz": "quux", }, - managedAnnotations: sets.NewString("foo"), + managedAnnotations: sets.New[string]("foo"), expect: false, }, { @@ -1336,8 +1346,9 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { expectedAnnotations: map[string]string{ "foo": "bar", }, - managedAnnotations: sets.NewString("foo"), - expect: true, + managedAnnotations: sets.New[string]("foo"), + expect: true, + expectChangedAnnotations: []string{"foo"}, }, { description: "if a managed annotation is updated", @@ -1347,17 +1358,19 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { expectedAnnotations: map[string]string{ "foo": "baz", }, - managedAnnotations: sets.NewString("foo"), - expect: true, + managedAnnotations: sets.New[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: sets.New[string]("foo"), + expect: true, + expectChangedAnnotations: []string{"foo"}, }, } @@ -1373,13 +1386,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") } } @@ -1574,7 +1588,7 @@ func TestUpdateLoadBalancerServiceSourceRanges(t *testing.T) { }, }, } - wantSvc, desired, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true) + wantSvc, desired, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true, false) if err != nil { t.Fatal(err) } @@ -1643,12 +1657,13 @@ func TestLoadBalancerServiceChangedEmptyAnnotations(t *testing.T) { // recreated if the annotations or spec of the service have changed. func Test_shouldRecreateLoadBalancer(t *testing.T) { testCases := []struct { - description string - current *corev1.Service - desired *corev1.Service - platform *configv1.PlatformStatus - expect bool - reason string + description string + current *corev1.Service + desired *corev1.Service + platform *configv1.PlatformStatus + expect bool + expectChangedAnnotations []string + expectChangedFields []string }{ { description: "AWS platform with different subnets", @@ -1670,7 +1685,9 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { Type: configv1.AWSPlatformType, }, expect: true, - reason: "its subnets changed", + expectChangedAnnotations: []string{ + awsLBSubnetsAnnotation, + }, }, { description: "AWS platform with different EIP allocations", @@ -1692,7 +1709,9 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { Type: configv1.AWSPlatformType, }, expect: true, - reason: "its eipAllocations changed", + expectChangedAnnotations: []string{ + awsEIPAllocationsAnnotation, + }, }, { description: "OpenStack platform with different LoadBalancerIP", @@ -1710,7 +1729,9 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { Type: configv1.OpenStackPlatformType, }, expect: true, - reason: "its load balancer IP changed", + expectChangedFields: []string{ + "spec.loadBalancerIP", + }, }, { description: "Platform with mutable scope and same scope", @@ -1753,7 +1774,9 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { Type: configv1.AWSPlatformType, }, expect: true, - reason: "its scope changed", + expectChangedAnnotations: []string{ + awsInternalLBAnnotation, + }, }, { description: "Platform with same configuration", @@ -1780,12 +1803,19 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - changed, reason := shouldRecreateLoadBalancer(tc.current, tc.desired, tc.platform) + changed, changedAnnotations, changedFields := shouldRecreateLoadBalancer(tc.current, tc.desired, tc.platform) if changed != tc.expect { t.Errorf("expected %t, got %t", tc.expect, changed) } - if reason != tc.reason { - t.Errorf("expected reason %s, got %s", tc.reason, reason) + sliceCmpOpts := []cmp.Option{ + cmpopts.EquateEmpty(), + cmpopts.SortSlices(func(a, b string) bool { return a < b }), + } + if !cmp.Equal(changedAnnotations, tc.expectChangedAnnotations, sliceCmpOpts...) { + t.Errorf("expected reason %s, got %s", tc.expectChangedAnnotations, changedAnnotations) + } + if !cmp.Equal(changedFields, tc.expectChangedFields, sliceCmpOpts...) { + t.Errorf("expected reason %s, got %s", tc.expectChangedFields, changedFields) } }) } diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index e0df9564d..3d99c1f13 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -71,6 +71,8 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle updated.Status.Selector = selector.String() updated.Status.TLSProfile = computeIngressTLSProfile(ic.Status.TLSProfile, deployment) + clearIngressControllerInactiveAWSLBTypeParametersStatus(platformStatus, updated, service) + if updated.Status.EndpointPublishingStrategy != nil && updated.Status.EndpointPublishingStrategy.LoadBalancer != nil { updated.Status.EndpointPublishingStrategy.LoadBalancer.AllowedSourceRanges = computeAllowedSourceRanges(service) } @@ -810,18 +812,28 @@ func IngressStatusesEqual(a, b operatorv1.IngressControllerStatus) bool { } if a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil && b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil && a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil && b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil { - if a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters != nil && b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters != nil { - if !awsSubnetsEqual(a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.Subnets, b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.Subnets) { + nlbParamsA := a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters + nlbParamsB := b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters + if nlbParamsA != nil && nlbParamsB != nil { + if !awsSubnetsEqual(nlbParamsA.Subnets, nlbParamsB.Subnets) { return false } - if !awsEIPAllocationsEqual(a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations, b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations) { + if !awsEIPAllocationsEqual(nlbParamsA.EIPAllocations, nlbParamsB.EIPAllocations) { return false } + } else if nlbParamsA != nlbParamsB { + // One is nil, the other is not. + return false } - if a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters != nil && b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters != nil { - if !awsSubnetsEqual(a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters.Subnets, b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters.Subnets) { + clbParamsA := a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters + clbParamsB := b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters + if clbParamsA != nil && clbParamsB != nil { + if !awsSubnetsEqual(clbParamsA.Subnets, clbParamsB.Subnets) { return false } + } else if clbParamsA != clbParamsB { + // One is nil, the other is not. + return false } } if getOpenStackFloatingIPInEPS(a.EndpointPublishingStrategy) != getOpenStackFloatingIPInEPS(b.EndpointPublishingStrategy) { @@ -949,12 +961,38 @@ func computeLoadBalancerProgressingStatus(ic *operatorv1.IngressController, serv } } +// clearIngressControllerInactiveAWSLBTypeParametersStatus clears AWS parameters for the inactive load balancer type, +// unless there is a load balancer type change is pending. When a change is pending, setDefaultPublishingStrategy +// sets the desired parameters for the new LB type. To avoid removing the desired state, we allow +// parameters for both classicLoadBalancer and networkLoadBalancer to exist during the transition. +func clearIngressControllerInactiveAWSLBTypeParametersStatus(platformStatus *configv1.PlatformStatus, ic *operatorv1.IngressController, service *corev1.Service) { + if service == nil { + return + } + if platformStatus.Type == configv1.AWSPlatformType { + haveLBType := getAWSLoadBalancerTypeFromServiceAnnotation(service) + wantLBType := getAWSLoadBalancerTypeInStatus(ic) + if haveLBType == wantLBType { + switch haveLBType { + case operatorv1.AWSNetworkLoadBalancer: + if getAWSClassicLoadBalancerParametersInStatus(ic) != nil { + ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters = nil + } + case operatorv1.AWSClassicLoadBalancer: + if getAWSNetworkLoadBalancerParametersInStatus(ic) != nil { + ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = nil + } + } + } + } +} + // updateIngressControllerAWSSubnetStatus mutates the provided IngressController object to // sync its status to the effective subnets on the LoadBalancer-type service. func updateIngressControllerAWSSubnetStatus(ic *operatorv1.IngressController, service *corev1.Service) { // Set the subnets status based on the actual service annotation and the load balancer type, // as NLBs and CLBs have separate subnet configuration fields. - switch getAWSLoadBalancerTypeInStatus(ic) { + switch getAWSLoadBalancerTypeFromServiceAnnotation(service) { case operatorv1.AWSNetworkLoadBalancer: // NetworkLoadBalancerParameters should be initialized by setDefaultPublishingStrategy // when an IngressController is admitted, so we don't need to initialize here. @@ -974,7 +1012,7 @@ func updateIngressControllerAWSSubnetStatus(ic *operatorv1.IngressController, se // sync its status to the effective eipAllocations on the LoadBalancer-type service. func updateIngressControllerAWSEIPAllocationStatus(ic *operatorv1.IngressController, service *corev1.Service) { // Set the eipAllocations status based on the actual service annotation and on the load balancer type `NLB`. - switch getAWSLoadBalancerTypeInStatus(ic) { + switch getAWSLoadBalancerTypeFromServiceAnnotation(service) { case operatorv1.AWSNetworkLoadBalancer: // NetworkLoadBalancerParameters should be initialized by setDefaultPublishingStrategy // when an IngressController is admitted, so we don't need to initialize here. diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index bd2a0ca15..b82a5e102 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -6,6 +6,7 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/pem" + "github.com/stretchr/testify/assert" "math/big" "reflect" "strings" @@ -655,8 +656,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 +677,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{ @@ -790,6 +795,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{ @@ -935,7 +947,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -947,7 +959,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, &operatorv1.AWSSubnets{}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -961,7 +973,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: false, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -975,7 +987,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -989,7 +1001,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { IDs: []operatorv1.AWSSubnetID{"subnet-12345"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -1007,7 +1019,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Names: []operatorv1.AWSSubnetName{"name-12345"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1025,7 +1037,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Names: []operatorv1.AWSSubnetName{"name-67890"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -1043,7 +1055,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, @@ -1061,7 +1073,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, @@ -1196,7 +1208,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1207,7 +1219,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, []operatorv1.EIPAllocation{}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1218,7 +1230,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, @@ -1229,7 +1241,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, @@ -1240,7 +1252,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, @@ -1251,7 +1263,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, @@ -1262,7 +1274,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, @@ -1273,7 +1285,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, @@ -1284,11 +1296,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), IC Status LBType Classic", + ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSClassicLoadBalancer), + service: lbService, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "LBType Classic LoadBalancerService, IC Status LBType NLB", + ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSNetworkLoadBalancer), + service: lbService, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionTrue, + }, + { + name: "LBType NLB LoadBalancerService, IC Status 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) { @@ -2292,6 +2325,44 @@ func Test_IngressStatusesEqual(t *testing.T) { []operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-xxxxxxxxxxxxxxxxx"}, ), }, + { + description: "classicLoadBalancer parameters cleared", + expected: false, + a: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSClassicLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + b: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + AWS: &operatorv1.AWSLoadBalancerParameters{}, + }, + }, + }, + }, + }, + { + description: "networkLoadBalancer parameters cleared", + expected: false, + a: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + b: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + AWS: &operatorv1.AWSLoadBalancerParameters{}, + }, + }, + }, + }, + }, } for _, tc := range testCases { @@ -3175,7 +3246,7 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) { }, }, } - wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true) + wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true, true) if err != nil { t.Errorf("unexpected error from desiredLoadBalancerService: %v", err) return @@ -3285,7 +3356,7 @@ func Test_computeIngressEvaluationConditionsDetectedCondition(t *testing.T) { }, } - wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true) + wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true, true) if err != nil { t.Fatalf("unexpected error from desiredLoadBalancerService: %v", err) } @@ -3383,3 +3454,77 @@ func Test_computeAllowedSourceRanges(t *testing.T) { }) } } + +func Test_clearIngressControllerInactiveAWSLBTypeParametersStatus(t *testing.T) { + awsPlatformStatus := &configv1.PlatformStatus{ + Type: configv1.AWSPlatformType, + } + lbServiceWithNLB := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + AWSLBTypeAnnotation: AWSNLBAnnotation, + }, + }, + } + lbServiceWithCLB := &corev1.Service{} + icStatusWithAWSLBParams := func(lbType operatorv1.AWSLoadBalancerType, nlbParams, clbParams bool) operatorv1.IngressControllerStatus { + icStatus := operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: lbType, + }, + }, + }, + }, + } + if nlbParams { + icStatus.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{} + } + if clbParams { + icStatus.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{} + } + return icStatus + } + testCases := []struct { + name string + service *corev1.Service + icStatus operatorv1.IngressControllerStatus + expectICStatus operatorv1.IngressControllerStatus + }{ + { + name: "service is nil", + service: nil, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + }, + { + name: "clear clb params when lb type change is not pending", + service: lbServiceWithNLB, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, false), + }, + { + name: "clear nlb params when lb type change is not pending", + service: lbServiceWithCLB, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSClassicLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSClassicLoadBalancer, false, true), + }, + { + name: "don't clear any params when lb type change is pending", + service: lbServiceWithCLB, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ic := &operatorv1.IngressController{ + Status: tc.icStatus, + } + clearIngressControllerInactiveAWSLBTypeParametersStatus(awsPlatformStatus, ic, tc.service) + assert.Equal(t, tc.expectICStatus, ic.Status) + }) + } +} diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index b9fb0d818..62c31b09b 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -1371,19 +1371,23 @@ func TestAWSResourceTagsChanged(t *testing.T) { assertLoadBalancerServiceAnnotationWithPollImmediate(t, kclient, defaultIC, awsLBAdditionalResourceTags, expectedTags) } +// 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) @@ -1393,50 +1397,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) - } - if v := lbService.Annotations[ingresscontroller.AWSLBTypeAnnotation]; len(v) != 0 { - t.Fatalf("load balancer service has unexpected %s=%s annotation", ingresscontroller.AWSLBTypeAnnotation, v) - } + // LB Annotation should be empty by default (meaning CLB). + waitForLBAnnotation(t, ic, ingresscontroller.AWSLBTypeAnnotation, false, "") - pp := &operatorv1.ProviderLoadBalancerParameters{ - Type: operatorv1.AWSLoadBalancerProvider, - AWS: &operatorv1.AWSLoadBalancerParameters{ - Type: operatorv1.AWSNetworkLoadBalancer, - }, + // 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 err := updateIngressControllerWithRetryOnConflict(t, name, timeout, func(ic *operatorv1.IngressController) { - ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters = pp + // 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) } - // Wait for the load balancer and DNS to be ready. - if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, availableConditionsForIngressControllerWithLoadBalancer...); err != nil { + // 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) } +} - 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) +// 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) + } + + // 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).Name) + if err := recreateIngressControllerService(t, ic); err != nil { + t.Fatalf("failed to delete and recreate service: %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) + } + + // 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) } }