Merge pull request #45869 from MrHohn/esipp-default-move

Automatic merge from submit-queue

Move defaulting logic for ExternalTrafficPolicy into defaults.go

Link #45812.

Per [#45812/comment](https://github.com/kubernetes/kubernetes/issues/45812#issuecomment-301632807), move defaulting logic to defaults.go so that federation will get the same code.

This PR does not have any functional change :)

/assign @freehan @thockin 
/cc @madhusudancs @nikhiljindal @shashidharatd
Also /cc @caesarxuchao per the client-go changes.

**Release note**:

```release-note
NONE
```

pkg/api/service/util.go

@@ -118,20 +118,6 @@ func GetServiceHealthCheckNodePort(service *api.Service) int32 {
 	return service.Spec.HealthCheckNodePort
 }
 
-// SetDefaultExternalTrafficPolicyIfNeeded defaults the ExternalTrafficPolicy field
-// for NodePort / LoadBalancer service to Global for consistency.
-// TODO: Move this default logic to default.go once beta annotation is deprecated.
-func SetDefaultExternalTrafficPolicyIfNeeded(service *api.Service) {
-	if _, ok := service.Annotations[api.BetaAnnotationExternalTraffic]; ok {
-		// Don't default this field if beta annotation exists.
-		return
-	} else if (service.Spec.Type == api.ServiceTypeNodePort ||
-		service.Spec.Type == api.ServiceTypeLoadBalancer) &&
-		service.Spec.ExternalTrafficPolicy == "" {
-		service.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeGlobal
-	}
-}
-
 // ClearExternalTrafficPolicy resets the ExternalTrafficPolicy field.
 func ClearExternalTrafficPolicy(service *api.Service) {
 	// First check the beta annotation and then the first class field. This is so that

pkg/api/service/util_test.go

@@ -20,7 +20,6 @@ import (
 	"testing"
 
 	"fmt"
-	"reflect"
 	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -298,106 +297,6 @@ func TestGetServiceHealthCheckNodePort(t *testing.T) {
 	})
 }
 
