diff --git a/pkg/api/service/util.go b/pkg/api/service/util.go index 080b11ee469..50041f77e2e 100644 --- a/pkg/api/service/util.go +++ b/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 diff --git a/pkg/api/service/util_test.go b/pkg/api/service/util_test.go index 649ab000e8c..82ae6a813fa 100644 --- a/pkg/api/service/util_test.go +++ b/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 diff --git a/pkg/api/testing/fuzzer.go b/pkg/api/testing/fuzzer.go index 5cfe3d1c764..185b6f906bc 100644 --- a/pkg/api/testing/fuzzer.go +++ b/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 diff --git a/pkg/api/v1/defaults.go b/pkg/api/v1/defaults.go index 701031cc140..5472223cd72 100644 --- a/pkg/api/v1/defaults.go +++ b/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 diff --git a/pkg/api/v1/defaults_test.go b/pkg/api/v1/defaults_test.go index 20ef7ea3b57..c63bfda9bb2 100644 --- a/pkg/api/v1/defaults_test.go +++ b/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)) diff --git a/pkg/api/v1/service/util.go b/pkg/api/v1/service/util.go index 2e0dad146d2..fbb7cb8144d 100644 --- a/pkg/api/v1/service/util.go +++ b/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 diff --git a/pkg/api/v1/service/util_test.go b/pkg/api/v1/service/util_test.go index a01495f29bc..ade8bc81d28 100644 --- a/pkg/api/v1/service/util_test.go +++ b/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 diff --git a/pkg/api/v1/zz_generated.defaults.go b/pkg/api/v1/zz_generated.defaults.go index 121b39185c1..31e79506692 100644 --- a/pkg/api/v1/zz_generated.defaults.go +++ b/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) { diff --git a/pkg/registry/core/service/rest.go b/pkg/registry/core/service/rest.go index 28e28bee26c..eadf3050319 100644 --- a/pkg/registry/core/service/rest.go +++ b/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 diff --git a/pkg/registry/core/service/rest_test.go b/pkg/registry/core/service/rest_test.go index f3f3ff00549..af54feb4a26 100644 --- a/pkg/registry/core/service/rest_test.go +++ b/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() diff --git a/staging/src/k8s.io/client-go/pkg/api/v1/defaults.go b/staging/src/k8s.io/client-go/pkg/api/v1/defaults.go index 8324ebd50ab..47f610df18f 100644 --- a/staging/src/k8s.io/client-go/pkg/api/v1/defaults.go +++ b/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 diff --git a/staging/src/k8s.io/client-go/pkg/api/v1/zz_generated.defaults.go b/staging/src/k8s.io/client-go/pkg/api/v1/zz_generated.defaults.go index 121b39185c1..31e79506692 100644 --- a/staging/src/k8s.io/client-go/pkg/api/v1/zz_generated.defaults.go +++ b/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) {