From 93ddc44ae5f66ffdaf0ea1a399a70fd542f78551 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Wed, 18 Sep 2024 17:22:51 -0400 Subject: [PATCH] [WILL SQUASH] Refactor IsProxyProtocolNeeded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IsProxyProtocolNeeded must get the LB Type from the service, as the status now may not accurately reflect the actual LB Type during an update. However, we can't rely only on the service because it doesn’t exist during the initial bootstrap (and potentially when it gets deleted). In that case, it's safe to use the status to determine the anticipated LB Type. Without this commit results in a misalignment of the proxy protocol value between the load balancer and the router deployment when an LB type change is pending. This commit will be squashed into the previous one, as both are dependent on each other. I've kept them separate for easier review. --- pkg/operator/controller/ingress/controller.go | 35 +++++++++++++------ .../controller/ingress/controller_test.go | 25 ++++++++++++- pkg/operator/controller/ingress/deployment.go | 6 +--- .../controller/ingress/deployment_test.go | 12 +++---- .../ingress/load_balancer_service.go | 19 +++++----- .../ingress/load_balancer_service_test.go | 8 ++--- .../controller/ingress/status_test.go | 4 +-- 7 files changed, 69 insertions(+), 40 deletions(-) diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 83d877e2c..229926fb4 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -1110,7 +1110,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) @@ -1129,7 +1141,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 { @@ -1224,23 +1236,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..84374d962 100644 --- a/pkg/operator/controller/ingress/controller_test.go +++ b/pkg/operator/controller/ingress/controller_test.go @@ -1471,11 +1471,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 +1530,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 +1625,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 2f10ee871..a41f0d40a 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -337,8 +337,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 } @@ -404,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 } @@ -434,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] @@ -802,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 } @@ -1290,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 359fbd07d..366c2e640 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -769,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) @@ -777,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) @@ -962,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) } @@ -1588,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) } diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index c22c3e80b..88f1d3196 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -3207,7 +3207,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 @@ -3317,7 +3317,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) }