NET-5951 - Unique route names for implicit routes (#19174)

* NET-5951 - Unique route names for implicit routes * remove use of datacenter * PR feedback
hashicorp · Oct 13, 2023 · 145d995 · 145d995
1 parent 71cb64b
commit 145d995
Show file tree

Hide file tree

Showing 10 changed files with 65 additions and 32 deletions.
diff --git a/internal/mesh/internal/controllers/sidecarproxy/builder/destination_builder.go b/internal/mesh/internal/controllers/sidecarproxy/builder/destination_builder.go
@@ -89,6 +89,13 @@ func (b *Builder) buildDestination(
 		defaultDC(""),
 	)
 
+	var routeName string
+	if destination.Explicit != nil {
+		routeName = lb.listener.Name
+	} else {
+		routeName = DestinationResourceID(cpr.ParentRef.Ref)
+	}
+
 	var (
 		useRDS                bool
 		needsNullRouteCluster bool
@@ -100,8 +107,6 @@ func (b *Builder) buildDestination(
 
 		route := config.Http
 
-		listenerName := lb.listener.Name
-
 		// this corresponds to roughly "makeUpstreamRouteForDiscoveryChain"
 
 		var proxyRouteRules []*pbproxystate.RouteRule
@@ -134,9 +139,9 @@ func (b *Builder) buildDestination(
 			}
 		}
 
-		b.addRoute(listenerName, &pbproxystate.Route{
+		b.addRoute(routeName, &pbproxystate.Route{
 			VirtualHosts: []*pbproxystate.VirtualHost{{
-				Name:       listenerName,
+				Name:       routeName,
 				RouteRules: proxyRouteRules,
 			}},
 		})
@@ -145,8 +150,6 @@ func (b *Builder) buildDestination(
 		useRDS = true
 		route := config.Grpc
 
-		listenerName := lb.listener.Name
-
 		var proxyRouteRules []*pbproxystate.RouteRule
 		for _, routeRule := range route.Rules {
 			for _, backendRef := range routeRule.BackendRefs {
@@ -178,9 +181,9 @@ func (b *Builder) buildDestination(
 			}
 		}
 
-		b.addRoute(listenerName, &pbproxystate.Route{
+		b.addRoute(routeName, &pbproxystate.Route{
 			VirtualHosts: []*pbproxystate.VirtualHost{{
-				Name:       listenerName,
+				Name:       routeName,
 				RouteRules: proxyRouteRules,
 			}},
 		})
@@ -479,7 +482,7 @@ func makeExplicitListener(explicit *pbmesh.Destination, direction pbproxystate.D
 				Port: destinationAddr.IpPort.Port,
 			},
 		}
-		listener.Name = DestinationListenerName(explicit.DestinationRef.Name, explicit.DestinationPort, destinationAddr.IpPort.Ip, destinationAddr.IpPort.Port)
+		listener.Name = DestinationListenerName(explicit.DestinationRef, explicit.DestinationPort, destinationAddr.IpPort.Ip, destinationAddr.IpPort.Port)
 	case *pbmesh.Destination_Unix:
 		destinationAddr := explicit.ListenAddr.(*pbmesh.Destination_Unix)
 		listener.BindAddress = &pbproxystate.Listener_UnixSocket{
@@ -488,7 +491,7 @@ func makeExplicitListener(explicit *pbmesh.Destination, direction pbproxystate.D
 				Mode: destinationAddr.Unix.Mode,
 			},
 		}
-		listener.Name = DestinationListenerName(explicit.DestinationRef.Name, explicit.DestinationPort, destinationAddr.Unix.Path, 0)
+		listener.Name = DestinationListenerName(explicit.DestinationRef, explicit.DestinationPort, destinationAddr.Unix.Path, 0)
 	}
 
 	return listener

diff --git a/internal/mesh/internal/controllers/sidecarproxy/builder/naming.go b/internal/mesh/internal/controllers/sidecarproxy/builder/naming.go
@@ -28,10 +28,19 @@ func DestinationStatPrefix(serviceRef *pbresource.Reference, portName, datacente
 		datacenter)
 }
 
-func DestinationListenerName(name, portName string, address string, port uint32) string {
+func DestinationListenerName(destinationRef *pbresource.Reference, portName string, address string, port uint32) string {
+	name := fmt.Sprintf("%s:%s:%s", DestinationResourceID(destinationRef), portName, address)
 	if port != 0 {
-		return fmt.Sprintf("%s:%s:%s:%d", name, portName, address, port)
+		return fmt.Sprintf("%s:%d", name, port)
 	}
 
-	return fmt.Sprintf("%s:%s:%s", name, portName, address)
+	return name
+}
+
+// DestinationResourceID returns a string representation that uniquely identifies the
+// upstream in a canonical but human readable way.
+func DestinationResourceID(destinationRef *pbresource.Reference) string {
+	tenancyPrefix := fmt.Sprintf("%s/%s/%s", destinationRef.Tenancy.Partition,
+		destinationRef.Tenancy.PeerName, destinationRef.Tenancy.Namespace)
+	return fmt.Sprintf("%s/%s", tenancyPrefix, destinationRef.Name)
 }
diff --git a/...carproxy/builder/testdata/destination/l4-implicit-and-explicit-destinations-tproxy.golden b/...carproxy/builder/testdata/destination/l4-implicit-and-explicit-destinations-tproxy.golden
@@ -81,7 +81,7 @@
           "host": "1.1.1.1",
           "port": 1234
         },
-        "name": "api-1:tcp:1.1.1.1:1234",
+        "name": "default/local/default/api-1:tcp:1.1.1.1:1234",
         "routers": [
           {
             "l4": {

diff --git a/...nternal/controllers/sidecarproxy/builder/testdata/destination/l4-multi-destination.golden b/...nternal/controllers/sidecarproxy/builder/testdata/destination/l4-multi-destination.golden
@@ -135,7 +135,7 @@
           "host": "1.1.1.1",
           "port": 1234
         },
-        "name": "api-1:tcp:1.1.1.1:1234",
+        "name": "default/local/default/api-1:tcp:1.1.1.1:1234",
         "routers": [
           {
             "l4": {
@@ -162,7 +162,7 @@
       },
       {
         "direction": "DIRECTION_OUTBOUND",
-        "name": "api-2:tcp:/path/to/socket",
+        "name": "default/local/default/api-2:tcp:/path/to/socket",
         "routers": [
           {
             "l4": {
@@ -184,7 +184,7 @@
           "host": "1.1.1.1",
           "port": 2345
         },
-        "name": "api-1:tcp2:1.1.1.1:2345",
+        "name": "default/local/default/api-1:tcp2:1.1.1.1:2345",
         "routers": [
           {
             "l4": {
@@ -211,7 +211,7 @@
       },
       {
         "direction": "DIRECTION_OUTBOUND",
-        "name": "api-2:tcp2:/path/to/socket",
+        "name": "default/local/default/api-2:tcp2:/path/to/socket",
         "routers": [
           {
             "l4": {

diff --git a/...decarproxy/builder/testdata/destination/l4-single-destination-ip-port-bind-address.golden b/...decarproxy/builder/testdata/destination/l4-single-destination-ip-port-bind-address.golden
@@ -81,7 +81,7 @@
           "host": "1.1.1.1",
           "port": 1234
         },
-        "name": "api-1:tcp:1.1.1.1:1234",
+        "name": "default/local/default/api-1:tcp:1.1.1.1:1234",
         "routers": [
           {
             "l4": {

diff --git a/...rproxy/builder/testdata/destination/l4-single-destination-unix-socket-bind-address.golden b/...rproxy/builder/testdata/destination/l4-single-destination-unix-socket-bind-address.golden
@@ -40,7 +40,7 @@
     "listeners": [
       {
         "direction": "DIRECTION_OUTBOUND",
-        "name": "api-2:tcp:/path/to/socket",
+        "name": "default/local/default/api-2:tcp:/path/to/socket",
         "routers": [
           {
             "l4": {

diff --git a/...rnal/controllers/sidecarproxy/builder/testdata/destination/mixed-multi-destination.golden b/...rnal/controllers/sidecarproxy/builder/testdata/destination/mixed-multi-destination.golden
@@ -167,7 +167,7 @@
           "host": "1.1.1.1",
           "port": 1234
         },
-        "name": "api-1:tcp:1.1.1.1:1234",
+        "name": "default/local/default/api-1:tcp:1.1.1.1:1234",
         "routers": [
           {
             "l4": {
@@ -194,7 +194,7 @@
       },
       {
         "direction": "DIRECTION_OUTBOUND",
-        "name": "api-2:tcp:/path/to/socket",
+        "name": "default/local/default/api-2:tcp:/path/to/socket",
         "routers": [
           {
             "l4": {
@@ -216,22 +216,22 @@
           "host": "1.1.1.1",
           "port": 1234
         },
-        "name": "api-1:http:1.1.1.1:1234",
+        "name": "default/local/default/api-1:http:1.1.1.1:1234",
         "routers": [
           {
             "l7": {
-              "name": "api-1:http:1.1.1.1:1234",
+              "name": "default/local/default/api-1:http:1.1.1.1:1234",
               "statPrefix": "upstream."
             }
           }
         ]
       }
     ],
     "routes": {
-      "api-1:http:1.1.1.1:1234": {
+      "default/local/default/api-1:http:1.1.1.1:1234": {
         "virtualHosts": [
           {
-            "name": "api-1:http:1.1.1.1:1234",
+            "name": "default/local/default/api-1:http:1.1.1.1:1234",
             "routeRules": [
               {
                 "destination": {

diff --git a/...xy/builder/testdata/destination/multiport-l4-multiple-implicit-destinations-tproxy.golden b/...xy/builder/testdata/destination/multiport-l4-multiple-implicit-destinations-tproxy.golden
@@ -316,10 +316,31 @@
       }
     ],
     "routes": {
-      "outbound_listener": {
+      "default/local/default/api-app": {
         "virtualHosts": [
           {
-            "name": "outbound_listener",
+            "name": "default/local/default/api-app",
+            "routeRules": [
+              {
+                "destination": {
+                  "cluster": {
+                    "name": "http.api-app.default.dc1.internal.foo.consul"
+                  }
+                },
+                "match": {
+                  "pathMatch": {
+                    "prefix": "/"
+                  }
+                }
+              }
+            ]
+          }
+        ]
+      },
+      "default/local/default/api-app2": {
+        "virtualHosts": [
+          {
+            "name": "default/local/default/api-app2",
             "routeRules": [
               {
                 "destination": {

diff --git a/...proxy/builder/testdata/destination/multiport-l4-single-implicit-destination-tproxy.golden b/...proxy/builder/testdata/destination/multiport-l4-single-implicit-destination-tproxy.golden
@@ -174,10 +174,10 @@
       }
     ],
     "routes": {
-      "outbound_listener": {
+      "default/local/default/api-app": {
         "virtualHosts": [
           {
-            "name": "outbound_listener",
+            "name": "default/local/default/api-app",
             "routeRules": [
               {
                 "destination": {

diff --git a/...estination/multiport-l4-single-implicit-destination-with-multiple-workloads-tproxy.golden b/...estination/multiport-l4-single-implicit-destination-with-multiple-workloads-tproxy.golden
@@ -174,10 +174,10 @@
       }
     ],
     "routes": {
-      "outbound_listener": {
+      "default/local/default/api-app": {
         "virtualHosts": [
           {
-            "name": "outbound_listener",
+            "name": "default/local/default/api-app",
             "routeRules": [
               {
                 "destination": {