github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-07-30 23:15:14 +00:00
Code Issues Projects Releases Wiki Activity

Merge pull request #104846 from andrewsykim/fix-internal-traffic-policy-validation

Browse Source 
Stop defaulting Service internalTrafficPolicy when type is ExternalName
This commit is contained in:
Kubernetes Prow Robot 2022-01-04 17:40:32 -08:00 committed by GitHub
commit 1945829906
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 89 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
pkg/api/service/testing/make.go
View File

@ -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.

11
pkg/apis/core/v1/defaults.go
View File

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

30
pkg/apis/core/v1/defaults_test.go
View File

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

7
pkg/apis/core/validation/validation.go
View File

@ -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"), ""))
}
}
}

25
pkg/apis/core/validation/validation_test.go
View File

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

11
pkg/registry/core/service/storage/storage.go
View File

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

35
pkg/registry/core/service/storage/storage_test.go
View File

@ -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 "<nil>"
}
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",