[WIP] Bug 1960284: use unsupportedConfigOverrides to set prefer-local annotation

assets/router/service-cloud.yaml

@@ -13,7 +13,7 @@ spec:
     app: router
   # This also has the effect of marking LB pool targets as unhealthy when no
   # router pods are present on a node behind the service.
-  externalTrafficPolicy: Local
+  externalTrafficPolicy: Cluster
   ports:
   - name: http
     protocol: TCP

pkg/operator/controller/ingress/load_balancer_service.go

@@ -2,6 +2,7 @@ package ingress
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"strconv"
 	"strings"
@@ -298,6 +299,24 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 		}
 		// Azure load balancers are not customizable and are set to (2 fail @ 5s interval, 2 healthy)
 		// GCP load balancers are not customizable and are set to (3 fail @ 8s interval, 1 healthy)
+
+		// ExternalTrafficPolicy default is now Cluster, but can still be overridden, except for IBMCloud
+		if platform.Type != configv1.IBMCloudPlatformType {
+			var unsupportedConfigOverrides struct {
+				ExternalTrafficPolicy string `json:"externalTrafficPolicy"`
+			}
+			if len(ci.Spec.UnsupportedConfigOverrides.Raw) > 0 {
+				if err := json.Unmarshal(ci.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigOverrides); err != nil {
+					return true, nil, fmt.Errorf("ingresscontroller %q has invalid spec.unsupportedConfigOverrides: %w", ci.Name, err)
+				}
+			}
+			externalTrafficPolicy := corev1.ServiceExternalTrafficPolicyTypeCluster
+			switch unsupportedConfigOverrides.ExternalTrafficPolicy {
+			case string(corev1.ServiceExternalTrafficPolicyTypeLocal):
+				externalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeLocal
+			}
+			service.Spec.ExternalTrafficPolicy = externalTrafficPolicy
+		}
 	}
 
 	service.SetOwnerReferences([]metav1.OwnerReference{deploymentRef})
@@ -400,9 +419,10 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev
 	updated := current.DeepCopy()
 	changed := false
 
-	// Preserve everything but the AWS LB health check interval annotation &
-	// GCP Global Access internal Load Balancer annotation.
-	// (see <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>).
+	// Preserve everything except:
+	// AWS LB health check interval annotation (see <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>) ,
+	// GCP Global Access internal Load Balancer annotation (see <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>), &
+	// External Traffic Policy (see <https://bugzilla.redhat.com/show_bug.cgi?id=1929396>).
 	// Updating annotations and spec fields cannot be done unless the
 	// previous release blocks upgrades when the user has modified those
 	// fields (see <https://bugzilla.redhat.com/show_bug.cgi?id=1905490>).
@@ -419,6 +439,10 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev
 		changed = true
 	}
 
