From 7b3de002a1bf5373504d2b5dde2985cec012aecf Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Thu, 3 Mar 2022 13:15:39 -0500 Subject: [PATCH] Use externalTrafficPolicy: Cluster with OVN OVN-Kubernetes in OpenShift 4.9 and earlier does not support externalTrafficPolicy: Local, and specifying it is reported to result in imbalanced traffic for some users. This commit checks the cluster network type and sets externalTrafficPolicy: Cluster on the LoadBalancer-type service that the operator creates for ingress if the network type is "OVN-Kubernetes". This commit fixes bug 2060542. https://bugzilla.redhat.com/show_bug.cgi?id=2060542 * pkg/operator/controller/ingress/controller.go (ensureIngressController): Pass the network config to ensureLoadBalancerService and syncIngressControllerStatus. * pkg/operator/controller/ingress/load_balancer_service.go (ensureLoadBalancerService, loadBalancerServiceIsUpgradeable): Add a parameter for the network config. Pass the network type from the network config to desiredLoadBalancerService. (desiredLoadBalancerService): Add a parameter for the network type. Set externalTrafficPolicy: Cluster if the network type is "OVN-Kubernetes". * pkg/operator/controller/ingress/load_balancer_service_test.go (TestDesiredLoadBalancerService): Add a test case for OVN-Kubernetes. * pkg/operator/controller/ingress/status.go (syncIngressControllerStatus): Add a parameter for the network config. Pass the config to computeIngressUpgradeableCondition. (computeIngressUpgradeableCondition): Add a parameter for the network config. Pass the network config to loadBalancerServiceIsUpgradeable. * pkg/operator/controller/ingress/status_test.go (TestComputeIngressUpgradeableCondition): Pass the network type to desiredLoadBalancerService and the network config to computeIngressUpgradeableCondition. --- pkg/operator/controller/ingress/controller.go | 4 +- .../ingress/load_balancer_service.go | 16 +++-- .../ingress/load_balancer_service_test.go | 58 +++++++++++++++---- pkg/operator/controller/ingress/status.go | 8 +-- .../controller/ingress/status_test.go | 9 ++- 5 files changed, 71 insertions(+), 24 deletions(-) diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 83b3a8eb9..6f245cba7 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -760,7 +760,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d var lbService *corev1.Service var wildcardRecord *iov1.DNSRecord - if haveLB, lb, err := r.ensureLoadBalancerService(ci, deploymentRef, infraConfig); err != nil { + if haveLB, lb, err := r.ensureLoadBalancerService(ci, deploymentRef, infraConfig, networkConfig); err != nil { errs = append(errs, fmt.Errorf("failed to ensure load balancer service for %s: %v", ci.Name, err)) } else { lbService = lb @@ -799,7 +799,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d errs = append(errs, fmt.Errorf("failed to list pods in namespace %q: %v", operatorcontroller.DefaultOperatorNamespace, err)) } - errs = append(errs, r.syncIngressControllerStatus(ci, deployment, deploymentRef, pods.Items, lbService, operandEvents.Items, wildcardRecord, dnsConfig, infraConfig)) + errs = append(errs, r.syncIngressControllerStatus(ci, deployment, deploymentRef, pods.Items, lbService, operandEvents.Items, wildcardRecord, dnsConfig, infraConfig, networkConfig)) return retryable.NewMaybeRetryableAggregate(errs) } diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 40a645b2a..5a19ed0a2 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -156,12 +156,12 @@ 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, infraConfig *configv1.Infrastructure) (bool, *corev1.Service, error) { +func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, infraConfig *configv1.Infrastructure, networkConfig *configv1.Network) (bool, *corev1.Service, error) { platform, err := oputil.GetPlatformStatus(r.client, infraConfig) if err != nil { return false, nil, fmt.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err) } - wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platform) + wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platform, networkConfig.Spec.NetworkType) if err != nil { return false, nil, err } @@ -222,7 +222,7 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, // 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) (bool, *corev1.Service, error) { +func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, networkType string) (bool, *corev1.Service, error) { if ci.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType { return false, nil, nil } @@ -269,6 +269,12 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef } } } + // externalTrafficPolicy: Local is unsupported by OVN in + // OpenShift 4.9 and earlier; see + // . + if networkType == string(operatorv1.NetworkTypeOVNKubernetes) { + service.Spec.ExternalTrafficPolicy = corev1.ServiceExternalTrafficPolicyTypeCluster + } switch platform.Type { case configv1.AWSPlatformType: if ci.Status.EndpointPublishingStrategy.LoadBalancer != nil && @@ -560,8 +566,8 @@ func loadBalancerServiceTagsModified(current, expected *corev1.Service) (bool, * // return value is nil. Otherwise, if something or someone else has modified // 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) error { - want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform) +func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, current *corev1.Service, platform *configv1.PlatformStatus, networkConfig *configv1.Network) error { + want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, networkConfig.Spec.NetworkType) if err != nil { return err } diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index 29c68e47e..a0f5f630a 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -23,7 +23,9 @@ func TestDesiredLoadBalancerService(t *testing.T) { proxyNeeded bool expect bool platformStatus configv1.PlatformStatus + networkType string expectedResourceTags string + expectedETP string }{ { description: "external classic load balancer with scope for aws platform", @@ -159,7 +161,8 @@ func TestDesiredLoadBalancerService(t *testing.T) { lbStrategy: operatorv1.LoadBalancerStrategy{ Scope: operatorv1.ExternalLoadBalancer, }, - expect: true, + expect: true, + expectedETP: "Cluster", }, { description: "internal load balancer for ibm platform", @@ -168,7 +171,8 @@ func TestDesiredLoadBalancerService(t *testing.T) { lbStrategy: operatorv1.LoadBalancerStrategy{ Scope: operatorv1.InternalLoadBalancer, }, - expect: true, + expect: true, + expectedETP: "Cluster", }, { description: "external load balancer for azure platform", @@ -254,6 +258,18 @@ func TestDesiredLoadBalancerService(t *testing.T) { }, expect: true, }, + { + description: "external classic load balancer with scope for aws platform using OVN", + platform: configv1.AWSPlatformType, + strategyType: operatorv1.LoadBalancerServiceStrategyType, + lbStrategy: operatorv1.LoadBalancerStrategy{ + Scope: operatorv1.ExternalLoadBalancer, + }, + proxyNeeded: true, + networkType: string(operatorv1.NetworkTypeOVNKubernetes), + expectedETP: "Cluster", + expect: true, + }, } for _, tc := range testCases { @@ -282,6 +298,14 @@ func TestDesiredLoadBalancerService(t *testing.T) { }, } infraConfig.Status.PlatformStatus.Type = tc.platform + networkType := tc.networkType + if len(networkType) == 0 { + networkType = string(operatorv1.NetworkTypeOpenShiftSDN) + } + expectedETP := tc.expectedETP + if len(expectedETP) == 0 { + expectedETP = "Local" + } proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) switch { @@ -291,7 +315,7 @@ func TestDesiredLoadBalancerService(t *testing.T) { t.Errorf("test %q failed; expected IsProxyProtocolNeeded to return %v, got %v", tc.description, tc.proxyNeeded, proxyNeeded) } - haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus) + haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, networkType) switch { case err != nil: t.Errorf("test %q failed; unexpected error from desiredLoadBalancerService for endpoint publishing strategy type %v: %v", tc.description, tc.strategyType, err) @@ -320,8 +344,10 @@ func TestDesiredLoadBalancerService(t *testing.T) { if err := checkServiceHasAnnotation(svc, awsLBHealthCheckHealthyThresholdAnnotation, true, awsLBHealthCheckHealthyThresholdDefault); err != nil { t.Errorf("annotation check for test %q failed: %v", tc.description, err) } - if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil { - t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err) + if svc.Spec.ExternalTrafficPolicy == "Local" { + if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil { + t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err) + } } classicLB := tc.lbStrategy.ProviderParameters == nil || tc.lbStrategy.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer switch { @@ -392,8 +418,10 @@ func TestDesiredLoadBalancerService(t *testing.T) { t.Errorf("annotation check for test %q failed; unexpected annotation %s", tc.description, azureInternalLBAnnotation) } } - if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil { - t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err) + if svc.Spec.ExternalTrafficPolicy == "Local" { + if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil { + t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err) + } } case configv1.GCPPlatformType: if isInternal { @@ -406,8 +434,10 @@ func TestDesiredLoadBalancerService(t *testing.T) { t.Errorf("annotation check for test %q failed; unexpected annotation %s", tc.description, gcpLBTypeAnnotation) } } - if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil { - t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err) + if svc.Spec.ExternalTrafficPolicy == "Local" { + if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil { + t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err) + } } case configv1.OpenStackPlatformType: if isInternal { @@ -420,10 +450,16 @@ func TestDesiredLoadBalancerService(t *testing.T) { t.Errorf("annotation check for test %q failed; unexpected annotation %s", tc.description, openstackInternalLBAnnotation) } } - if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil { - t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err) + if svc.Spec.ExternalTrafficPolicy == "Local" { + if err := checkServiceHasAnnotation(svc, localWithFallbackAnnotation, true, ""); err != nil { + t.Errorf("local-with-fallback annotation check for test %q failed: %v", tc.description, err) + } } } + + if svc != nil && string(svc.Spec.ExternalTrafficPolicy) != expectedETP { + t.Errorf("expected spec.externalTrafficPolicy to be %q, got %q", expectedETP, svc.Spec.ExternalTrafficPolicy) + } } } diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index 62f642104..39dd05b09 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -50,7 +50,7 @@ type expectedCondition struct { // syncIngressControllerStatus computes the current status of ic and // updates status upon any changes since last sync. -func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressController, deployment *appsv1.Deployment, deploymentRef metav1.OwnerReference, pods []corev1.Pod, service *corev1.Service, operandEvents []corev1.Event, wildcardRecord *iov1.DNSRecord, dnsConfig *configv1.DNS, infraConfig *configv1.Infrastructure) error { +func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressController, deployment *appsv1.Deployment, deploymentRef metav1.OwnerReference, pods []corev1.Pod, service *corev1.Service, operandEvents []corev1.Event, wildcardRecord *iov1.DNSRecord, dnsConfig *configv1.DNS, infraConfig *configv1.Infrastructure, networkConfig *configv1.Network) error { selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) if err != nil { return fmt.Errorf("deployment has invalid spec.selector: %v", err) @@ -83,7 +83,7 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle degradedCondition, err := computeIngressDegradedCondition(updated.Status.Conditions, updated.Name) errs = append(errs, err) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, degradedCondition) - updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressUpgradeableCondition(ic, deploymentRef, service, platform, secret)) + updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressUpgradeableCondition(ic, deploymentRef, service, platform, networkConfig, secret)) updated.Status.Conditions = PruneConditions(updated.Status.Conditions) @@ -548,13 +548,13 @@ func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition, } // computeIngressUpgradeableCondition computes the IngressController's "Upgradeable" status condition. -func computeIngressUpgradeableCondition(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, service *corev1.Service, platform *configv1.PlatformStatus, secret *corev1.Secret) operatorv1.OperatorCondition { +func computeIngressUpgradeableCondition(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, service *corev1.Service, platform *configv1.PlatformStatus, networkConfig *configv1.Network, secret *corev1.Secret) operatorv1.OperatorCondition { var errs []error errs = append(errs, checkDefaultCertificate(secret, "*."+ic.Status.Domain)) if service != nil { - errs = append(errs, loadBalancerServiceIsUpgradeable(ic, deploymentRef, service, platform)) + errs = append(errs, loadBalancerServiceIsUpgradeable(ic, deploymentRef, service, platform, networkConfig)) } if err := kerrors.NewAggregate(errs); err != nil { diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 6723a1361..4264f0770 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -1312,7 +1312,12 @@ func TestComputeIngressUpgradeableCondition(t *testing.T) { }, }, } - wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus) + networkConfig := &configv1.Network{ + Spec: configv1.NetworkSpec{ + NetworkType: "OpenShiftSDN", + }, + } + wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, networkConfig.Spec.NetworkType) if err != nil { t.Errorf("%q: unexpected error from desiredLoadBalancerService: %v", tc.description, err) return @@ -1334,7 +1339,7 @@ func TestComputeIngressUpgradeableCondition(t *testing.T) { expectedStatus = operatorv1.ConditionTrue } - actual := computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, secret) + actual := computeIngressUpgradeableCondition(ic, deploymentRef, service, platformStatus, networkConfig, secret) if actual.Status != expectedStatus { t.Errorf("%q: expected Upgradeable to be %q, got %q", tc.description, expectedStatus, actual.Status) }