feat(kuma-cp) multiple outbound tags #831

jakubdyszkiewicz · 2020-06-15T10:49:28Z

Summary

This PR introduces support for multiple tags in outbound section.
so instead of

outbound:
- port: 1234
  service: backend

we've got

outbound:
- port: 1234
  tags:
    service: backend

many tags are supported here, but service is required.

I introduced backward compatibility just like we did with "interface" field, meaning user can still use "service" field.

Additionally, this PR refactors TrafficRoute to use builtin mechanism in Envoy for such functionallity called load balancing subsets.
Every endpoint has metadata with tags. When we generate TCP Proxy or route for HTTP Connection Manager we can use Weighted Clusters providing metadata.
For this funcionallity to work, we need lb split settings on cluster.

On top of that, OutboundProxyGenerator was getting really big so I split it into many functions.

Documentation

In progress

nickolaev

Looks very impressive!

Makefile.e2e.mk

nickolaev · 2020-06-15T18:43:51Z

pkg/plugins/discovery/k8s/controllers/testdata/02.dataplane.yaml

@@ -14,7 +14,9 @@ spec:
    outbound:
      - address: 10.108.144.24
        port: 443
-        service: test-app.playground.svc:443
+        tags:
+          service: test-app.playground.svc:443


I guess test-app_playground_svc_443?

Yes, but I don't want to introduce it in this PR.

pkg/xds/envoy/listeners/tcp_proxy_configurer_test.go

lobkovilya · 2020-06-16T09:42:26Z

pkg/xds/envoy/clusters/lb_subset_configurer.go

