From 2b69daa960ccea9a19f58e73a9079db94bc0819d Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Fri, 7 Jul 2023 11:51:55 +0800 Subject: [PATCH] Allow specifying ExternalTrafficPolicy for ClusterIP Services with ExternalIPs When defining a ClusterIP Service, we can specify externalIP, and the traffic policy of externalIP is subject to externalTrafficPolicy. However, the policy can't be set when type is not NodePort or LoadBalancer, and will default to Cluster when kube-proxy processes the Service. This commit updates the defaulting and validation of Service to allow specifying ExternalTrafficPolicy for ClusterIP Services with ExternalIPs. Signed-off-by: Quan Tian --- .../.import-restrictions | 1 + pkg/api/service/util.go | 7 ++ pkg/api/service/util_test.go | 43 ++++++++++ pkg/api/v1/service/util.go | 12 ++- pkg/api/v1/service/util_test.go | 63 ++++++++++++++ pkg/apis/core/v1/defaults.go | 7 +- pkg/apis/core/v1/defaults_test.go | 21 +++++ pkg/apis/core/validation/validation.go | 8 +- pkg/apis/core/validation/validation_test.go | 25 ++++++ pkg/kubectl/.import-restrictions | 1 + pkg/registry/core/service/strategy.go | 6 +- pkg/registry/core/service/strategy_test.go | 22 ++++- staging/publishing/import-restrictions.yaml | 1 + test/integration/service/service_test.go | 84 +++++++++++++++++++ 14 files changed, 282 insertions(+), 19 deletions(-) diff --git a/cmd/cloud-controller-manager/.import-restrictions b/cmd/cloud-controller-manager/.import-restrictions index 53eed5eb7b8..5b8a0d3582f 100644 --- a/cmd/cloud-controller-manager/.import-restrictions +++ b/cmd/cloud-controller-manager/.import-restrictions @@ -7,6 +7,7 @@ rules: - k8s.io/kubernetes/pkg/api/legacyscheme - k8s.io/kubernetes/pkg/api/service - k8s.io/kubernetes/pkg/api/v1/pod + - k8s.io/kubernetes/pkg/api/v1/service - k8s.io/kubernetes/pkg/apis/apps - k8s.io/kubernetes/pkg/apis/autoscaling - k8s.io/kubernetes/pkg/apis/core diff --git a/pkg/api/service/util.go b/pkg/api/service/util.go index c73d96a6c4d..fefb13b2b68 100644 --- a/pkg/api/service/util.go +++ b/pkg/api/service/util.go @@ -67,6 +67,13 @@ func GetLoadBalancerSourceRanges(service *api.Service) (utilnet.IPNetSet, error) return ipnets, nil } +// ExternallyAccessible checks if service is externally accessible. +func ExternallyAccessible(service *api.Service) bool { + return service.Spec.Type == api.ServiceTypeLoadBalancer || + service.Spec.Type == api.ServiceTypeNodePort || + (service.Spec.Type == api.ServiceTypeClusterIP && len(service.Spec.ExternalIPs) > 0) +} + // RequestsOnlyLocalTraffic checks if service requests OnlyLocal traffic. func RequestsOnlyLocalTraffic(service *api.Service) bool { if service.Spec.Type != api.ServiceTypeLoadBalancer && diff --git a/pkg/api/service/util_test.go b/pkg/api/service/util_test.go index 5a308239c5c..a0fc76861b5 100644 --- a/pkg/api/service/util_test.go +++ b/pkg/api/service/util_test.go @@ -129,6 +129,49 @@ func TestAllowAll(t *testing.T) { checkAllowAll(true, "192.168.0.1/32", "0.0.0.0/0") } +func TestExternallyAccessible(t *testing.T) { + checkExternallyAccessible := func(expect bool, service *api.Service) { + res := ExternallyAccessible(service) + if res != expect { + t.Errorf("Expected ExternallyAccessible = %v, got %v", expect, res) + } + } + + checkExternallyAccessible(false, &api.Service{}) + checkExternallyAccessible(false, &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeClusterIP, + }, + }) + checkExternallyAccessible(true, &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeClusterIP, + ExternalIPs: []string{"1.2.3.4"}, + }, + }) + checkExternallyAccessible(true, &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeLoadBalancer, + }, + }) + checkExternallyAccessible(true, &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeNodePort, + }, + }) + checkExternallyAccessible(false, &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeExternalName, + }, + }) + checkExternallyAccessible(false, &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeExternalName, + ExternalIPs: []string{"1.2.3.4"}, + }, + }) +} + func TestRequestsOnlyLocalTraffic(t *testing.T) { checkRequestsOnlyLocalTraffic := func(requestsOnlyLocalTraffic bool, service *api.Service) { res := RequestsOnlyLocalTraffic(service) diff --git a/pkg/api/v1/service/util.go b/pkg/api/v1/service/util.go index 683b8ed1f7d..b051c417928 100644 --- a/pkg/api/v1/service/util.go +++ b/pkg/api/v1/service/util.go @@ -67,10 +67,16 @@ func GetLoadBalancerSourceRanges(service *v1.Service) (utilnet.IPNetSet, error) return ipnets, nil } -// ExternalPolicyLocal checks if service has ETP = Local. +// ExternallyAccessible checks if service is externally accessible. +func ExternallyAccessible(service *v1.Service) bool { + return service.Spec.Type == v1.ServiceTypeLoadBalancer || + service.Spec.Type == v1.ServiceTypeNodePort || + (service.Spec.Type == v1.ServiceTypeClusterIP && len(service.Spec.ExternalIPs) > 0) +} + +// ExternalPolicyLocal checks if service is externally accessible and has ETP = Local. func ExternalPolicyLocal(service *v1.Service) bool { - if service.Spec.Type != v1.ServiceTypeLoadBalancer && - service.Spec.Type != v1.ServiceTypeNodePort { + if !ExternallyAccessible(service) { return false } return service.Spec.ExternalTrafficPolicy == v1.ServiceExternalTrafficPolicyLocal diff --git a/pkg/api/v1/service/util_test.go b/pkg/api/v1/service/util_test.go index eead2cf26f3..5d9f02bc5de 100644 --- a/pkg/api/v1/service/util_test.go +++ b/pkg/api/v1/service/util_test.go @@ -129,6 +129,49 @@ func TestAllowAll(t *testing.T) { checkAllowAll(true, "192.168.0.1/32", "0.0.0.0/0") } +func TestExternallyAccessible(t *testing.T) { + checkExternallyAccessible := func(expect bool, service *v1.Service) { + res := ExternallyAccessible(service) + if res != expect { + t.Errorf("Expected ExternallyAccessible = %v, got %v", expect, res) + } + } + + checkExternallyAccessible(false, &v1.Service{}) + checkExternallyAccessible(false, &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeClusterIP, + }, + }) + checkExternallyAccessible(true, &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeClusterIP, + ExternalIPs: []string{"1.2.3.4"}, + }, + }) + checkExternallyAccessible(true, &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeLoadBalancer, + }, + }) + checkExternallyAccessible(true, &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeNodePort, + }, + }) + checkExternallyAccessible(false, &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeExternalName, + }, + }) + checkExternallyAccessible(false, &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeExternalName, + ExternalIPs: []string{"1.2.3.4"}, + }, + }) +} + func TestExternalPolicyLocal(t *testing.T) { checkExternalPolicyLocal := func(requestsOnlyLocalTraffic bool, service *v1.Service) { res := ExternalPolicyLocal(service) @@ -144,6 +187,26 @@ func TestExternalPolicyLocal(t *testing.T) { Type: v1.ServiceTypeClusterIP, }, }) + checkExternalPolicyLocal(false, &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeClusterIP, + ExternalIPs: []string{"1.2.3.4"}, + }, + }) + checkExternalPolicyLocal(false, &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeClusterIP, + ExternalIPs: []string{"1.2.3.4"}, + ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyCluster, + }, + }) + checkExternalPolicyLocal(true, &v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeClusterIP, + ExternalIPs: []string{"1.2.3.4"}, + ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyLocal, + }, + }) checkExternalPolicyLocal(false, &v1.Service{ Spec: v1.ServiceSpec{ Type: v1.ServiceTypeNodePort, diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index e3a43b1fbd7..8b645ebf49d 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/kubernetes/pkg/api/v1/service" "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/util/parsers" "k8s.io/utils/pointer" @@ -122,11 +123,9 @@ func SetDefaults_Service(obj *v1.Service) { sp.TargetPort = intstr.FromInt32(sp.Port) } } - // Defaults ExternalTrafficPolicy field for NodePort / LoadBalancer service + // Defaults ExternalTrafficPolicy field for externally-accessible service // to Global for consistency. - if (obj.Spec.Type == v1.ServiceTypeNodePort || - obj.Spec.Type == v1.ServiceTypeLoadBalancer) && - obj.Spec.ExternalTrafficPolicy == "" { + if service.ExternallyAccessible(obj) && obj.Spec.ExternalTrafficPolicy == "" { obj.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyCluster } diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index c6db824c620..5377a57010c 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/pkg/apis/core/v1/defaults_test.go @@ -1631,6 +1631,27 @@ func TestSetDefaultServiceExternalTraffic(t *testing.T) { if out.Spec.ExternalTrafficPolicy != v1.ServiceExternalTrafficPolicyCluster { t.Errorf("Expected ExternalTrafficPolicy to be %v, got %v", v1.ServiceExternalTrafficPolicyCluster, out.Spec.ExternalTrafficPolicy) } + + in = &v1.Service{Spec: v1.ServiceSpec{Type: v1.ServiceTypeClusterIP, ExternalIPs: []string{"1.2.3.4"}}} + obj = roundTrip(t, runtime.Object(in)) + out = obj.(*v1.Service) + if out.Spec.ExternalTrafficPolicy != v1.ServiceExternalTrafficPolicyCluster { + t.Errorf("Expected ExternalTrafficPolicy to be %v, got %v", v1.ServiceExternalTrafficPolicyCluster, out.Spec.ExternalTrafficPolicy) + } + + in = &v1.Service{Spec: v1.ServiceSpec{Type: v1.ServiceTypeClusterIP}} + obj = roundTrip(t, runtime.Object(in)) + out = obj.(*v1.Service) + if out.Spec.ExternalTrafficPolicy != "" { + t.Errorf("Expected ExternalTrafficPolicy to be empty, got %v", out.Spec.ExternalTrafficPolicy) + } + + in = &v1.Service{Spec: v1.ServiceSpec{Type: v1.ServiceTypeExternalName}} + obj = roundTrip(t, runtime.Object(in)) + out = obj.(*v1.Service) + if out.Spec.ExternalTrafficPolicy != "" { + t.Errorf("Expected ExternalTrafficPolicy to be empty, got %v", out.Spec.ExternalTrafficPolicy) + } } func TestSetDefaultNamespace(t *testing.T) { diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 29c92c41fbe..d66823eeb85 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -5332,10 +5332,6 @@ func validateServicePort(sp *core.ServicePort, requireName, isHeadlessService bo return allErrs } -func needsExternalTrafficPolicy(svc *core.Service) bool { - return svc.Spec.Type == core.ServiceTypeLoadBalancer || svc.Spec.Type == core.ServiceTypeNodePort -} - var validExternalTrafficPolicies = sets.NewString( string(core.ServiceExternalTrafficPolicyCluster), string(core.ServiceExternalTrafficPolicyLocal)) @@ -5345,10 +5341,10 @@ func validateServiceExternalTrafficPolicy(service *core.Service) field.ErrorList fldPath := field.NewPath("spec") - if !needsExternalTrafficPolicy(service) { + if !apiservice.ExternallyAccessible(service) { if service.Spec.ExternalTrafficPolicy != "" { allErrs = append(allErrs, field.Invalid(fldPath.Child("externalTrafficPolicy"), service.Spec.ExternalTrafficPolicy, - "may only be set when `type` is 'NodePort' or 'LoadBalancer'")) + "may only be set for externally-accessible services")) } } else { if service.Spec.ExternalTrafficPolicy == "" { diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 262db4b4c4c..647cbae6a56 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -14542,27 +14542,38 @@ func TestValidateServiceCreate(t *testing.T) { }, { name: "invalid publicIPs localhost", tweakSvc: func(s *core.Service) { + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"127.0.0.1"} }, numErrs: 1, }, { name: "invalid publicIPs unspecified", tweakSvc: func(s *core.Service) { + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"0.0.0.0"} }, numErrs: 1, }, { name: "invalid publicIPs loopback", tweakSvc: func(s *core.Service) { + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"127.0.0.1"} }, numErrs: 1, }, { name: "invalid publicIPs host", tweakSvc: func(s *core.Service) { + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster s.Spec.ExternalIPs = []string{"myhost.mydomain"} }, numErrs: 1, + }, { + name: "valid publicIPs", + tweakSvc: func(s *core.Service) { + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyCluster + s.Spec.ExternalIPs = []string{"1.2.3.4"} + }, + numErrs: 0, }, { name: "dup port name", tweakSvc: func(s *core.Service) { @@ -15540,6 +15551,13 @@ func TestValidateServiceExternalTrafficPolicy(t *testing.T) { s.Spec.HealthCheckNodePort = 34567 }, numErrs: 2, + }, { + name: "cannot set externalTrafficPolicy field on ExternalName service", + tweakSvc: func(s *core.Service) { + s.Spec.Type = core.ServiceTypeExternalName + s.Spec.ExternalTrafficPolicy = core.ServiceExternalTrafficPolicyLocal + }, + numErrs: 1, }, { name: "externalTrafficPolicy is required on NodePort service", tweakSvc: func(s *core.Service) { @@ -15552,6 +15570,13 @@ func TestValidateServiceExternalTrafficPolicy(t *testing.T) { s.Spec.Type = core.ServiceTypeLoadBalancer }, numErrs: 1, + }, { + name: "externalTrafficPolicy is required on ClusterIP service with externalIPs", + tweakSvc: func(s *core.Service) { + s.Spec.Type = core.ServiceTypeClusterIP + s.Spec.ExternalIPs = []string{"1.2.3,4"} + }, + numErrs: 1, }, } diff --git a/pkg/kubectl/.import-restrictions b/pkg/kubectl/.import-restrictions index ec6d8e80176..d7075253226 100644 --- a/pkg/kubectl/.import-restrictions +++ b/pkg/kubectl/.import-restrictions @@ -2,6 +2,7 @@ rules: - selectorRegexp: k8s[.]io/kubernetes/pkg allowedPrefixes: - k8s.io/kubernetes/pkg/api/legacyscheme + - k8s.io/kubernetes/pkg/api/v1/service - k8s.io/kubernetes/pkg/apis/admission - k8s.io/kubernetes/pkg/apis/admission/install - k8s.io/kubernetes/pkg/apis/admission/v1 diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go index b11a6329834..4a91778a37f 100644 --- a/pkg/registry/core/service/strategy.go +++ b/pkg/registry/core/service/strategy.go @@ -277,7 +277,7 @@ func dropTypeDependentFields(newSvc *api.Service, oldSvc *api.Service) { // If a user is switching to a type that doesn't need ExternalTrafficPolicy // AND they did not change this field, it is safe to drop it. - if needsExternalTrafficPolicy(oldSvc) && !needsExternalTrafficPolicy(newSvc) && sameExternalTrafficPolicy(oldSvc, newSvc) { + if serviceapi.ExternallyAccessible(oldSvc) && !serviceapi.ExternallyAccessible(newSvc) && sameExternalTrafficPolicy(oldSvc, newSvc) { newSvc.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicy("") } @@ -376,10 +376,6 @@ func sameLoadBalancerClass(oldSvc, newSvc *api.Service) bool { return *oldSvc.Spec.LoadBalancerClass == *newSvc.Spec.LoadBalancerClass } -func needsExternalTrafficPolicy(svc *api.Service) bool { - return svc.Spec.Type == api.ServiceTypeNodePort || svc.Spec.Type == api.ServiceTypeLoadBalancer -} - func sameExternalTrafficPolicy(oldSvc, newSvc *api.Service) bool { return oldSvc.Spec.ExternalTrafficPolicy == newSvc.Spec.ExternalTrafficPolicy } diff --git a/pkg/registry/core/service/strategy_test.go b/pkg/registry/core/service/strategy_test.go index 40f4262f5be..d36ed678adb 100644 --- a/pkg/registry/core/service/strategy_test.go +++ b/pkg/registry/core/service/strategy_test.go @@ -510,6 +510,18 @@ func TestDropTypeDependentFields(t *testing.T) { svc.Spec.Ports[i].NodePort += 100 } } + setExternalIPs := func(svc *api.Service) { + svc.Spec.ExternalIPs = []string{"1.1.1.1"} + } + clearExternalIPs := func(svc *api.Service) { + svc.Spec.ExternalIPs = nil + } + setExternalTrafficPolicyCluster := func(svc *api.Service) { + svc.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyCluster + } + clearExternalTrafficPolicy := func(svc *api.Service) { + svc.Spec.ExternalTrafficPolicy = "" + } clearIPFamilies := func(svc *api.Service) { svc.Spec.IPFamilies = nil } @@ -651,7 +663,7 @@ func TestDropTypeDependentFields(t *testing.T) { name: "don't clear changed healthCheckNodePort", svc: makeValidServiceCustom(setTypeLoadBalancer, setHCNodePort), patch: patches(setTypeClusterIP, changeHCNodePort), - expect: makeValidServiceCustom(setHCNodePort, changeHCNodePort), + expect: makeValidServiceCustom(setHCNodePort, changeHCNodePort, clearExternalTrafficPolicy), }, { // allocatedLoadBalancerNodePorts cases name: "clear allocatedLoadBalancerNodePorts true -> true", svc: makeValidServiceCustom(setTypeLoadBalancer, setAllocateLoadBalancerNodePortsTrue), @@ -722,6 +734,11 @@ func TestDropTypeDependentFields(t *testing.T) { svc: makeValidServiceCustom(setTypeLoadBalancer, setLoadBalancerClass), patch: nil, expect: makeValidServiceCustom(setTypeLoadBalancer, setLoadBalancerClass), + }, { + name: "clear externalTrafficPolicy when removing externalIPs for Type=ClusterIP", + svc: makeValidServiceCustom(setTypeClusterIP, setExternalIPs, setExternalTrafficPolicyCluster), + patch: patches(clearExternalIPs), + expect: makeValidServiceCustom(setTypeClusterIP, clearExternalTrafficPolicy), }} for _, tc := range testCases { @@ -759,6 +776,9 @@ func TestDropTypeDependentFields(t *testing.T) { if !reflect.DeepEqual(result.Spec.LoadBalancerClass, tc.expect.Spec.LoadBalancerClass) { t.Errorf("failed %q: expected LoadBalancerClass %v, got %v", tc.name, tc.expect.Spec.LoadBalancerClass, result.Spec.LoadBalancerClass) } + if !reflect.DeepEqual(result.Spec.ExternalTrafficPolicy, tc.expect.Spec.ExternalTrafficPolicy) { + t.Errorf("failed %q: expected ExternalTrafficPolicy %v, got %v", tc.name, tc.expect.Spec.ExternalTrafficPolicy, result.Spec.ExternalTrafficPolicy) + } }) } } diff --git a/staging/publishing/import-restrictions.yaml b/staging/publishing/import-restrictions.yaml index 7d3da19b4ce..0808db2176e 100644 --- a/staging/publishing/import-restrictions.yaml +++ b/staging/publishing/import-restrictions.yaml @@ -4,6 +4,7 @@ - k8s.io/apiserver/pkg/util/feature - k8s.io/component-base/featuregate/testing - k8s.io/kubernetes/pkg/apis/core + - k8s.io/kubernetes/pkg/api/v1/service - k8s.io/kubernetes/pkg/features - k8s.io/kubernetes/pkg/fieldpath - k8s.io/kubernetes/pkg/util diff --git a/test/integration/service/service_test.go b/test/integration/service/service_test.go index b721b72513e..9dc6543a09c 100644 --- a/test/integration/service/service_test.go +++ b/test/integration/service/service_test.go @@ -180,3 +180,87 @@ func Test_ConvertingToExternalNameServiceDropsInternalTrafficPolicy(t *testing.T t.Errorf("service internalTrafficPolicy should be droppped but is set: %v", service.Spec.InternalTrafficPolicy) } } + +// Test_RemovingExternalIPsFromClusterIPServiceDropsExternalTrafficPolicy tests that removing externalIPs from a +// ClusterIP Service results in the externalTrafficPolicy field being dropped. +func Test_RemovingExternalIPsFromClusterIPServiceDropsExternalTrafficPolicy(t *testing.T) { + server := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd()) + defer server.TearDownFn() + + client, err := clientset.NewForConfig(server.ClientConfig) + if err != nil { + t.Fatalf("Error creating clientset: %v", err) + } + + ns := framework.CreateNamespaceOrDie(client, "test-removing-external-ips-drops-external-traffic-policy", t) + defer framework.DeleteNamespaceOrDie(client, ns, t) + + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-123", + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeClusterIP, + Ports: []corev1.ServicePort{{ + Port: int32(80), + }}, + Selector: map[string]string{ + "foo": "bar", + }, + ExternalIPs: []string{"1.1.1.1"}, + }, + } + + service, err = client.CoreV1().Services(ns.Name).Create(context.TODO(), service, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Error creating test service: %v", err) + } + + if service.Spec.ExternalTrafficPolicy != corev1.ServiceExternalTrafficPolicyCluster { + t.Error("service externalTrafficPolicy was not set for clusterIP Service with externalIPs") + } + + // externalTrafficPolicy should be dropped after removing externalIPs. + newService := service.DeepCopy() + newService.Spec.ExternalIPs = []string{} + + service, err = client.CoreV1().Services(ns.Name).Update(context.TODO(), newService, metav1.UpdateOptions{}) + if err != nil { + t.Fatalf("error updating service: %v", err) + } + + if service.Spec.ExternalTrafficPolicy != "" { + t.Errorf("service externalTrafficPolicy should be droppped but is set: %v", service.Spec.ExternalTrafficPolicy) + } + + service, err = client.CoreV1().Services(ns.Name).Get(context.TODO(), service.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting service: %v", err) + } + + if service.Spec.ExternalTrafficPolicy != "" { + t.Errorf("service externalTrafficPolicy should be droppped but is set: %v", service.Spec.ExternalTrafficPolicy) + } + + // externalTrafficPolicy should be set after adding externalIPs again. + newService = service.DeepCopy() + newService.Spec.ExternalIPs = []string{"1.1.1.1"} + + service, err = client.CoreV1().Services(ns.Name).Update(context.TODO(), newService, metav1.UpdateOptions{}) + if err != nil { + t.Fatalf("error updating service: %v", err) + } + + if service.Spec.ExternalTrafficPolicy != corev1.ServiceExternalTrafficPolicyCluster { + t.Error("service externalTrafficPolicy was not set for clusterIP Service with externalIPs") + } + + service, err = client.CoreV1().Services(ns.Name).Get(context.TODO(), service.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("error getting service: %v", err) + } + + if service.Spec.ExternalTrafficPolicy != corev1.ServiceExternalTrafficPolicyCluster { + t.Error("service externalTrafficPolicy was not set for clusterIP Service with externalIPs") + } +}