[WILL SQUASH] Refactor IsProxyProtocolNeeded

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

@@ -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 &&

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:

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)

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)

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

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -724,15 +724,15 @@ 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)
 			case tc.proxyNeeded && !proxyNeeded || !tc.proxyNeeded && proxyNeeded:
 				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)
 			}

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)
 			}