+	var selectors []*envoy_api.Cluster_LbSubsetConfig_LbSubsetSelector
+	subsets := map[string]bool{} // we only need unique subsets
+	for _, tags := range e.tags {
+		keys := tags.WithoutTag(v1alpha1.ServiceTag).Keys() // service tag is not included in metadata


Could you please use alias mesh_proto.ServiceTag?

lobkovilya · 2020-06-16T09:54:56Z

pkg/xds/envoy/listeners/tcp_proxy_configurer.go

@@ -47,16 +46,17 @@ func (c *TcpProxyConfigurer) tcpProxy() *envoy_tcp.TcpProxy {
 	proxy := envoy_tcp.TcpProxy{
 		StatPrefix: util_xds.SanitizeMetric(c.statsName),
 	}
-	if len(c.clusters) == 1 {
+	if len(c.clusters) == 1 && envoy_common.Metadata(c.clusters[0].Tags) == nil {


Probably makes sense to check len(c.cluster[0].Tags) == 0. If Tags is nil that expression also will be true. But it covers case when Tags is empty map. I think we don't want to generate TcpProxy_WeightedCluster_ClusterWeight with empty Tags map

You may want to generate TcpProxy_WeightedCluster_ClusterWeight with empty tags map. TrafficRoute from web to backend with split 50% to backend and 50% to backend-api (different name, but without tags).

lobkovilya · 2020-06-16T09:58:14Z

pkg/xds/envoy/routes/default_route_configurer.go

@@ -44,16 +44,17 @@ type RouteConfigurer struct {

 func (c RouteConfigurer) routeAction() *envoy_route.RouteAction {
 	routeAction := envoy_route.RouteAction{}
-	if len(c.clusters) == 1 {
+	if len(c.clusters) == 1 && envoy_common.Metadata(c.clusters[0].Tags) == nil {


And probably the same here?

lobkovilya · 2020-06-16T10:15:25Z

pkg/xds/generator/outbound_proxy_generator.go

+	for _, outbound := range outbounds {
+		// Determine the list of destination clusters
+		// For one outbound listener it may contain many clusters (ex. TrafficRoute to many destinations)
+		clusters, err := g.determineClusters(proxy, outbound)


clusters and allClusters are not mapped to Envoy clusters 1 to 1, maybe make sense to consider renaming to subset or something like that

Ok, changed ClusterInfo to ClusterSubsets. and ClusterInfo#Name to ClusterSubset#ClusterName. I think it make more sense now.

lobkovilya · 2020-06-16T10:19:46Z

pkg/xds/generator/outbound_proxy_generator.go

+func (_ OutboundProxyGenerator) generateEDS(proxy *model.Proxy, clusters envoy_common.Clusters) model.ResourceSet {
+	var resources model.ResourceSet
+	for _, clusterName := range clusters.ClusterNames() {
+		serviceName := clusters.Tags(clusterName)[0][kuma_mesh.ServiceTag]


Does serviceName equal to clusterName?

For now it does, but just for safety I don't want to make this assumption.

lobkovilya · 2020-06-17T11:17:47Z

pkg/xds/generator/outbound_proxy_generator.go

-func (_ OutboundProxyGenerator) generateEds(ctx xds_context.Context, proxy *model.Proxy, clusters []envoy_common.ClusterInfo) (resources []*model.Resource, allEndpoints []model.Endpoint, _ error) {
-	for _, cluster := range clusters {
-		serviceName := cluster.Tags[kuma_mesh.ServiceTag]
+func (_ OutboundProxyGenerator) generateCDS(ctx xds_context.Context, proxy *model.Proxy, clusters envoy_common.Clusters) (model.ResourceSet, error) {


When it comes to acronyms I thought we always make just the first letter capital. But I know that according to a convention - https://github.com/golang/go/wiki/CodeReviewComments#initialisms, letters should be in a consistent case. So do we want to follow it everywhere now?

Yes, that's why I changed generateEds to generateEDS

jakubdyszkiewicz requested a review from a team June 15, 2020 10:49

nickolaev added the multicluster label Jun 15, 2020

nickolaev approved these changes Jun 15, 2020

View reviewed changes

lobkovilya reviewed Jun 16, 2020

View reviewed changes

nickolaev requested a review from lobkovilya June 16, 2020 17:40

jakubdyszkiewicz added 5 commits June 17, 2020 11:01

traffic route subset lb

0f3c1b9

outbound tags

4c8facf

move clusters

355e58d

refactor outbound proxy generator

68641fa

e2e comments

b0e3ece

nickolaev mentioned this pull request Jun 17, 2020

chore(kuma-cp) remove interface from dataplane model #832

Merged

fixes after rebase

d9477f7

jakubdyszkiewicz force-pushed the feat/outbound-tags branch from 1d5c921 to d9477f7 Compare June 17, 2020 10:08

rename clusterinfo to clustersubset

20f9e2b

lobkovilya reviewed Jun 17, 2020

View reviewed changes

lobkovilya approved these changes Jun 17, 2020

View reviewed changes

jakubdyszkiewicz merged commit 971c1a7 into master Jun 17, 2020

nickolaev mentioned this pull request Jun 17, 2020

Ingres and outbound tags incompatibility #837

Closed

jakubdyszkiewicz deleted the feat/outbound-tags branch October 15, 2020 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kuma-cp) multiple outbound tags #831

feat(kuma-cp) multiple outbound tags #831

jakubdyszkiewicz commented Jun 15, 2020

nickolaev left a comment

nickolaev Jun 15, 2020

jakubdyszkiewicz Jun 16, 2020

lobkovilya Jun 16, 2020

lobkovilya Jun 16, 2020

jakubdyszkiewicz Jun 17, 2020

lobkovilya Jun 16, 2020

lobkovilya Jun 16, 2020

jakubdyszkiewicz Jun 17, 2020

lobkovilya Jun 16, 2020

jakubdyszkiewicz Jun 17, 2020

lobkovilya Jun 17, 2020

jakubdyszkiewicz Jun 17, 2020

feat(kuma-cp) multiple outbound tags #831

feat(kuma-cp) multiple outbound tags #831

Conversation

jakubdyszkiewicz commented Jun 15, 2020

Summary

Documentation

nickolaev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment