github/kubernetes
4
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Allow specifying ExternalTrafficPolicy for ClusterIP Services with ExternalIPs

Browse Source 
When defining a ClusterIP Service, we can specify externalIP, and the
traffic policy of externalIP is subject to externalTrafficPolicy.
However, the policy can't be set when type is not NodePort or
LoadBalancer, and will default to Cluster when kube-proxy processes the
Service.

This commit updates the defaulting and validation of Service to allow
specifying ExternalTrafficPolicy for ClusterIP Services with
ExternalIPs.

Signed-off-by: Quan Tian <qtian@vmware.com>
This commit is contained in:
Quan Tian
2023-07-07 11:51:55 +08:00
parent 3cf3702d1e
commit 2b69daa960
14 changed files with 282 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
cmd/cloud-controller-manager/.import-restrictions
View File

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

7
pkg/api/service/util.go
View File

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

43
pkg/api/service/util_test.go
View File

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

12
pkg/api/v1/service/util.go
View File

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

63
pkg/api/v1/service/util_test.go
View File

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

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

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

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

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

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

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

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

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

1
pkg/kubectl/.import-restrictions
View File

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

6
pkg/registry/core/service/strategy.go
View File

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

22
pkg/registry/core/service/strategy_test.go
View File

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

1
staging/publishing/import-restrictions.yaml
View File

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

84
test/integration/service/service_test.go
View File

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