From 320dc8a8a0a454eef7a90099af680d563755e933 Mon Sep 17 00:00:00 2001 From: candita Date: Thu, 6 May 2021 19:23:38 -0400 Subject: [PATCH] Bug 1929396: use ExternalTrafficPolicy Cluster by default --- assets/router/service-cloud.yaml | 2 +- pkg/manifests/bindata.go | 8 +- .../ingress/load_balancer_service.go | 30 +++++- .../ingress/load_balancer_service_test.go | 34 ++++--- test/e2e/operator_test.go | 94 +++++++++++++++++++ 5 files changed, 149 insertions(+), 19 deletions(-) diff --git a/assets/router/service-cloud.yaml b/assets/router/service-cloud.yaml index ac030f5983..c7ab7a23b0 100644 --- a/assets/router/service-cloud.yaml +++ b/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 diff --git a/pkg/manifests/bindata.go b/pkg/manifests/bindata.go index 169c430202..1160ae079a 100644 --- a/pkg/manifests/bindata.go +++ b/pkg/manifests/bindata.go @@ -13,7 +13,7 @@ // assets/router/metrics/role.yaml (291B) // assets/router/namespace.yaml (499B) // assets/router/service-account.yaml (213B) -// assets/router/service-cloud.yaml (631B) +// assets/router/service-cloud.yaml (633B) // assets/router/service-internal.yaml (429B) // manifests/00-cluster-role.yaml (2.935kB) // manifests/00-custom-resource-definition-internal.yaml (6.924kB) @@ -362,7 +362,7 @@ func assetsRouterServiceAccountYaml() (*asset, error) { return a, nil } -var _assetsRouterServiceCloudYaml = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x7c\x90\x41\x6b\x14\x41\x10\x85\xef\xfd\x2b\x1e\xec\x39\x41\x31\x07\x99\x63\x72\x12\x82\x2c\xb8\x78\xaf\xf4\xd4\xec\x34\xe9\xa9\x6a\xaa\x6a\x56\xf7\xdf\xcb\xf4\xec\x82\xa2\x78\xec\x07\xf5\xfa\x7b\xdf\x01\xaf\x4a\x23\x9e\xa9\x92\x64\x36\x7c\x63\xbb\x94\xcc\x08\x45\xab\x94\x19\x45\x30\x99\x4a\x40\x27\xc4\xcc\x30\x5d\x83\x6d\x8b\x73\xd5\x75\x04\xcb\xa5\x98\xca\xc2\x12\xfe\x98\x0e\xf8\x22\x67\x63\x77\xbc\xa8\x84\x69\xad\x6c\xf0\xc6\xb9\x4c\x25\xe3\x42\x75\x65\x07\x19\x83\x5a\xab\x85\x47\x50\xc0\x56\x89\xb2\xf0\x63\x7a\x2f\x32\x0e\x77\x82\x44\xad\x7c\x67\xf3\xa2\x32\xe0\xf2\x31\x2d\x1c\x34\x52\xd0\x90\x80\x03\xbe\xd2\xc2\x28\x0e\xe7\xf8\xa3\x02\x10\x5a\xd8\x1b\x65\x1e\xa0\x8d\xc5\xe7\x32\xc5\x43\xd9\xa1\x12\x50\xe9\x8d\xab\x6f\x25\xd8\x18\x86\xdb\x9e\xb4\x31\x6e\x69\x5c\x1b\x0f\xdd\xc9\x5d\x49\x02\x9c\x2b\xe7\x50\xfb\xfb\x6c\x63\x39\xcd\xc5\x41\xd5\x15\x33\x79\x77\xc4\xd3\xc4\xb9\x1b\x5b\xc8\xde\x8b\x9c\xf1\xfa\x8c\xa6\x5a\x11\x64\x67\x0e\x07\x39\x56\x99\x99\x6a\xcc\x57\xfc\x98\x59\x20\xda\xcb\x6e\x7a\x9b\x8e\xbb\xa7\x66\xec\xbc\xd9\x17\x10\x44\x47\xc6\x1b\xcf\x45\xc6\xfe\x8f\xef\xaa\xb6\xd9\xfc\x33\xd8\x84\xea\xc9\x68\x9a\x4a\x3e\x6a\x2d\xf9\xba\x0d\xc9\x54\x13\xd0\xd4\xa2\xaf\x7e\xe8\x82\x06\xcc\x11\xad\xaf\x69\xa6\xa1\x59\xeb\x80\xd3\xcb\x71\x4f\xd4\x62\xc0\xe7\x0f\xfd\xb1\x03\x1f\x7b\x74\xbb\xf9\xbd\xc2\xff\xdb\xf1\xf4\xf4\xe9\x9f\x25\x9e\x7e\x05\x00\x00\xff\xff\x56\xdc\x0d\xe9\x77\x02\x00\x00") +var _assetsRouterServiceCloudYaml = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x7c\x90\x41\x6b\x14\x41\x10\x85\xef\xf3\x2b\x1e\xec\x39\x41\x31\x07\xe9\x63\xf6\x24\x04\x59\x70\xf1\x5e\xe9\xa9\xd9\x69\xd2\x53\xd5\x54\xd5\xac\xee\xbf\x97\xe9\xd9\x80\xa2\x78\xec\x07\xf5\xfa\x7b\xdf\x01\x2f\x4a\x23\x9e\xa9\x92\x64\x36\x7c\x63\xbb\x96\xcc\x08\x45\xab\x94\x19\x45\x30\x99\x4a\x40\x27\xc4\xcc\x30\x5d\x83\x6d\x8b\x73\xd5\x75\x04\xcb\xb5\x98\xca\xc2\x12\xfe\x38\x1c\xf0\x45\x2e\xc6\xee\x38\xaa\x84\x69\xad\x6c\xf0\xc6\xb9\x4c\x25\xe3\x4a\x75\x65\x07\x19\x83\x5a\xab\x85\x47\x50\xc0\x56\x89\xb2\xf0\xe3\xf0\x56\x64\x4c\xef\x04\x03\xb5\xf2\x9d\xcd\x8b\x4a\xc2\xf5\xe3\xb0\x70\xd0\x48\x41\x69\x00\x0e\xf8\x4a\x0b\xa3\x38\x9c\xe3\x8f\x0a\x40\x68\x61\x6f\x94\x39\x41\x1b\x8b\xcf\x65\x8a\x87\xb2\x43\x0d\x40\xa5\x57\xae\xbe\x95\x60\x63\x48\xf7\x3d\xc3\xc6\xb8\xa5\x71\x6b\x9c\xba\x93\x77\x25\x03\xe0\x5c\x39\x87\xda\xdf\x67\x1b\xcb\x79\x2e\x0e\xaa\xae\x98\xc9\xbb\x23\x9e\x26\xce\xdd\xd8\x42\xf6\x56\xe4\x82\x97\x67\x34\xd5\x8a\x20\xbb\x70\x38\xc8\xb1\xca\xcc\x54\x63\xbe\xe1\xc7\xcc\x02\xd1\x5e\x76\xd7\xdb\x74\xdc\x3d\x35\x63\xe7\xcd\xbe\x80\x20\x3a\x32\x5e\x79\x2e\x32\xf6\x7f\x7c\x57\xb5\xcd\xe6\x9f\xc1\x26\x54\xcf\x46\xd3\x54\xf2\x49\x6b\xc9\xb7\x84\x63\x5d\x7d\xe7\x6c\x6a\xd1\x77\x3f\x74\x45\x09\x73\x44\xeb\x7b\x9a\x69\x68\xd6\x9a\x70\x3e\x9e\xf6\x44\x2d\x12\x3e\x7f\xe8\x8f\x1d\xf9\xd4\xa3\xfb\xcd\xef\x15\xfe\xdf\x8e\xa7\xa7\x4f\xff\x2c\xf1\xe1\x57\x00\x00\x00\xff\xff\xc5\x5e\xe2\xc9\x79\x02\x00\x00") func assetsRouterServiceCloudYamlBytes() ([]byte, error) { return bindataRead( @@ -377,8 +377,8 @@ func assetsRouterServiceCloudYaml() (*asset, error) { return nil, err } - info := bindataFileInfo{name: "assets/router/service-cloud.yaml", size: 631, mode: os.FileMode(420), modTime: time.Unix(1, 0)} - a := &asset{bytes: bytes, info: info, digest: [32]uint8{0xcd, 0x97, 0xc4, 0xab, 0x8b, 0xa9, 0xa1, 0x47, 0xf7, 0xf, 0xeb, 0x38, 0x1b, 0xc2, 0xf7, 0x8c, 0xd9, 0xba, 0x35, 0x9b, 0x1, 0x67, 0x26, 0xd7, 0x3f, 0x6d, 0xa5, 0x5f, 0xf0, 0x92, 0x84, 0x8f}} + info := bindataFileInfo{name: "assets/router/service-cloud.yaml", size: 633, mode: os.FileMode(420), modTime: time.Unix(1, 0)} + a := &asset{bytes: bytes, info: info, digest: [32]uint8{0x6a, 0x33, 0x76, 0xf9, 0xc1, 0x19, 0xdd, 0xc3, 0xeb, 0x5, 0xe7, 0xbd, 0xcc, 0x42, 0x6c, 0x51, 0x1b, 0xb8, 0xd7, 0xa8, 0x11, 0x83, 0xee, 0x25, 0x82, 0xbf, 0x87, 0xdb, 0xdf, 0xf8, 0x69, 0xbe}} return a, nil } diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index b19667e6e7..f3d6faa4a8 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/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 ). + // Preserve everything except: + // AWS LB health check interval annotation (see ) , + // GCP Global Access internal Load Balancer annotation (see ), & + // External Traffic Policy (see ). // Updating annotations and spec fields cannot be done unless the // previous release blocks upgrades when the user has modified those // fields (see ). @@ -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 } diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index 4c371fad9b..7bc72fdf26 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/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{ { diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 011e7a7598..49bb726b90 100644 --- a/test/e2e/operator_test.go +++ b/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 {