Merge pull request #119150 from tnqn/external-traffic-policy-external-ips

Allow specifying ExternalTrafficPolicy for Services with ExternalIPs

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

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 &&

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)

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 &&
+	if !ExternallyAccessible(service) {
-		service.Spec.Type != v1.ServiceTypeNodePort {
 		return false
 	}
 	return service.Spec.ExternalTrafficPolicy == v1.ServiceExternalTrafficPolicyLocal

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,

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 ||
+	if service.ExternallyAccessible(obj) && obj.Spec.ExternalTrafficPolicy == "" {
-		obj.Spec.Type == v1.ServiceTypeLoadBalancer) &&
-		obj.Spec.ExternalTrafficPolicy == "" {
 		obj.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyCluster
 	}
 

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) {

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 == "" {

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,
 	},
 	}
 

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

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
 }

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)
+			}
 		})
 	}
 }

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

test/integration/service/service_test.go

@@ -29,7 +29,7 @@ import (
 
 // Test_ExternalNameServiceStopsDefaultingInternalTrafficPolicy tests that Services no longer default
 // the internalTrafficPolicy field when Type is ExternalName. This test exists due to historic reasons where
-// the internalTrafficPolicy field was being defaulted in older versions. New versions stop defauting the
+// the internalTrafficPolicy field was being defaulted in older versions. New versions stop defaulting the
 // field and drop on read, but for compatibility reasons we still accept the field.
 func Test_ExternalNameServiceStopsDefaultingInternalTrafficPolicy(t *testing.T) {
 	server := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd())
@@ -74,7 +74,7 @@ func Test_ExternalNameServiceStopsDefaultingInternalTrafficPolicy(t *testing.T)
 
 // Test_ExternalNameServiceDropsInternalTrafficPolicy tests that Services accepts the internalTrafficPolicy field on Create,
 // but drops the field on read. This test exists due to historic reasons where the internalTrafficPolicy field was being defaulted
-// in older versions. New versions stop defauting the field and drop on read, but for compatibility reasons we still accept the field.
+// in older versions. New versions stop defaulting the field and drop on read, but for compatibility reasons we still accept the field.
 func Test_ExternalNameServiceDropsInternalTrafficPolicy(t *testing.T) {
 	server := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd())
 	defer server.TearDownFn()
@@ -120,7 +120,7 @@ func Test_ExternalNameServiceDropsInternalTrafficPolicy(t *testing.T) {
 
 // Test_ConvertingToExternalNameServiceDropsInternalTrafficPolicy tests that converting a Service to Type=ExternalName
 // results in the internalTrafficPolicy field being dropped.This test exists due to historic reasons where the internalTrafficPolicy
-// field was being defaulted in older versions. New versions stop defauting the field and drop on read, but for compatibility reasons
+// field was being defaulted in older versions. New versions stop defaulting the field and drop on read, but for compatibility reasons
 // we still accept the field.
 func Test_ConvertingToExternalNameServiceDropsInternalTrafficPolicy(t *testing.T) {
 	server := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd())
@@ -164,7 +164,7 @@ func Test_ConvertingToExternalNameServiceDropsInternalTrafficPolicy(t *testing.T
 
 	service, err = client.CoreV1().Services(ns.Name).Update(context.TODO(), newService, metav1.UpdateOptions{})
 	if err != nil {
-		t.Fatalf("error getting service: %v", err)
+		t.Fatalf("error updating service: %v", err)
 	}
 
 	if service.Spec.InternalTrafficPolicy != nil {
@@ -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")
+	}
+}