+	if current.Spec.ExternalTrafficPolicy != expected.Spec.ExternalTrafficPolicy {
+		updated.Spec.ExternalTrafficPolicy = expected.Spec.ExternalTrafficPolicy
+		changed = true
+	}
 	if !changed {
 		return false, nil
 	}

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -15,14 +15,15 @@ import (
 
 func TestDesiredLoadBalancerService(t *testing.T) {
 	testCases := []struct {
-		description          string
-		platform             configv1.PlatformType
-		strategyType         operatorv1.EndpointPublishingStrategyType
-		lbStrategy           operatorv1.LoadBalancerStrategy
-		proxyNeeded          bool
-		expect               bool
-		platformStatus       configv1.PlatformStatus
-		expectedResourceTags string
+		description           string
+		platform              configv1.PlatformType
+		strategyType          operatorv1.EndpointPublishingStrategyType
+		lbStrategy            operatorv1.LoadBalancerStrategy
+		proxyNeeded           bool
+		expect                bool
+		platformStatus        configv1.PlatformStatus
+		expectedResourceTags  string
+		externalTrafficPolicy corev1.ServiceExternalTrafficPolicyType
 	}{
 		{
 			description:  "external classic load balancer with scope for aws platform",
@@ -253,6 +254,17 @@ func TestDesiredLoadBalancerService(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description:  "external load balancer for AWS platform, external traffic policy cluster",
+			platform:     configv1.AWSPlatformType,
+			strategyType: operatorv1.LoadBalancerServiceStrategyType,
+			lbStrategy: operatorv1.LoadBalancerStrategy{
+				Scope: operatorv1.ExternalLoadBalancer,
+			},
+			proxyNeeded:           true,
+			externalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster,
+			expect:                true,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -474,9 +486,9 @@ func TestLoadBalancerServiceChanged(t *testing.T) {
 		{
 			description: "if .spec.externalTrafficPolicy changes",
 			mutate: func(svc *corev1.Service) {
-				svc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeCluster
+				svc.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeLocal
 			},
-			expect: false,
+			expect: true,
 		},
 		{
 			description: "if .spec.healthCheckNodePort changes",
@@ -562,7 +574,7 @@ func TestLoadBalancerServiceChanged(t *testing.T) {
 			},
 			Spec: corev1.ServiceSpec{
 				ClusterIP:             "1.2.3.4",
-				ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeLocal,
+				ExternalTrafficPolicy: corev1.ServiceExternalTrafficPolicyTypeCluster,
 				HealthCheckNodePort:   int32(33333),
 				Ports: []corev1.ServicePort{
 					{

test/e2e/operator_test.go

@@ -1920,6 +1920,81 @@ func TestLoadBalancingAlgorithmUnsupportedConfigOverride(t *testing.T) {
 	}
 }
 
+// NOTE: This test mutates the default ingress controller, otherwise
+// it may get throttled by CI.
+//
+// TestExternalTrafficPolicyConfigOverride verifies that the
+// operator configures router LoadBalancer service to use the "Local"
+// external traffic policy if the ingresscontroller is so configured
+// using an unsupported config override.
+func TestExternalTrafficPolicyConfigOverride(t *testing.T) {
+	if infraConfig.Status.Platform == configv1.IBMCloudPlatformType {
+		t.Skip("test skipped on IBMCloud platform")
+		return
+	}
+
+	ic := &operatorv1.IngressController{}
+	if err := kclient.Get(context.TODO(), defaultName, ic); err != nil {
+		t.Fatalf("failed to get default ingresscontroller: %v", err)
+	}
+
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, defaultName, availableConditionsForIngressControllerWithLoadBalancer...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	service := &corev1.Service{}
+	if err := kclient.Get(context.TODO(), controller.LoadBalancerServiceName(ic), service); err != nil {
+		t.Fatalf("failed to get ingresscontroller service: %v", err)
+	}
+
+	originalTrafficPolicy := service.Spec.ExternalTrafficPolicy
+	expectedTrafficPolicy := corev1.ServiceExternalTrafficPolicyTypeLocal
+	ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{
+		Raw: []byte(`{"externalTrafficPolicy":"Local"}`),
+	}
+
+	if err := kclient.Update(context.TODO(), ic); err != nil {
+		t.Fatalf("failed to update ingresscontroller to traffic policy Local: %v", err)
+	}
+
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, defaultName, availableConditionsForIngressControllerWithLoadBalancer...); err != nil {
+		t.Errorf("failed to observe expected conditions: %v", err)
+	}
+
+	if err := waitForServiceUpdate(t, controller.LoadBalancerServiceName(ic), expectedTrafficPolicy); err != nil {
+		t.Errorf("failed to successfully update and revert external traffic policy: %c", err)
+	}
+
+	if err := kclient.Get(context.TODO(), controller.LoadBalancerServiceName(ic), service); err != nil {
+		t.Fatalf("failed to get ingresscontroller service: %v", err)
+	}
+
+	if service.Spec.ExternalTrafficPolicy != expectedTrafficPolicy {
+		t.Fatalf("expected updated service spec to use the %q traffic policy, but it used the %q traffic policy", expectedTrafficPolicy, service.Spec.ExternalTrafficPolicy)
+	}
+
+	if err := kclient.Get(context.TODO(), defaultName, ic); err != nil {
+		t.Fatalf("failed to refresh default ingresscontroller: %v", err)
+	}
+
+	// Remove the changes made to the default ingress controller
+	ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{
+		Raw: []byte(`{"externalTrafficPolicy":"Cluster"}`),
+	}
+
+	if err := kclient.Update(context.TODO(), ic); err != nil {
+		t.Fatalf("failed to revert ingresscontroller to default traffic policy: %v", err)
+	}
+
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, defaultName, availableConditionsForIngressControllerWithLoadBalancer...); err != nil {
+		t.Errorf("failed to observe expected conditions: %v", err)
+	}
+
+	if err := waitForServiceUpdate(t, controller.LoadBalancerServiceName(ic), originalTrafficPolicy); err != nil {
+		t.Errorf("failed to successfully update and revert external traffic policy: %c", err)
+	}
+}
+
 func newLoadBalancerController(name types.NamespacedName, domain string) *operatorv1.IngressController {
 	repl := int32(1)
 	return &operatorv1.IngressController{
@@ -2070,6 +2145,25 @@ func waitForDeploymentEnvVar(t *testing.T, cl client.Client, deployment *appsv1.
 	return err
 }
 
+func waitForServiceUpdate(t *testing.T, servicename types.NamespacedName, policy corev1.ServiceExternalTrafficPolicyType) error {
+	service := &corev1.Service{}
+	err := wait.PollImmediate(1*time.Second, 30*time.Second, func() (bool, error) {
+		if err := kclient.Get(context.TODO(), servicename, service); err != nil {
+			t.Logf("failed to get ingresscontroller service to check traffic policy: %v", err)
+			return false, nil
+		}
+		if service.Spec.ExternalTrafficPolicy != policy {
+			return false, nil
+		}
+		return true, nil
+	})
+
+	if service.Spec.ExternalTrafficPolicy != policy {
+		t.Fatalf("expected reverted service spec to use the %q traffic policy, but it used the %q traffic policy", policy, service.Spec.ExternalTrafficPolicy)
+	}
+	return err
+}
+
 func clusterOperatorConditionMap(conditions ...configv1.ClusterOperatorStatusCondition) map[string]string {
 	conds := map[string]string{}
 	for _, cond := range conditions {