Skip to content

Commit

Permalink
feat(NSC): change service.local internal traffic policy posture
Browse files Browse the repository at this point in the history 
Over time, feedback from users has been that our interpretation of how
the kube-router service.local annotation interacts with the internal
traffic policy has been that it is too restrictive.

It seems like tuning it to fall in line with the local internal traffic
policy is too restrictive. This commit changes that posture, by equating
the service.local annotation with External Traffic Policy Local and
Internal Traffic Policy Cluster.

This means that when service.local is set the following will be true:

* ExternalIPs / LoadBalancer IPs will only be available on a node that
  hosts the workload
* ExternalIPs / LoadBalancer IPs will only be BGP advertised (when
  enabled) by nodes that host the workload
* Services will have the same posture as External Traffic Policy set to
  local
* ClusterIPs will be available on all nodes for LoadBalancing
* ClusterIPs will only be BGP advertised (when enabled) by nodes that
  host the workload
* Cluster IP services will have the same posture as Internal Traffic
  Policy set to cluster

For anyone desiring the original functionality of the service.local
annotation that has been in place since kube-router v2.1.0, all that
would need to be done is to set `internalTrafficPolicy` to Local as
described here: https://kubernetes.io/docs/concepts/services-networking/service-traffic-policy/
  • Loading branch information
@aauren @mrueg
aauren authored and mrueg committed Dec 4, 2024
1 parent 1662973 commit 44439d6
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 22 deletions.
7 changes: 4 additions & 3 deletions pkg/controllers/proxy/network_services_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -978,10 +978,11 @@ func (nsc *NetworkServicesController) buildServicesInfo() serviceInfoMap {
svcInfo.extTrafficPolicy = &svc.Spec.ExternalTrafficPolicy

// The kube-router.io/service.local annotation has the ability to override the internal and external traffic
// policy that is set in the spec. In this case we set both to local when the annotation is true so that
// previous functionality of the annotation is best preserved.
// policy that is set in the spec. Previously, when this was active set both to local when the annotation is
// true so that previous functionality of the annotation is best preserved. However, this has proved to not
// be a good fit for ClusterIP traffic, so we retain cluster for internal traffic policy.
if svc.ObjectMeta.Annotations[svcLocalAnnotation] == "true" {
intTrafficPolicyLocal := v1.ServiceInternalTrafficPolicyLocal
intTrafficPolicyLocal := v1.ServiceInternalTrafficPolicyCluster
extTrafficPolicyLocal := v1.ServiceExternalTrafficPolicyLocal
svcInfo.intTrafficPolicy = &intTrafficPolicyLocal
svcInfo.extTrafficPolicy = &extTrafficPolicyLocal
Expand Down
5 changes: 2 additions & 3 deletions pkg/controllers/routing/ecmp_vip.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ func (nrc *NetworkRoutingController) shouldAdvertiseService(svc *v1core.Service,

// If:
// - We are assessing the clusterIP of the service (the internally facing VIP)
// - The service has an internal traffic policy of "local" or the service has the service.local annotation on it
// - The service has an internal traffic policy of "local"
// - The service doesn't have any endpoints on the node we're executing on
// Then: return false
// We handle spec.internalTrafficPolicy different because it was introduced in v1.26 and may not be available in all
Expand All @@ -493,8 +493,7 @@ func (nrc *NetworkRoutingController) shouldAdvertiseService(svc *v1core.Service,
if svc.Spec.InternalTrafficPolicy != nil {
serIntTrafPol = *svc.Spec.InternalTrafficPolicy == v1core.ServiceInternalTrafficPolicyLocal
}
intLocalPol := (serIntTrafPol || svc.Annotations[svcLocalAnnotation] == "true")
if isClusterIP && intLocalPol && !hasLocalEndpoints {
if isClusterIP && serIntTrafPol && !hasLocalEndpoints {
return false, nil
}

Expand Down
32 changes: 16 additions & 16 deletions pkg/controllers/routing/ecmp_vip_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -620,35 +620,35 @@ func Test_getVIPsForService(t *testing.T) {
{
service: getClusterSvc(),
endpoints: getNoLocalAddressesEPs(),
advertisedIPs: []string{},
withdrawnIPs: []string{"10.0.0.1"},
advertisedIPs: []string{"10.0.0.1"},
withdrawnIPs: []string{},
annotations: map[string]string{
svcLocalAnnotation: "true",
},
},
{
service: getExternalSvc(),
endpoints: getNoLocalAddressesEPs(),
advertisedIPs: []string{},
withdrawnIPs: []string{"10.0.0.1", "1.1.1.1"},
advertisedIPs: []string{"10.0.0.1"},
withdrawnIPs: []string{"1.1.1.1"},
annotations: map[string]string{
svcLocalAnnotation: "true",
},
},
{
service: getNodePortSvc(),
endpoints: getNoLocalAddressesEPs(),
advertisedIPs: []string{},
withdrawnIPs: []string{"10.0.0.1", "1.1.1.1"},
advertisedIPs: []string{"10.0.0.1"},
withdrawnIPs: []string{"1.1.1.1"},
annotations: map[string]string{
svcLocalAnnotation: "true",
},
},
{
service: getLoadBalancerSvc(),
endpoints: getNoLocalAddressesEPs(),
advertisedIPs: []string{},
withdrawnIPs: []string{"10.0.0.1", "1.1.1.1", "10.0.255.1", "10.0.255.2"},
advertisedIPs: []string{"10.0.0.1"},
withdrawnIPs: []string{"1.1.1.1", "10.0.255.1", "10.0.255.2"},
annotations: map[string]string{
svcLocalAnnotation: "true",
},
Expand All @@ -672,35 +672,35 @@ func Test_getVIPsForService(t *testing.T) {
{
service: getClusterSvc(),
endpoints: getNoLocalAddressesEPs(),
advertisedIPs: []string{},
withdrawnIPs: []string{"10.0.0.1"},
advertisedIPs: []string{"10.0.0.1"},
withdrawnIPs: []string{},
annotations: map[string]string{
svcLocalAnnotation: "true",
},
},
{
service: getExternalSvc(),
endpoints: getNoLocalAddressesEPs(),
advertisedIPs: []string{},
withdrawnIPs: []string{"10.0.0.1", "1.1.1.1"},
advertisedIPs: []string{"10.0.0.1"},
withdrawnIPs: []string{"1.1.1.1"},
annotations: map[string]string{
svcLocalAnnotation: "true",
},
},
{
service: getNodePortSvc(),
endpoints: getNoLocalAddressesEPs(),
advertisedIPs: []string{},
withdrawnIPs: []string{"10.0.0.1", "1.1.1.1"},
advertisedIPs: []string{"10.0.0.1"},
withdrawnIPs: []string{"1.1.1.1"},
annotations: map[string]string{
svcLocalAnnotation: "true",
},
},
{
service: getLoadBalancerSvc(),
endpoints: getNoLocalAddressesEPs(),
advertisedIPs: []string{},
withdrawnIPs: []string{"10.0.0.1", "1.1.1.1", "10.0.255.1", "10.0.255.2"},
advertisedIPs: []string{"10.0.0.1"},
withdrawnIPs: []string{"1.1.1.1", "10.0.255.1", "10.0.255.2"},
annotations: map[string]string{
svcLocalAnnotation: "true",
},
Expand Down

0 comments on commit 44439d6

Please sign in to comment.