Add local-with-fallback unsupported config override

* pkg/operator/controller/ingress/load_balancer_service.go
(desiredLoadBalancerService): Use the new shouldUseLocalWithFallback
function to determine whether to set the "local-with-fallback" annotation.
(shouldUseLocalWithFallback): New function.  Check the service's external
traffic policy and the ingresscontroller's unsupported config overrides and
return a Boolean value indicating whether "local-with-fallback" should be
enabled, or an error if the override could not be parsed.
* pkg/operator/controller/ingress/load_balancer_service_test.go
(TestShouldUseLocalWithFallback): New test.  Verify that
shouldUseLocalWithFallback behaves as expected.
* pkg/operator/controller/ingress/nodeport_service.go
(ensureNodePortService): Check the error value from desiredNodePortService.
(desiredNodePortService): Add error return value.  Use the new
shouldUseLocalWithFallback function (which can return an error) to
determine whether to set the "local-with-fallback" annotation.
* pkg/operator/controller/ingress/nodeport_service_test.go
(TestDesiredNodePortService): Check the error value from
desiredNodePortService.
* test/e2e/operator_test.go
(TestLocalWithFallbackOverrideForLoadBalancerService)
(TestLocalWithFallbackOverrideForNodePortService): New tests.  Verify that
the operator does not set the "local-with-fallback" annotation on a
LoadBalancer or NodePort service if the localWithFallback unsupported
config override is set to "false".
Co-authored-by: candita <cholman@redhat.com>

pkg/operator/controller/ingress/load_balancer_service.go

@@ -2,6 +2,7 @@ package ingress
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"strconv"
 	"strings"
@@ -306,7 +307,9 @@ 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)
 
-		if service.Spec.ExternalTrafficPolicy == corev1.ServiceExternalTrafficPolicyTypeLocal {
+		if v, err := shouldUseLocalWithFallback(ci, service); err != nil {
+			return true, service, err
+		} else if v {
 			service.Annotations[localWithFallbackAnnotation] = ""
 		}
 	}
@@ -315,6 +318,38 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 	return true, service, nil
 }
 
