Repository: kumahq/kuma
Pull request #1863 (2021-04-21 12:48:24)

fix(kuma-cp) always set locality on endpoint in multizone
### The problem

Kuma has a locality-aware loadbalancing feature that uses priority under the hood. We could even not set zone on endpoints and just use priority but we do this anyways. The only profit right now from setting this is the visibility of the zone in `/clusters`.

Right now there is a problem that if you have `localityAwareLoadbalancing` on the mesh to `false` and then switch it to `true`, we send EDS with proper settings but for some reason, Envoy does not reflect `zone`, `subzone` and `region` change in EDS settings. The most important for our feature is `priority` which **IS** reflected dynamically after we change endpoints.

This might be the caused by this issue https://github.com/envoyproxy/envoy/issues/12392

The problem only affects changing locality of endpoint. If we remove endpoint and reapply it again, it works.
If we restart the control plane, CDS version change and the data is ok again.

### The solution

I noticed that `zone`, `subzone` and `region` changes when we send CDS change after EDS. We could implement sending CDS change after EDS with changed locality, but detecting that we only changed locality is hard. Sending CDS change after every EDS is on the other hand expensive (extra data on the wire, cluster warming etc.).

The locality of the endpoint (aside of priority) does not really change dynamically in Kuma. Here are the cases
* You added `kuma.io/subzone` tag. In this case you have to redeploy the dataplane (unless you use old Kuma DP flow on Universal), the change will be visible, because old endpoint will be removed and new one will be added
* You change the zone of the remote CP, in this case, you need to remove zone from global and redeploy the CP with new zone so the change will be visible (ingress endpoints will be removed and applied)

My solution is to always set Locality to endpoint if we have proper tags BUT only set priority when locality-aware load balancing is enabled. This way we will always see `zone` on the endpoint in multizone deployment.


Patch:
diff --git a/pkg/xds/topology/outbound.go b/pkg/xds/topology/outbound.go
index 691dc8d11..72004e262 100644
--- a/pkg/xds/topology/outbound.go
+++ b/pkg/xds/topology/outbound.go
@@ -179,18 +179,25 @@ func convertToEnvoy(ds *v1alpha1.DataSource, mesh string, loader datasource.Load
 }
 
 func localityFromTags(mesh *mesh_core.MeshResource, priority uint32, tags map[string]string) *core_xds.Locality {
-	if !mesh.Spec.GetRouting().GetLocalityAwareLoadBalancing() {
-		return nil
-	}
-
 	region, regionPresent := tags[mesh_proto.RegionTag]
 	zone, zonePresent := tags[mesh_proto.ZoneTag]
 	subZone, subZonePresent := tags[mesh_proto.SubZoneTag]
 
 	if !regionPresent && !zonePresent && !subZonePresent {
+		// this means that we are running in standalone since in multi-zone Kuma always adds Zone tag automatically
 		return nil
 	}
 
+	if !mesh.Spec.GetRouting().GetLocalityAwareLoadBalancing() {
+		// we want to set the Locality even when localityAwareLoadbalancing is enabled
+		// If we set the locality we have an extra visibility about this in /clusters etc.
+		// Kuma's LocalityAwareLoadBalancing feature is based only on Priority therefore when it's disabled we need to set Priority to local
+		//
+		// Setting this regardless of LocalityAwareLoadBalancing on the mesh also solves the problem that endpoints have problems when moving from one locality to another
+		// https://github.com/envoyproxy/envoy/issues/12392
+		priority = priorityLocal
+	}
+
 	return &core_xds.Locality{
 		Region:   region,
 		Zone:     zone,


Test patch:
diff --git a/pkg/xds/topology/outbound_test.go b/pkg/xds/topology/outbound_test.go
index 33603ab22..4c806db59 100644
--- a/pkg/xds/topology/outbound_test.go
+++ b/pkg/xds/topology/outbound_test.go
@@ -59,7 +59,6 @@ var _ = Describe("TrafficRoute", func() {
 		dataSourceLoader = datasource.NewDataSourceLoader(secretManager)
 	})
 	Describe("GetOutboundTargets()", func() {
-
 		It("should pick proper dataplanes for each outbound destination", func() {
 			// given
 			backend := &mesh_core.DataplaneResource{ // dataplane that is a source of traffic
@@ -197,12 +196,18 @@ var _ = Describe("TrafficRoute", func() {
 						mesh_proto.ServiceTag: "elastic",
 						mesh_proto.RegionTag:  "eu",
 					},
+					Locality: &core_xds.Locality{
+						Region: "eu",
+					},
 					Weight: 1,
 				},
 				{
 					Target: "192.168.0.6",
 					Port:   9200,
 					Tags:   map[string]string{mesh_proto.ServiceTag: "elastic", mesh_proto.RegionTag: "us"},
+					Locality: &core_xds.Locality{
+						Region: "us",
+					},
 					Weight: 1,
 				},
 			}))
@@ -214,6 +219,9 @@ var _ = Describe("TrafficRoute", func() {
 						mesh_proto.ServiceTag: "frontend",
 						mesh_proto.RegionTag:  "eu",
 					},
+					Locality: &core_xds.Locality{
+						Region: "eu",
+					},
 					Weight: 1,
 				},
 			}))
@@ -225,6 +233,9 @@ var _ = Describe("TrafficRoute", func() {
 						mesh_proto.ServiceTag: "backend",
 						mesh_proto.RegionTag:  "eu",
 					},
+					Locality: &core_xds.Locality{
+						Region: "eu",
+					},
 					Weight: 1,
 				},
 			}))
@@ -362,6 +373,9 @@ var _ = Describe("TrafficRoute", func() {
 							Target: "192.168.0.100",
 							Port:   12345,
 							Tags:   map[string]string{mesh_proto.ServiceTag: "redis", "version": "v2", mesh_proto.RegionTag: "eu"},
+							Locality: &core_xds.Locality{
+								Region: "eu",
+							},
 							Weight: 2,
 						},
 						{
@@ -374,6 +388,9 @@ var _ = Describe("TrafficRoute", func() {
 							Target: "192.168.0.101",
 							Port:   12345,
 							Tags:   map[string]string{mesh_proto.ServiceTag: "redis", "version": "v2", mesh_proto.RegionTag: "eu"},
+							Locality: &core_xds.Locality{
+								Region: "eu",
+							},
 							Weight: 2,
 						},
 						{
@@ -445,6 +462,9 @@ var _ = Describe("TrafficRoute", func() {
 							Target: "192.168.0.100",
 							Port:   12345,
 							Tags:   map[string]string{mesh_proto.ServiceTag: "redis", "version": "v2", mesh_proto.RegionTag: "eu"},
+							Locality: &core_xds.Locality{
+								Region: "eu",
+							},
 							Weight: 2,
 						},
 						{