diff --git a/pkg/api/service/testing/make.go b/pkg/api/service/testing/make.go index 4767448a67a..de4145c17da 100644 --- a/pkg/api/service/testing/make.go +++ b/pkg/api/service/testing/make.go @@ -48,9 +48,6 @@ func MakeService(name string, tweaks ...Tweak) *api.Service { SetTypeClusterIP(svc) // Default to 1 port SetPorts(MakeServicePort("", 93, intstr.FromInt(76), api.ProtocolTCP))(svc) - // Default internalTrafficPolicy to "Cluster". This probably should not - // apply to ExternalName, but it went into beta and is not worth breaking. - SetInternalTrafficPolicy(api.ServiceInternalTrafficPolicyCluster)(svc) for _, tweak := range tweaks { tweak(svc) @@ -68,6 +65,8 @@ func SetTypeClusterIP(svc *api.Service) { svc.Spec.ExternalName = "" svc.Spec.ExternalTrafficPolicy = "" svc.Spec.AllocateLoadBalancerNodePorts = nil + internalTrafficPolicy := api.ServiceInternalTrafficPolicyCluster + svc.Spec.InternalTrafficPolicy = &internalTrafficPolicy } // SetTypeNodePort sets the service type to NodePort and clears other fields. @@ -76,6 +75,8 @@ func SetTypeNodePort(svc *api.Service) { svc.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeCluster svc.Spec.ExternalName = "" svc.Spec.AllocateLoadBalancerNodePorts = nil + internalTrafficPolicy := api.ServiceInternalTrafficPolicyCluster + svc.Spec.InternalTrafficPolicy = &internalTrafficPolicy } // SetTypeLoadBalancer sets the service type to LoadBalancer and clears other fields. @@ -84,6 +85,8 @@ func SetTypeLoadBalancer(svc *api.Service) { svc.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyTypeCluster svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(true) svc.Spec.ExternalName = "" + internalTrafficPolicy := api.ServiceInternalTrafficPolicyCluster + svc.Spec.InternalTrafficPolicy = &internalTrafficPolicy } // SetTypeExternalName sets the service type to ExternalName and clears other fields. @@ -94,6 +97,7 @@ func SetTypeExternalName(svc *api.Service) { svc.Spec.ClusterIP = "" svc.Spec.ClusterIPs = nil svc.Spec.AllocateLoadBalancerNodePorts = nil + svc.Spec.InternalTrafficPolicy = nil } // SetPorts sets the service ports list. diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index 3328790fd65..dcf5ea17dd1 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -131,9 +131,14 @@ func SetDefaults_Service(obj *v1.Service) { obj.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeCluster } - if utilfeature.DefaultFeatureGate.Enabled(features.ServiceInternalTrafficPolicy) && obj.Spec.InternalTrafficPolicy == nil { - serviceInternalTrafficPolicyCluster := v1.ServiceInternalTrafficPolicyCluster - obj.Spec.InternalTrafficPolicy = &serviceInternalTrafficPolicyCluster + if utilfeature.DefaultFeatureGate.Enabled(features.ServiceInternalTrafficPolicy) { + if obj.Spec.InternalTrafficPolicy == nil { + if obj.Spec.Type == v1.ServiceTypeNodePort || obj.Spec.Type == v1.ServiceTypeLoadBalancer || obj.Spec.Type == v1.ServiceTypeClusterIP { + serviceInternalTrafficPolicyCluster := v1.ServiceInternalTrafficPolicyCluster + obj.Spec.InternalTrafficPolicy = &serviceInternalTrafficPolicyCluster + } + } + } if obj.Spec.Type == v1.ServiceTypeLoadBalancer { diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index 750ded75bf7..5b30be72986 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/pkg/apis/core/v1/defaults_test.go @@ -1840,19 +1840,19 @@ func TestSetDefaultServiceInternalTrafficPolicy(t *testing.T) { local := v1.ServiceInternalTrafficPolicyLocal testCases := []struct { name string - expectedInternalTrafficPolicy v1.ServiceInternalTrafficPolicyType + expectedInternalTrafficPolicy *v1.ServiceInternalTrafficPolicyType svc v1.Service featureGateOn bool }{ { name: "must set default internalTrafficPolicy", - expectedInternalTrafficPolicy: v1.ServiceInternalTrafficPolicyCluster, + expectedInternalTrafficPolicy: &cluster, svc: v1.Service{}, featureGateOn: true, }, { name: "must not set default internalTrafficPolicy when it's cluster", - expectedInternalTrafficPolicy: v1.ServiceInternalTrafficPolicyCluster, + expectedInternalTrafficPolicy: &cluster, svc: v1.Service{ Spec: v1.ServiceSpec{ InternalTrafficPolicy: &cluster, @@ -1860,9 +1860,19 @@ func TestSetDefaultServiceInternalTrafficPolicy(t *testing.T) { }, featureGateOn: true, }, + { + name: "must not set default internalTrafficPolicy when type is ExternalName", + expectedInternalTrafficPolicy: nil, + svc: v1.Service{ + Spec: v1.ServiceSpec{ + Type: v1.ServiceTypeExternalName, + }, + }, + featureGateOn: true, + }, { name: "must not set default internalTrafficPolicy when it's local", - expectedInternalTrafficPolicy: v1.ServiceInternalTrafficPolicyLocal, + expectedInternalTrafficPolicy: &local, svc: v1.Service{ Spec: v1.ServiceSpec{ InternalTrafficPolicy: &local, @@ -1872,7 +1882,7 @@ func TestSetDefaultServiceInternalTrafficPolicy(t *testing.T) { }, { name: "must not set default internalTrafficPolicy when gate is disabled", - expectedInternalTrafficPolicy: "", + expectedInternalTrafficPolicy: nil, svc: v1.Service{}, featureGateOn: false, }, @@ -1883,14 +1893,8 @@ func TestSetDefaultServiceInternalTrafficPolicy(t *testing.T) { obj := roundTrip(t, runtime.Object(&test.svc)) svc := obj.(*v1.Service) - if test.expectedInternalTrafficPolicy == "" { - if svc.Spec.InternalTrafficPolicy != nil { - t.Fatalf("expected .spec.internalTrafficPolicy: null, got %v", *svc.Spec.InternalTrafficPolicy) - } - } else { - if *svc.Spec.InternalTrafficPolicy != test.expectedInternalTrafficPolicy { - t.Fatalf("expected .spec.internalTrafficPolicy: %v got %v", test.expectedInternalTrafficPolicy, *svc.Spec.InternalTrafficPolicy) - } + if !reflect.DeepEqual(svc.Spec.InternalTrafficPolicy, test.expectedInternalTrafficPolicy) { + t.Errorf("expected .spec.internalTrafficPolicy: %v got %v", test.expectedInternalTrafficPolicy, svc.Spec.InternalTrafficPolicy) } }) } diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index c5ab24e8f31..62c42638929 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -4809,7 +4809,12 @@ func validateServiceInternalTrafficFieldsValue(service *core.Service) field.Erro if utilfeature.DefaultFeatureGate.Enabled(features.ServiceInternalTrafficPolicy) { if service.Spec.InternalTrafficPolicy == nil { - allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("internalTrafficPolicy"), "")) + // We do not forbid internalTrafficPolicy on other Service types because of historical reasons. + // We did not check that before it went beta and we don't want to invalidate existing stored objects. + if service.Spec.Type == core.ServiceTypeNodePort || + service.Spec.Type == core.ServiceTypeLoadBalancer || service.Spec.Type == core.ServiceTypeClusterIP { + allErrs = append(allErrs, field.Required(field.NewPath("spec").Child("internalTrafficPolicy"), "")) + } } } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 9b81c9b3152..695d38f1dc2 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -12281,6 +12281,31 @@ func TestValidateServiceCreate(t *testing.T) { featureGates: []featuregate.Feature{features.ServiceInternalTrafficPolicy}, numErrs: 1, }, + { + name: "internalTrafficPolicy field nil when type is ExternalName", + tweakSvc: func(s *core.Service) { + s.Spec.InternalTrafficPolicy = nil + s.Spec.Type = core.ServiceTypeExternalName + s.Spec.ExternalName = "foo.bar.com" + }, + featureGates: []featuregate.Feature{features.ServiceInternalTrafficPolicy}, + numErrs: 0, + }, + { + // Typically this should fail validation, but in v1.22 we have existing clusters + // that may have allowed internalTrafficPolicy when Type=ExternalName. + // This test case ensures we don't break compatibility for internalTrafficPolicy + // when Type=ExternalName + name: "internalTrafficPolicy field is set when type is ExternalName", + tweakSvc: func(s *core.Service) { + cluster := core.ServiceInternalTrafficPolicyCluster + s.Spec.InternalTrafficPolicy = &cluster + s.Spec.Type = core.ServiceTypeExternalName + s.Spec.ExternalName = "foo.bar.com" + }, + featureGates: []featuregate.Feature{features.ServiceInternalTrafficPolicy}, + numErrs: 0, + }, { name: "invalid internalTraffic field", tweakSvc: func(s *core.Service) { diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go index 363cdc8f1ea..d9d77ae6cf4 100644 --- a/pkg/registry/core/service/storage/storage.go +++ b/pkg/registry/core/service/storage/storage.go @@ -242,6 +242,17 @@ func (r *REST) defaultOnReadService(service *api.Service) { // Set ipFamilies and ipFamilyPolicy if needed. r.defaultOnReadIPFamilies(service) + + // We unintentionally defaulted internalTrafficPolicy when it's not needed + // for the ExternalName type. It's too late to change the field in storage, + // but we can drop the field when read. + defaultOnReadInternalTrafficPolicy(service) +} + +func defaultOnReadInternalTrafficPolicy(service *api.Service) { + if service.Spec.Type == api.ServiceTypeExternalName { + service.Spec.InternalTrafficPolicy = nil + } } func (r *REST) defaultOnReadIPFamilies(service *api.Service) { diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index 444b5be00d2..b522d07a825 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/pkg/registry/core/service/storage/storage_test.go @@ -598,7 +598,7 @@ func TestServiceDefaultOnRead(t *testing.T) { svctest.SetIPFamilies(api.IPv4Protocol)), }, { name: "external name", - input: makeServiceList(svctest.SetTypeExternalName), + input: makeServiceList(svctest.SetTypeExternalName, svctest.SetInternalTrafficPolicy(api.ServiceInternalTrafficPolicyCluster)), expect: makeServiceList(svctest.SetTypeExternalName), }, { name: "dual v4v6", @@ -679,6 +679,9 @@ func TestServiceDefaultOnRead(t *testing.T) { if want, got := exp.Spec.IPFamilies, svc.Spec.IPFamilies; !reflect.DeepEqual(want, got) { t.Errorf("ipFamilies: expected %v, got %v", want, got) } + if want, got := fmtInternalTrafficPolicy(exp.Spec.InternalTrafficPolicy), fmtInternalTrafficPolicy(svc.Spec.InternalTrafficPolicy); want != got { + t.Errorf("internalTrafficPolicy: expected %v, got %v", want, got) + } }) } } @@ -1243,6 +1246,13 @@ func fmtIPFamilyPolicy(pol *api.IPFamilyPolicyType) string { return string(*pol) } +func fmtInternalTrafficPolicy(pol *api.ServiceInternalTrafficPolicyType) string { + if pol == nil { + return "" + } + return string(*pol) +} + func fmtIPFamilies(fams []api.IPFamily) string { if fams == nil { return "[]" @@ -11378,31 +11388,16 @@ func TestFeatureInternalTrafficPolicy(t *testing.T) { } testCases := []cudTestCase{{ - name: "ExternalName_policy:none-ExternalName_policy:Local", + name: "ExternalName_policy:none-ExternalName_policy:none", create: svcTestCase{ svc: svctest.MakeService("foo", svctest.SetTypeExternalName), - prove: prove(proveITP(api.ServiceInternalTrafficPolicyCluster)), + prove: prove(proveITP("")), }, update: svcTestCase{ svc: svctest.MakeService("foo", - svctest.SetTypeExternalName, - svctest.SetInternalTrafficPolicy(api.ServiceInternalTrafficPolicyLocal)), - prove: prove(proveITP(api.ServiceInternalTrafficPolicyLocal)), - }, - }, { - name: "ExternalName_policy:Cluster-ExternalName_policy:Local", - create: svcTestCase{ - svc: svctest.MakeService("foo", - svctest.SetTypeExternalName, - svctest.SetInternalTrafficPolicy(api.ServiceInternalTrafficPolicyCluster)), - prove: prove(proveITP(api.ServiceInternalTrafficPolicyCluster)), - }, - update: svcTestCase{ - svc: svctest.MakeService("foo", - svctest.SetTypeExternalName, - svctest.SetInternalTrafficPolicy(api.ServiceInternalTrafficPolicyLocal)), - prove: prove(proveITP(api.ServiceInternalTrafficPolicyLocal)), + svctest.SetTypeExternalName), + prove: prove(proveITP("")), }, }, { name: "ClusterIP_policy:none-ClusterIP_policy:Local",