+// shouldUseLocalWithFallback returns a Boolean value indicating whether the
+// local-with-fallback annotation should be set for the given service, and
+// returns an error if the given ingresscontroller has an invalid unsupported
+// config override.
+func shouldUseLocalWithFallback(ic *operatorv1.IngressController, service *corev1.Service) (bool, error) {
+	// By default, use local-with-fallback when using the "Local" external
+	// traffic policy.
+	if service.Spec.ExternalTrafficPolicy != corev1.ServiceExternalTrafficPolicyTypeLocal {
+		return false, nil
+	}
+
+	// Allow the user to override local-with-fallback.
+	if len(ic.Spec.UnsupportedConfigOverrides.Raw) > 0 {
+		var unsupportedConfigOverrides struct {
+			LocalWithFallback string `json:"localWithFallback"`
+		}
+		if err := json.Unmarshal(ic.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigOverrides); err != nil {
+			return false, fmt.Errorf("ingresscontroller %q has invalid spec.unsupportedConfigOverrides: %w", ic.Name, err)
+		}
+		override := unsupportedConfigOverrides.LocalWithFallback
+		if len(override) != 0 {
+			if val, err := strconv.ParseBool(override); err != nil {
+				return false, fmt.Errorf("ingresscontroller %q has invalid spec.unsupportedConfigOverrides.localWithFallback: %w", ic.Name, err)
+			} else {
+				return val, nil
+			}
+		}
+	}
+
+	return true, nil
+}
+
 // currentLoadBalancerService returns any existing LB service for the
 // ingresscontroller.
 func (r *reconciler) currentLoadBalancerService(ci *operatorv1.IngressController) (bool, *corev1.Service, error) {

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -10,6 +10,7 @@ import (
 	operatorv1 "github.com/openshift/api/operator/v1"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
@@ -451,6 +452,72 @@ func checkServiceHasAnnotation(svc *corev1.Service, name string, expectValue boo
 	}
 }
 
+// TestShouldUseLocalWithFallback verifies that shouldUseLocalWithFallback
+// behaves as expected.
+func TestShouldUseLocalWithFallback(t *testing.T) {
+	testCases := []struct {
+		description string
+		local       bool
+		override    string
+		expect      bool
+		expectError bool
+	}{
+		{
+			description: "if using Cluster without an override",
+			local:       false,
+			expect:      false,
+		},
+		{
+			description: "if using Local without an override",
+			local:       true,
+			expect:      true,
+		},
+		{
+			description: "if using Local with an override",
+			local:       true,
+			override:    `{"localWithFallback":"false"}`,
+			expect:      false,
+		},
+		{
+			description: "if using Local with a garbage override",
+			local:       true,
+			override:    `{"localWithFallback":"x"}`,
+			expectError: true,
+		},
+	}
+	for _, tc := range testCases {
+		var override []byte
+		if len(tc.override) != 0 {
+			override = []byte(tc.override)
+		}
+		ic := &operatorv1.IngressController{
+			Spec: operatorv1.IngressControllerSpec{
+				UnsupportedConfigOverrides: runtime.RawExtension{
+					Raw: override,
+				},
+			},
+		}
+		policy := corev1.ServiceExternalTrafficPolicyTypeCluster
+		if tc.local {
+			policy = corev1.ServiceExternalTrafficPolicyTypeLocal
+		}
+		service := corev1.Service{
+			Spec: corev1.ServiceSpec{
+				ExternalTrafficPolicy: policy,
+			},
+		}
+		actual, err := shouldUseLocalWithFallback(ic, &service)
+		switch {
+		case !tc.expectError && err != nil:
+			t.Errorf("%q: unexpected error: %w", tc.description, err)
+		case tc.expectError && err == nil:
+			t.Errorf("%q: expected error, got nil", tc.description)
+		case tc.expect != actual:
+			t.Errorf("%q: expected %t, got %t", tc.description, tc.expect, actual)
+		}
+	}
+}
+
 func TestLoadBalancerServiceChanged(t *testing.T) {
 	testCases := []struct {
 		description string

pkg/operator/controller/ingress/nodeport_service.go

@@ -47,7 +47,10 @@ func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, dep
 		}
 	}
 
-	wantService, desired := desiredNodePortService(ic, deploymentRef, wantMetricsPort)
+	wantService, desired, err := desiredNodePortService(ic, deploymentRef, wantMetricsPort)
+	if err != nil {
+		return false, nil, err
+	}
 
 	switch {
 	case !wantService && !haveService:
@@ -80,19 +83,17 @@ func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, dep
 
 // desiredNodePortService returns a Boolean indicating whether a NodePort
 // service is desired, as well as the NodePort service if one is desired.
-func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, wantMetricsPort bool) (bool, *corev1.Service) {
+func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, wantMetricsPort bool) (bool, *corev1.Service, error) {
 	if ic.Status.EndpointPublishingStrategy.Type != operatorv1.NodePortServiceStrategyType {
-		return false, nil
+		return false, nil, nil
 	}
 
 	name := controller.NodePortServiceName(ic)
 	service := &corev1.Service{
 		ObjectMeta: metav1.ObjectMeta{
-			Annotations: map[string]string{
-				localWithFallbackAnnotation: "",
-			},
-			Namespace: name.Namespace,
-			Name:      name.Name,
+			Annotations: map[string]string{},
+			Namespace:   name.Namespace,
+			Name:        name.Name,
 			Labels: map[string]string{
 				"app":                                  "router",
 				"router":                               name.Name,
@@ -130,7 +131,13 @@ func desiredNodePortService(ic *operatorv1.IngressController, deploymentRef meta
 		service.Spec.Ports = service.Spec.Ports[0:2]
 	}
 
-	return true, service
+	if v, err := shouldUseLocalWithFallback(ic, service); err != nil {
+		return true, service, err
+	} else if v {
+		service.Annotations[localWithFallbackAnnotation] = ""
+	}
+
+	return true, service, nil
 }
 
 // currentNodePortService returns a Boolean indicating whether a NodePort

pkg/operator/controller/ingress/nodeport_service_test.go

@@ -132,8 +132,10 @@ func TestDesiredNodePortService(t *testing.T) {
 				},
 			},
 		}
-		want, svc := desiredNodePortService(ic, deploymentRef, tc.wantMetricsPort)
-		if want != tc.expect {
+		want, svc, err := desiredNodePortService(ic, deploymentRef, tc.wantMetricsPort)
+		if err != nil {
+			t.Errorf("unexpected error from desiredNodePortService: %w", err)
+		} else if want != tc.expect {
 			t.Errorf("expected desiredNodePortService to return %t for endpoint publishing strategy type %v, got %t, with service %#v", tc.expect, tc.strategyType, want, svc)
 		} else if tc.expect && !reflect.DeepEqual(svc, &tc.expectService) {
 			t.Errorf("expected desiredNodePortService to return %#v, got %#v", &tc.expectService, svc)

test/e2e/operator_test.go

@@ -1978,6 +1978,128 @@ func TestLoadBalancingAlgorithmUnsupportedConfigOverride(t *testing.T) {
 	}
 }
 
+// TestLocalWithFallbackOverrideForLoadBalancerService verifies that the
+// operator does not set the local-with-fallback annotation on a LoadBalancer
+// service if the the localWithFallback unsupported config override is set to
+// "false".
+//
+// Note: This test mutates the default ingresscontroller rather than creating a
+// new one to reduce the risk of failing due to cloud provider API throttling.
+func TestLocalWithFallbackOverrideForLoadBalancerService(t *testing.T) {
+	supportedPlatforms := map[configv1.PlatformType]struct{}{
+		configv1.AWSPlatformType:   {},
+		configv1.AzurePlatformType: {},
+		configv1.GCPPlatformType:   {},
+	}
+	platform := infraConfig.Status.Platform
+	if _, supported := supportedPlatforms[platform]; !supported {
+		t.Skipf("test skipped on platform %q", platform)
+	}
+
+	ic := &operatorv1.IngressController{}
+	if err := kclient.Get(context.TODO(), defaultName, ic); err != nil {
+		t.Fatalf("failed to get ingresscontroller %q: %v", defaultName, err)
+	}
+
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, defaultName, defaultAvailableConditions...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	service := &corev1.Service{}
+	serviceName := controller.LoadBalancerServiceName(ic)
+	if err := kclient.Get(context.TODO(), serviceName, service); err != nil {
+		t.Fatalf("failed to get service %q: %v", serviceName, err)
+	}
+
+	const annotation = "traffic-policy.network.alpha.openshift.io/local-with-fallback"
+
+	if _, ok := service.Annotations[annotation]; !ok {
+		t.Fatalf("failed to observe the %q annotation on service %q", annotation, serviceName)
+	}
+
+	ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{
+		Raw: []byte(`{"localWithFallback":"false"}`),
+	}
+	if err := kclient.Update(context.TODO(), ic); err != nil {
+		t.Fatalf("failed to update ingresscontroller %q with override: %v", defaultName, err)
+	}
+	defer func() {
+		if err := kclient.Get(context.TODO(), defaultName, ic); err != nil {
+			t.Fatalf("failed to get ingresscontroller %q: %v", defaultName, err)
+		}
+		ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{}
+		if err := kclient.Update(context.TODO(), ic); err != nil {
+			t.Fatalf("failed to update ingresscontroller %q to remove the override: %v", defaultName, 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 service %q: %v", serviceName, err)
+			return false, nil
+		}
+		_, ok := service.Annotations[annotation]
+		return !ok, nil
+	})
+	if _, ok := service.Annotations[annotation]; ok {
+		t.Fatalf("failed to observe removal of the %q annotation on service %q", annotation, serviceName)
+	}
+}
+
+// TestLocalWithFallbackOverrideForNodePortService verifies that the operator
+// does not set the local-with-fallback annotation on a NodePort service if the
+// the localWithFallback unsupported config override is set to "false".
+func TestLocalWithFallbackOverrideForNodePortService(t *testing.T) {
+	icName := types.NamespacedName{
+		Namespace: operatorNamespace,
+		Name:      "local-with-fallback",
+	}
+	domain := icName.Name + "." + dnsConfig.Spec.BaseDomain
+	ic := newNodePortController(icName, domain)
+	if err := kclient.Create(context.TODO(), ic); err != nil {
+		t.Fatalf("failed to create ingresscontroller %q: %v", icName, err)
+	}
+	defer assertIngressControllerDeleted(t, kclient, ic)
+
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, availableConditionsForIngressControllerWithNodePort...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	service := &corev1.Service{}
+	serviceName := controller.NodePortServiceName(ic)
+	if err := kclient.Get(context.TODO(), serviceName, service); err != nil {
+		t.Fatalf("failed to get service %q: %v", serviceName, err)
+	}
+
+	const annotation = "traffic-policy.network.alpha.openshift.io/local-with-fallback"
+
+	if _, ok := service.Annotations[annotation]; !ok {
+		t.Fatalf("failed to observe the %q annotation on ingresscontroller %q", annotation, icName)
+	}
+
+	if err := kclient.Get(context.TODO(), icName, ic); err != nil {
+		t.Fatalf("failed to get ingresscontroller %q: %v", icName, err)
+	}
+	ic.Spec.UnsupportedConfigOverrides = runtime.RawExtension{
+		Raw: []byte(`{"localWithFallback":"false"}`),
+	}
+	if err := kclient.Update(context.TODO(), ic); err != nil {
+		t.Fatalf("failed to update ingresscontroller %q with override: %v", icName, 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 service %q: %v", serviceName, err)
+			return false, nil
+		}
+		_, ok := service.Annotations[annotation]
+		return !ok, nil
+	})
+	if _, ok := service.Annotations[annotation]; ok {
+		t.Fatalf("failed to observe removal of the %q annotation on service %q", annotation, serviceName)
+	}
+}
+
 func newLoadBalancerController(name types.NamespacedName, domain string) *operatorv1.IngressController {
 	repl := int32(1)
 	return &operatorv1.IngressController{