-func TestSetDefaultExternalTrafficPolicyIfNeeded(t *testing.T) {
-	testCases := []struct {
-		inputService    *api.Service
-		expectedService *api.Service
-	}{
-		// First class fields cases.
-		{
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeLoadBalancer,
-				},
-			},
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeLoadBalancer,
-					ExternalTrafficPolicy: api.ServiceExternalTrafficPolicyTypeGlobal,
-				},
-			},
-		},
-		{
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeNodePort,
-				},
-			},
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeNodePort,
-					ExternalTrafficPolicy: api.ServiceExternalTrafficPolicyTypeGlobal,
-				},
-			},
-		},
-		{
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeClusterIP,
-				},
-			},
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeClusterIP,
-				},
-			},
-		},
-		// Beta annotations cases.
-		{
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeLoadBalancer,
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						api.BetaAnnotationExternalTraffic: api.AnnotationValueExternalTrafficLocal,
-					},
-				},
-			},
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeLoadBalancer,
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						api.BetaAnnotationExternalTraffic: api.AnnotationValueExternalTrafficLocal,
-					},
-				},
-			},
-		},
-		{
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeLoadBalancer,
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						api.BetaAnnotationExternalTraffic: api.AnnotationValueExternalTrafficGlobal,
-					},
-				},
-			},
-			&api.Service{
-				Spec: api.ServiceSpec{
-					Type: api.ServiceTypeLoadBalancer,
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						api.BetaAnnotationExternalTraffic: api.AnnotationValueExternalTrafficGlobal,
-					},
-				},
-			},
-		},
-	}
-
-	for i, tc := range testCases {
-		SetDefaultExternalTrafficPolicyIfNeeded(tc.inputService)
-		if !reflect.DeepEqual(tc.inputService, tc.expectedService) {
-			t.Errorf("%v: got unexpected service", i)
-			spew.Dump(tc)
-		}
-	}
-}
-
 func TestClearExternalTrafficPolicy(t *testing.T) {
 	testCases := []struct {
 		inputService *api.Service

pkg/api/testing/fuzzer.go

@@ -319,6 +319,10 @@ func coreFuncs(t apitesting.TestingCommon) []interface{} {
 			types := []api.ServiceType{api.ServiceTypeClusterIP, api.ServiceTypeNodePort, api.ServiceTypeLoadBalancer}
 			*p = types[c.Rand.Intn(len(types))]
 		},
+		func(p *api.ServiceExternalTrafficPolicyType, c fuzz.Continue) {
+			types := []api.ServiceExternalTrafficPolicyType{api.ServiceExternalTrafficPolicyTypeGlobal, api.ServiceExternalTrafficPolicyTypeLocal}
+			*p = types[c.Rand.Intn(len(types))]
+		},
 		func(ct *api.Container, c fuzz.Continue) {
 			c.FuzzNoCustom(ct)                                          // fuzz self without calling this function again
 			ct.TerminationMessagePath = "/" + ct.TerminationMessagePath // Must be non-empty

pkg/api/v1/defaults.go

@@ -96,15 +96,15 @@ func SetDefaults_Container(obj *Container) {
 		obj.TerminationMessagePolicy = TerminationMessageReadFile
 	}
 }
-func SetDefaults_ServiceSpec(obj *ServiceSpec) {
-	if obj.SessionAffinity == "" {
-		obj.SessionAffinity = ServiceAffinityNone
+func SetDefaults_Service(obj *Service) {
+	if obj.Spec.SessionAffinity == "" {
+		obj.Spec.SessionAffinity = ServiceAffinityNone
 	}
-	if obj.Type == "" {
-		obj.Type = ServiceTypeClusterIP
+	if obj.Spec.Type == "" {
+		obj.Spec.Type = ServiceTypeClusterIP
 	}
-	for i := range obj.Ports {
-		sp := &obj.Ports[i]
+	for i := range obj.Spec.Ports {
+		sp := &obj.Spec.Ports[i]
 		if sp.Protocol == "" {
 			sp.Protocol = ProtocolTCP
 		}
@@ -112,6 +112,16 @@ func SetDefaults_ServiceSpec(obj *ServiceSpec) {
 			sp.TargetPort = intstr.FromInt(int(sp.Port))
 		}
 	}
+	// Defaults ExternalTrafficPolicy field for NodePort / LoadBalancer service
+	// to Global for consistency.
+	if _, ok := obj.Annotations[BetaAnnotationExternalTraffic]; ok {
+		// Don't default this field if beta annotation exists.
+		return
+	} else if (obj.Spec.Type == ServiceTypeNodePort ||
+		obj.Spec.Type == ServiceTypeLoadBalancer) &&
+		obj.Spec.ExternalTrafficPolicy == "" {
+		obj.Spec.ExternalTrafficPolicy = ServiceExternalTrafficPolicyTypeGlobal
+	}
 }
 func SetDefaults_Pod(obj *Pod) {
 	// If limits are specified, but requests are not, default requests to limits

pkg/api/v1/defaults_test.go

@@ -874,6 +874,41 @@ func TestSetDefaultServicePort(t *testing.T) {
 	}
 }
 
+func TestSetDefaulServiceExternalTraffic(t *testing.T) {
+	in := &v1.Service{}
+	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.ServiceTypeNodePort}}
+	obj = roundTrip(t, runtime.Object(in))
+	out = obj.(*v1.Service)
+	if out.Spec.ExternalTrafficPolicy != v1.ServiceExternalTrafficPolicyTypeGlobal {
+		t.Errorf("Expected ExternalTrafficPolicy to be %v, got %v", v1.ServiceExternalTrafficPolicyTypeGlobal, out.Spec.ExternalTrafficPolicy)
+	}
+
+	in = &v1.Service{Spec: v1.ServiceSpec{Type: v1.ServiceTypeLoadBalancer}}
+	obj = roundTrip(t, runtime.Object(in))
+	out = obj.(*v1.Service)
+	if out.Spec.ExternalTrafficPolicy != v1.ServiceExternalTrafficPolicyTypeGlobal {
+		t.Errorf("Expected ExternalTrafficPolicy to be %v, got %v", v1.ServiceExternalTrafficPolicyTypeGlobal, out.Spec.ExternalTrafficPolicy)
+	}
+
+	in = &v1.Service{
+		Spec: v1.ServiceSpec{Type: v1.ServiceTypeLoadBalancer},
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{v1.BetaAnnotationExternalTraffic: v1.AnnotationValueExternalTrafficLocal},
+		},
+	}
+	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) {
 	s := &v1.Namespace{}
 	obj2 := roundTrip(t, runtime.Object(s))

pkg/api/v1/service/util.go

@@ -118,20 +118,6 @@ func GetServiceHealthCheckNodePort(service *v1.Service) int32 {
 	return service.Spec.HealthCheckNodePort
 }
 
-// SetDefaultExternalTrafficPolicyIfNeeded defaults the ExternalTrafficPolicy field
-// for NodePort / LoadBalancer service to Global for consistency.
-// TODO: Move this default logic to default.go once beta annotation is deprecated.
-func SetDefaultExternalTrafficPolicyIfNeeded(service *v1.Service) {
-	if _, ok := service.Annotations[v1.BetaAnnotationExternalTraffic]; ok {
-		// Don't default this field if beta annotation exists.
-		return
-	} else if (service.Spec.Type == v1.ServiceTypeNodePort ||
-		service.Spec.Type == v1.ServiceTypeLoadBalancer) &&
-		service.Spec.ExternalTrafficPolicy == "" {
-		service.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeGlobal
-	}
-}
-
 // ClearExternalTrafficPolicy resets the ExternalTrafficPolicy field.
 func ClearExternalTrafficPolicy(service *v1.Service) {
 	// First check the beta annotation and then the first class field. This is so existing

pkg/api/v1/service/util_test.go

@@ -20,7 +20,6 @@ import (
 	"testing"
 
 	"fmt"
-	"reflect"
 	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -298,106 +297,6 @@ func TestGetServiceHealthCheckNodePort(t *testing.T) {
 	})
 }
 
-func TestSetDefaultExternalTrafficPolicyIfNeeded(t *testing.T) {
-	testCases := []struct {
-		inputService    *v1.Service
-		expectedService *v1.Service
-	}{
-		// First class fields cases.
-		{
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeLoadBalancer,
-				},
-			},
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeLoadBalancer,
-					ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyTypeGlobal,
-				},
-			},
-		},
-		{
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeNodePort,
-				},
-			},
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeNodePort,
-					ExternalTrafficPolicy: v1.ServiceExternalTrafficPolicyTypeGlobal,
-				},
-			},
-		},
-		{
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeClusterIP,
-				},
-			},
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeClusterIP,
-				},
-			},
-		},
-		// Beta annotations cases.
-		{
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeLoadBalancer,
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						v1.BetaAnnotationExternalTraffic: v1.AnnotationValueExternalTrafficLocal,
-					},
-				},
-			},
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeLoadBalancer,
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						v1.BetaAnnotationExternalTraffic: v1.AnnotationValueExternalTrafficLocal,
-					},
-				},
-			},
-		},
-		{
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeLoadBalancer,
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						v1.BetaAnnotationExternalTraffic: v1.AnnotationValueExternalTrafficGlobal,
-					},
-				},
-			},
-			&v1.Service{
-				Spec: v1.ServiceSpec{
-					Type: v1.ServiceTypeLoadBalancer,
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{
-						v1.BetaAnnotationExternalTraffic: v1.AnnotationValueExternalTrafficGlobal,
-					},
-				},
-			},
-		},
-	}
-
-	for i, tc := range testCases {
-		SetDefaultExternalTrafficPolicyIfNeeded(tc.inputService)
-		if !reflect.DeepEqual(tc.inputService, tc.expectedService) {
-			t.Errorf("%v: got unexpected service", i)
-			spew.Dump(tc)
-		}
-	}
-}
-
 func TestClearExternalTrafficPolicy(t *testing.T) {
 	testCases := []struct {
 		inputService *v1.Service

pkg/api/v1/zz_generated.defaults.go

@@ -620,7 +620,7 @@ func SetObjectDefaults_SecretList(in *SecretList) {
 }
 
 func SetObjectDefaults_Service(in *Service) {
-	SetDefaults_ServiceSpec(&in.Spec)
+	SetDefaults_Service(in)
 }
 
 func SetObjectDefaults_ServiceList(in *ServiceList) {

pkg/registry/core/service/rest.go

@@ -164,7 +164,6 @@ func (rs *REST) Create(ctx genericapirequest.Context, obj runtime.Object) (runti
 
 	// Handle ExternalTraiffc related fields during service creation.
 	if utilfeature.DefaultFeatureGate.Enabled(features.ExternalTrafficLocalOnly) {
-		apiservice.SetDefaultExternalTrafficPolicyIfNeeded(service)
 		if apiservice.NeedsHealthCheck(service) {
 			if err := rs.allocateHealthCheckNodePort(service); err != nil {
 				return nil, errors.NewInternalError(err)
@@ -414,7 +413,6 @@ func (rs *REST) Update(ctx genericapirequest.Context, name string, objInfo rest.
 
 	// Handle ExternalTraiffc related updates.
 	if utilfeature.DefaultFeatureGate.Enabled(features.ExternalTrafficLocalOnly) {
-		apiservice.SetDefaultExternalTrafficPolicyIfNeeded(service)
 		success, err := rs.healthCheckNodePortUpdate(oldService, service)
 		if !success || err != nil {
 			return nil, false, err

pkg/registry/core/service/rest_test.go

@@ -1235,40 +1235,6 @@ func TestServiceRegistryExternalTrafficGlobalBeta(t *testing.T) {
 	}
 }
 
-// Validate that ExternalTraffic is default to Global for loadBalancer service.
-func TestServiceRegistryExternalTrafficDefaultGlobal(t *testing.T) {
-	ctx := genericapirequest.NewDefaultContext()
-	storage, _ := NewTestREST(t, nil)
-	svc := &api.Service{
-		ObjectMeta: metav1.ObjectMeta{Name: "external-lb-esipp"},
-		Spec: api.ServiceSpec{
-			Selector:        map[string]string{"bar": "baz"},
-			SessionAffinity: api.ServiceAffinityNone,
-			Type:            api.ServiceTypeLoadBalancer,
-			Ports: []api.ServicePort{{
-				Port:       6502,
-				Protocol:   api.ProtocolTCP,
-				TargetPort: intstr.FromInt(6502),
-			}},
-		},
-	}
-	created_svc, err := storage.Create(ctx, svc)
-	if created_svc == nil || err != nil {
-		t.Errorf("Unexpected failure creating service %v", err)
-	}
-	created_service := created_svc.(*api.Service)
-	if service.NeedsHealthCheck(created_service) {
-		t.Errorf("Expecting health check not needed, returned health check needed instead")
-	}
-	// Make sure the service does not have the health check node port allocated
-	if port := service.GetServiceHealthCheckNodePort(created_service); port != 0 {
-		t.Errorf("Unexpected allocation of health check node port: %v", port)
-	}
-	if created_service.Spec.ExternalTrafficPolicy != api.ServiceExternalTrafficPolicyTypeGlobal {
-		t.Errorf("Expecting externalTraffic to be %v, got:%v", api.ServiceExternalTrafficPolicyTypeGlobal, created_service.Spec.ExternalTrafficPolicy)
-	}
-}
-
 // Validate that the health check nodePort is not allocated when service type is ClusterIP
 func TestServiceRegistryExternalTrafficAnnotationClusterIP(t *testing.T) {
 	ctx := genericapirequest.NewDefaultContext()

staging/src/k8s.io/client-go/pkg/api/v1/defaults.go

@@ -96,15 +96,15 @@ func SetDefaults_Container(obj *Container) {
 		obj.TerminationMessagePolicy = TerminationMessageReadFile
 	}
 }
-func SetDefaults_ServiceSpec(obj *ServiceSpec) {
-	if obj.SessionAffinity == "" {
-		obj.SessionAffinity = ServiceAffinityNone
+func SetDefaults_Service(obj *Service) {
+	if obj.Spec.SessionAffinity == "" {
+		obj.Spec.SessionAffinity = ServiceAffinityNone
 	}
-	if obj.Type == "" {
-		obj.Type = ServiceTypeClusterIP
+	if obj.Spec.Type == "" {
+		obj.Spec.Type = ServiceTypeClusterIP
 	}
-	for i := range obj.Ports {
-		sp := &obj.Ports[i]
+	for i := range obj.Spec.Ports {
+		sp := &obj.Spec.Ports[i]
 		if sp.Protocol == "" {
 			sp.Protocol = ProtocolTCP
 		}
@@ -112,6 +112,16 @@ func SetDefaults_ServiceSpec(obj *ServiceSpec) {
 			sp.TargetPort = intstr.FromInt(int(sp.Port))
 		}
 	}
+	// Defaults ExternalTrafficPolicy field for NodePort / LoadBalancer service
+	// to Global for consistency.
+	if _, ok := obj.Annotations[BetaAnnotationExternalTraffic]; ok {
+		// Don't default this field if beta annotation exists.
+		return
+	} else if (obj.Spec.Type == ServiceTypeNodePort ||
+		obj.Spec.Type == ServiceTypeLoadBalancer) &&
+		obj.Spec.ExternalTrafficPolicy == "" {
+		obj.Spec.ExternalTrafficPolicy = ServiceExternalTrafficPolicyTypeGlobal
+	}
 }
 func SetDefaults_Pod(obj *Pod) {
 	// If limits are specified, but requests are not, default requests to limits

staging/src/k8s.io/client-go/pkg/api/v1/zz_generated.defaults.go

@@ -620,7 +620,7 @@ func SetObjectDefaults_SecretList(in *SecretList) {
 }
 
 func SetObjectDefaults_Service(in *Service) {
-	SetDefaults_ServiceSpec(&in.Spec)
+	SetDefaults_Service(in)
 }
 
 func SetObjectDefaults_ServiceList(in *ServiceList) {