diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index bf3e06094..3cbd8050b 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -1088,7 +1088,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) @@ -1107,7 +1119,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 { @@ -1202,23 +1214,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 530de3071..2cecd6ccf 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 5df994e60..3b101a143 100644 --- a/pkg/operator/controller/ingress/deployment.go +++ b/pkg/operator/controller/ingress/deployment.go @@ -112,15 +112,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 2defacffb..45c44cb56 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) } @@ -681,7 +681,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) } @@ -777,7 +777,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) } @@ -885,7 +885,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) } @@ -1003,10 +1003,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 1fd6eb0da..e234a09d7 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 } @@ -430,11 +430,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] @@ -775,7 +770,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 } @@ -1241,7 +1240,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 2db68d74f..fa12c04c4 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -724,7 +724,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) @@ -732,7 +732,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) @@ -916,7 +916,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) } @@ -1535,7 +1535,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 3cecfc550..7cc2118dd 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -3117,7 +3117,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 @@ -3227,7 +3227,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) }