From 3a39011e11d346b5f28ec0c4662555d291f784ba Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Wed, 18 Sep 2024 17:22:51 -0400 Subject: [PATCH] LB Type Should Reflect Actual LB Type --- pkg/operator/controller/ingress/controller.go | 32 +++++-- .../controller/ingress/controller_test.go | 25 ++++- pkg/operator/controller/ingress/deployment.go | 6 +- .../controller/ingress/deployment_test.go | 12 +-- .../ingress/load_balancer_service.go | 66 +++++++++---- .../ingress/load_balancer_service_test.go | 8 +- .../controller/ingress/status_test.go | 93 ++++++++++++++++--- 7 files changed, 180 insertions(+), 62 deletions(-) diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 43984ec1a4..dd9258f444 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -1069,7 +1069,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) @@ -1088,7 +1100,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 { @@ -1183,23 +1195,23 @@ 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 + // LBType is only configured at service creation and won't be updated afterward. If the service already + // exists, we must use the actual LB Type from the service instead of the one specified in the spec. + lbType := getAWSLoadBalancerTypeInStatus(ic) + if service != nil { + lbType = getAWSLoadBalancerTypeFromServiceAnnotation(service) } + 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 530de30715..2cecd6ccfd 100644 --- a/pkg/operator/controller/ingress/controller_test.go +++ b/pkg/operator/controller/ingress/controller_test.go @@ -1437,11 +1437,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 }{ @@ -1487,6 +1496,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, @@ -1568,7 +1591,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 f859d4552c..b1fc6cc111 100644 --- a/pkg/operator/controller/ingress/deployment.go +++ b/pkg/operator/controller/ingress/deployment.go @@ -111,15 +111,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) 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 96a5608589..4aa7879cb8 100644 --- a/pkg/operator/controller/ingress/deployment_test.go +++ b/pkg/operator/controller/ingress/deployment_test.go @@ -378,7 +378,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) } @@ -664,7 +664,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) } @@ -760,7 +760,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) } @@ -868,7 +868,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) } @@ -986,10 +986,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) 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 7cef35dda3..9b71c18b2d 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -279,8 +279,8 @@ var ( // 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 } @@ -346,7 +346,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 } @@ -365,18 +365,13 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef service.Spec.Selector = controller.IngressControllerDeploymentPodSelector(ci).MatchLabels - lb := ci.Status.EndpointPublishingStrategy.LoadBalancer - isInternal := lb != nil && lb.Scope == operatorv1.InternalLoadBalancer + lbStatus := ci.Status.EndpointPublishingStrategy.LoadBalancer + isInternal := lbStatus != nil && lbStatus.Scope == operatorv1.InternalLoadBalancer if service.Annotations == nil { 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] @@ -386,10 +381,10 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef // Set the GCP Global Access annotation for internal load balancers on GCP only if platform.Type == configv1.GCPPlatformType { - if lb != nil && lb.ProviderParameters != nil && - lb.ProviderParameters.Type == operatorv1.GCPLoadBalancerProvider && - lb.ProviderParameters.GCP != nil { - globalAccessEnabled := lb.ProviderParameters.GCP.ClientAccess == operatorv1.GCPGlobalAccess + if lbStatus != nil && lbStatus.ProviderParameters != nil && + lbStatus.ProviderParameters.Type == operatorv1.GCPLoadBalancerProvider && + lbStatus.ProviderParameters.GCP != nil { + globalAccessEnabled := lbStatus.ProviderParameters.GCP.ClientAccess == operatorv1.GCPGlobalAccess service.Annotations[GCPGlobalAccessAnnotation] = strconv.FormatBool(globalAccessEnabled) } } @@ -405,8 +400,8 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef if proxyNeeded { service.Annotations[awsLBProxyProtocolAnnotation] = "*" } - if lb != nil && lb.ProviderParameters != nil { - if aws := lb.ProviderParameters.AWS; aws != nil && lb.ProviderParameters.Type == operatorv1.AWSLoadBalancerProvider { + if lbStatus != nil && lbStatus.ProviderParameters != nil { + if aws := lbStatus.ProviderParameters.AWS; aws != nil && lbStatus.ProviderParameters.Type == operatorv1.AWSLoadBalancerProvider { switch aws.Type { case operatorv1.AWSNetworkLoadBalancer: service.Annotations[AWSLBTypeAnnotation] = AWSNLBAnnotation @@ -764,7 +759,11 @@ func loadBalancerServiceTagsModified(current, expected *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 } @@ -804,7 +803,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service wantSubnets, haveSubnets *operatorv1.AWSSubnets paramsFieldName string ) - switch getAWSLoadBalancerTypeInStatus(ic) { + switch getAWSLoadBalancerTypeFromServiceAnnotation(service) { case operatorv1.AWSNetworkLoadBalancer: if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil { wantSubnets = nlbParams.Subnets @@ -835,7 +834,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service } } - if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeInStatus(ic) == operatorv1.AWSNetworkLoadBalancer { + if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeFromServiceAnnotation(service) == operatorv1.AWSNetworkLoadBalancer { var ( wantEIPAllocations, haveEIPAllocations []operatorv1.EIPAllocation ) @@ -966,6 +965,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. @@ -1180,6 +1194,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 && @@ -1188,7 +1214,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 cb0f61ea47..e3467a86b4 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -725,7 +725,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) @@ -733,7 +733,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) @@ -917,7 +917,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) } @@ -1524,7 +1524,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) } diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 25abb25ff8..1457141efb 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -763,6 +763,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{ @@ -924,7 +931,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -938,7 +945,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 +963,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Names: []operatorv1.AWSSubnetName{"name-12345"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -974,7 +981,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Names: []operatorv1.AWSSubnetName{"name-67890"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -992,7 +999,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 +1017,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, @@ -1139,6 +1146,42 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, }, + { + name: "CLB LoadBalancerService, AWS Subnets spec and status are NOT equal, but Service Annotation is NLB", + ic: loadBalancerIngressControllerWithAWSSubnets( + operatorv1.AWSClassicLoadBalancer, + &operatorv1.AWSSubnets{ + IDs: []operatorv1.AWSSubnetID{"subnet-12345"}, + Names: []operatorv1.AWSSubnetName{"name-12345"}, + }, + &operatorv1.AWSSubnets{ + IDs: []operatorv1.AWSSubnetID{"subnet-67890"}, + Names: []operatorv1.AWSSubnetName{"name-67890"}, + }, + ), + service: lbServiceWithNLB, + awsSubnetsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "NLB LoadBalancerService, AWS Subnets spec and status are equal, but Service Annotation is CLB", + ic: loadBalancerIngressControllerWithAWSSubnets( + operatorv1.AWSNetworkLoadBalancer, + &operatorv1.AWSSubnets{ + IDs: []operatorv1.AWSSubnetID{"subnet-12345"}, + Names: []operatorv1.AWSSubnetName{"name-12345"}, + }, + &operatorv1.AWSSubnets{ + IDs: []operatorv1.AWSSubnetID{"subnet-67890"}, + Names: []operatorv1.AWSSubnetName{"name-67890"}, + }, + ), + service: &corev1.Service{}, + awsSubnetsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, { name: "NLB LoadBalancerService, AWS EIPAllocations nil spec and nil status", ic: loadBalancerIngressControllerWithAWSEIPAllocations( @@ -1178,7 +1221,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 +1232,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 +1243,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 +1254,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 +1265,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 +1276,33 @@ 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: "CLB LoadBalancerService, AWS EIPAllocations spec and status are NOT equal, but Service Annotation is NLB", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + []operatorv1.EIPAllocation{"eipalloc-aaaaaaaaaaaaaaaaa", "eipalloc-bbbbbbbbbbbbbbbbb"}, + ), + service: lbServiceWithNLB, + awsSubnetsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "NLB LoadBalancerService, AWS EIPAllocations spec and status are equal, but Service Annotation is CLB", + ic: loadBalancerIngressControllerWithAWSEIPAllocations( + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"}, + []operatorv1.EIPAllocation{"eipalloc-aaaaaaaaaaaaaaaaa", "eipalloc-bbbbbbbbbbbbbbbbb"}, + ), + service: &corev1.Service{}, + awsSubnetsEnabled: true, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -3085,7 +3150,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, false) if err != nil { t.Errorf("unexpected error from desiredLoadBalancerService: %v", err) return @@ -3195,7 +3260,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, false) if err != nil { t.Fatalf("unexpected error from desiredLoadBalancerService: %v", err) }