command/envoy: stop using the DebugConfig from Self endpoint

The DebugConfig in the self endpoint can change at any time. It's not a stable API.

This commit adds the XDSPort to a stable part of the XDS api, and changes the envoy command to read
this new field.

It includes support for the old API as well, in case a newer CLI is used with an older API, and
adds a test for both cases.

agent/agent.go

@@ -692,10 +692,7 @@ func (a *Agent) listenAndServeGRPC() error {
 		tlsConfig = nil
 	}
 	var err error
-	a.grpcServer, err = xdsServer.GRPCServer(tlsConfig)
+	a.grpcServer = xds.NewGRPCServer(xdsServer, tlsConfig)
-	if err != nil {
-		return err
-	}
 
 	ln, err := a.startListeners(a.config.GRPCAddrs)
 	if err != nil {

agent/agent_endpoint.go

@@ -39,11 +39,12 @@ type Self struct {
 	Member      serf.Member
 	Stats       map[string]map[string]string
 	Meta        map[string]string
-	XDS         *xdsSelf `json:"xDS,omitempty"`
+	XDS         *XDSSelf `json:"xDS,omitempty"`
 }
 
-type xdsSelf struct {
+type XDSSelf struct {
 	SupportedProxies map[string][]string
+	Port             int
 }
 
 func (s *HTTPHandlers) AgentSelf(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
@@ -66,12 +67,13 @@ func (s *HTTPHandlers) AgentSelf(resp http.ResponseWriter, req *http.Request) (i
 		}
 	}
 
-	var xds *xdsSelf
+	var xds *XDSSelf
 	if s.agent.grpcServer != nil {
-		xds = &xdsSelf{
+		xds = &XDSSelf{
 			SupportedProxies: map[string][]string{
 				"envoy": proxysupport.EnvoyVersions,
 			},
+			Port: s.agent.config.GRPCPort,
 		}
 	}
 

agent/xds/server.go

@@ -548,8 +548,9 @@ func tokenFromContext(ctx context.Context) string {
 	return ""
 }
 
-// GRPCServer returns a server instance that can handle xDS requests.
+// NewGRPCServer creates a grpc.Server, registers the Server, and then returns
-func (s *Server) GRPCServer(tlsConfigurator *tlsutil.Configurator) (*grpc.Server, error) {
+// the grpc.Server.
+func NewGRPCServer(s *Server, tlsConfigurator *tlsutil.Configurator) *grpc.Server {
 	opts := []grpc.ServerOption{
 		grpc.MaxConcurrentStreams(2048),
 	}
@@ -565,8 +566,7 @@ func (s *Server) GRPCServer(tlsConfigurator *tlsutil.Configurator) (*grpc.Server
 	if !s.DisableV2Protocol {
 		envoy_discovery_v2.RegisterAggregatedDiscoveryServiceServer(srv, &adsServerV2Shim{srv: s})
 	}
-
+	return srv
-	return srv, nil
 }
 
 // authorize the xDS request using the token stored in ctx. This authorization is

command/connect/envoy/envoy.go

@@ -426,7 +426,7 @@ func (c *cmd) templateArgs() (*BootstrapTplArgs, error) {
 		return nil, err
 	}
 
-	grpcAddr, err := c.grpcAddress(httpCfg)
+	xdsAddr, err := c.xdsAddress(httpCfg)
 	if err != nil {
 		return nil, err
 	}
@@ -471,7 +471,7 @@ func (c *cmd) templateArgs() (*BootstrapTplArgs, error) {
 	caPEM = strings.Replace(strings.Join(pems, ""), "\n", "\\n", -1)
 
 	return &BootstrapTplArgs{
-		GRPC:                  grpcAddr,
+		GRPC:                  xdsAddr,
 		ProxyCluster:          cluster,
 		ProxyID:               c.proxyID,
 		ProxySourceService:    proxySourceService,
@@ -557,13 +557,12 @@ func (c *cmd) generateConfig() ([]byte, error) {
 }
 
 // TODO: make method a function
-func (c *cmd) grpcAddress(httpCfg *api.Config) (GRPC, error) {
+func (c *cmd) xdsAddress(httpCfg *api.Config) (GRPC, error) {
 	g := GRPC{}
 
 	addr := c.grpcAddr
-	// See if we need to lookup grpcAddr
 	if addr == "" {
-		port, err := c.lookupGRPCPort()
+		port, err := c.lookupXDSPort()
 		if err != nil {
 			c.UI.Error(fmt.Sprintf("Error connecting to Consul agent: %s", err))
 		}
@@ -621,11 +620,25 @@ func (c *cmd) grpcAddress(httpCfg *api.Config) (GRPC, error) {
 	return g, nil
 }
 
-func (c *cmd) lookupGRPCPort() (int, error) {
+func (c *cmd) lookupXDSPort() (int, error) {
 	self, err := c.client.Agent().Self()
 	if err != nil {
 		return 0, err
 	}
+
+	type response struct {
+		XDS struct {
+			Port int
+		}
+	}
+
+	var resp response
+	if err := mapstructure.Decode(self, &resp); err == nil && resp.XDS.Port != 0 {
+		return resp.XDS.Port, nil
+	}
+
+	// Fallback to old API for the case where a new consul CLI is being used with
+	// an older API version.
 	cfg, ok := self["DebugConfig"]
 	if !ok {
 		return 0, fmt.Errorf("unexpected agent response: no debug config")

command/connect/envoy/envoy_test.go

@@ -112,7 +112,8 @@ type generateConfigTestCase struct {
 	Files             map[string]string
 	ProxyConfig       map[string]interface{}
 	NamespacesEnabled bool
-	GRPCPort          int // only used for testing custom-configured grpc port
+	XDSPort           int  // only used for testing custom-configured grpc port
+	AgentSelf110      bool // fake the agent API from versions v1.10 and earlier
 	WantArgs          BootstrapTplArgs
 	WantErr           string
 }
@@ -357,9 +358,35 @@ func TestGenerateConfig(t *testing.T) {
 			},
 		},
 		{
-			Name:     "grpc-addr-config",
+			Name:    "xds-addr-config",
 			Flags:   []string{"-proxy-id", "test-proxy"},
-			GRPCPort: 9999,
+			XDSPort: 9999,
+			WantArgs: BootstrapTplArgs{
+				EnvoyVersion: defaultEnvoyVersion,
+				ProxyCluster: "test-proxy",
+				ProxyID:      "test-proxy",
+				// We don't know this til after the lookup so it will be empty in the
+				// initial args call we are testing here.
+				ProxySourceService: "",
+				// Should resolve IP, note this might not resolve the same way
+				// everywhere which might make this test brittle but not sure what else
+				// to do.
+				GRPC: GRPC{
+					AgentAddress: "127.0.0.1",
+					AgentPort:    "9999",
+				},
+				AdminAccessLogPath:    "/dev/null",
+				AdminBindAddress:      "127.0.0.1",
+				AdminBindPort:         "19000",
+				LocalAgentClusterName: xds.LocalAgentClusterName,
+				PrometheusScrapePath:  "/metrics",
+			},
+		},
+		{
+			Name:         "deprecated-grpc-addr-config",
+			Flags:        []string{"-proxy-id", "test-proxy"},
+			XDSPort:      9999,
+			AgentSelf110: true,
 			WantArgs: BootstrapTplArgs{
 				EnvoyVersion: defaultEnvoyVersion,
 				ProxyCluster: "test-proxy",
@@ -1014,29 +1041,23 @@ func TestEnvoy_GatewayRegistration(t *testing.T) {
 // testMockAgent combines testMockAgentProxyConfig and testMockAgentSelf,
 // routing /agent/service/... requests to testMockAgentProxyConfig and
 // routing /agent/self requests to testMockAgentSelf.
-func testMockAgent(agentCfg map[string]interface{}, grpcPort int, namespacesEnabled bool) http.HandlerFunc {
+func testMockAgent(tc generateConfigTestCase) http.HandlerFunc {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	return func(w http.ResponseWriter, r *http.Request) {
-		if strings.Contains(r.URL.Path, "/agent/services") {
+		switch {
-			testMockAgentGatewayConfig(namespacesEnabled)(w, r)
+		case strings.Contains(r.URL.Path, "/agent/services"):
-			return
+			testMockAgentGatewayConfig(tc.NamespacesEnabled)(w, r)
-		}
+		case strings.Contains(r.URL.Path, "/agent/service"):
-
+			testMockAgentProxyConfig(tc.ProxyConfig, tc.NamespacesEnabled)(w, r)
-		if strings.Contains(r.URL.Path, "/agent/service") {
+		case strings.Contains(r.URL.Path, "/agent/self"):
-			testMockAgentProxyConfig(agentCfg, namespacesEnabled)(w, r)
+			testMockAgentSelf(tc.XDSPort, tc.AgentSelf110)(w, r)
-			return
+		default:
-		}
-
-		if strings.Contains(r.URL.Path, "/agent/self") {
-			testMockAgentSelf(grpcPort)(w, r)
-			return
-		}
-
 			http.NotFound(w, r)
-	})
+		}
+	}
 }
 
 func testMockAgentGatewayConfig(namespacesEnabled bool) http.HandlerFunc {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	return func(w http.ResponseWriter, r *http.Request) {
 		// Parse the proxy-id from the end of the URL (blindly assuming it's correct
 		// format)
 		params := r.URL.Query()
@@ -1071,7 +1092,7 @@ func testMockAgentGatewayConfig(namespacesEnabled bool) http.HandlerFunc {
 			return
 		}
 		w.Write(cfgJSON)
-	})
+	}
 }
 
 func namespaceFromQuery(r *http.Request) string {
@@ -1093,7 +1114,7 @@ func partitionFromQuery(r *http.Request) string {
 }
 
 func testMockAgentProxyConfig(cfg map[string]interface{}, namespacesEnabled bool) http.HandlerFunc {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	return func(w http.ResponseWriter, r *http.Request) {
 		// Parse the proxy-id from the end of the URL (blindly assuming it's correct
 		// format)
 		proxyID := strings.TrimPrefix(r.URL.Path, "/v1/agent/service/")
@@ -1123,7 +1144,7 @@ func testMockAgentProxyConfig(cfg map[string]interface{}, namespacesEnabled bool
 			return
 		}
 		w.Write(cfgJSON)
-	})
+	}
 }
 
 func TestEnvoyCommand_canBindInternal(t *testing.T) {
@@ -1223,16 +1244,21 @@ func TestEnvoyCommand_canBindInternal(t *testing.T) {
 }
 
 // testMockAgentSelf returns an empty /v1/agent/self response except GRPC
-// port is filled in to match the given wantGRPCPort argument.
+// port is filled in to match the given wantXDSPort argument.
-func testMockAgentSelf(wantGRPCPort int) http.HandlerFunc {
+func testMockAgentSelf(wantXDSPort int, agentSelf110 bool) http.HandlerFunc {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	return func(w http.ResponseWriter, r *http.Request) {
 		resp := agent.Self{
 			Config: map[string]interface{}{
 				"Datacenter": "dc1",
 			},
-			DebugConfig: map[string]interface{}{
+		}
-				"GRPCPort": wantGRPCPort,
+
-			},
+		if agentSelf110 {
+			resp.DebugConfig = map[string]interface{}{
+				"GRPCPort": wantXDSPort,
+			}
+		} else {
+			resp.XDS = &agent.XDSSelf{Port: wantXDSPort}
 		}
 
 		selfJSON, err := json.Marshal(resp)
@@ -1242,5 +1268,5 @@ func testMockAgentSelf(wantGRPCPort int) http.HandlerFunc {
 			return
 		}
 		w.Write(selfJSON)
-	})
+	}
 }

command/connect/envoy/testdata/xds-addr-config.golden

@@ -0,0 +1,185 @@
+{
+  "admin": {
+    "access_log_path": "/dev/null",
+    "address": {
+      "socket_address": {
+        "address": "127.0.0.1",
+        "port_value": 19000
+      }
+    }
+  },
+  "node": {
+    "cluster": "test-proxy",
+    "id": "test-proxy",
+    "metadata": {
+      "namespace": "default",
+      "partition": "default",
+      "envoy_version": "1.18.4"
+    }
+  },
+  "static_resources": {
+    "clusters": [
+      {
+        "name": "local_agent",
+        "ignore_health_on_host_removal": false,
+        "connect_timeout": "1s",
+        "type": "STATIC",
+        "http2_protocol_options": {},
+        "loadAssignment": {
+          "clusterName": "local_agent",
+          "endpoints": [
+            {
+              "lbEndpoints": [
+                {
+                  "endpoint": {
+                    "address": {
+                      "socket_address": {
+                        "address": "127.0.0.1",
+                        "port_value": 9999
+                      }
+                    }
+                  }
+                }
+              ]
+            }
+          ]
+        }
+      }
+    ]
+  },
+  "stats_config": {
+    "stats_tags": [
+      {
+        "regex": "^cluster\\.(?:passthrough~)?((?:([^.]+)~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.destination.custom_hash"
+      },
+      {
+        "regex": "^cluster\\.(?:passthrough~)?((?:[^.]+~)?(?:([^.]+)\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.destination.service_subset"
+      },
+      {
+        "regex": "^cluster\\.(?:passthrough~)?((?:[^.]+~)?(?:[^.]+\\.)?([^.]+)\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.destination.service"
+      },
+      {
+        "regex": "^cluster\\.(?:passthrough~)?((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.([^.]+)\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.destination.namespace"
+      },
+      {
+        "regex": "^cluster\\.(?:passthrough~)?((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.([^.]+)\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.destination.datacenter"
+      },
+      {
+        "regex": "^cluster\\.(?:passthrough~)?((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.([^.]+)\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.destination.routing_type"
+      },
+      {
+        "regex": "^cluster\\.(?:passthrough~)?((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.([^.]+)\\.consul\\.)",
+        "tag_name": "consul.destination.trust_domain"
+      },
+      {
+        "regex": "^cluster\\.(?:passthrough~)?(((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+)\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.destination.target"
+      },
+      {
+        "regex": "^cluster\\.(?:passthrough~)?(((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+)\\.consul\\.)",
+        "tag_name": "consul.destination.full_target"
+      },
+      {
+        "regex": "^(?:tcp|http)\\.upstream\\.(([^.]+)(?:\\.[^.]+)?\\.[^.]+\\.)",
+        "tag_name": "consul.upstream.service"
+      },
+      {
+        "regex": "^(?:tcp|http)\\.upstream\\.([^.]+(?:\\.[^.]+)?\\.([^.]+)\\.)",
+        "tag_name": "consul.upstream.datacenter"
+      },
+      {
+        "regex": "^(?:tcp|http)\\.upstream\\.([^.]+(?:\\.([^.]+))?\\.[^.]+\\.)",
+        "tag_name": "consul.upstream.namespace"
+      },
+      {
+        "regex": "^cluster\\.((?:([^.]+)~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.custom_hash"
+      },
+      {
+        "regex": "^cluster\\.((?:[^.]+~)?(?:([^.]+)\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.service_subset"
+      },
+      {
+        "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?([^.]+)\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.service"
+      },
+      {
+        "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.([^.]+)\\.[^.]+\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.namespace"
+      },
+      {
+        "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.([^.]+)\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.datacenter"
+      },
+      {
+        "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.([^.]+)\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.routing_type"
+      },
+      {
+        "regex": "^cluster\\.((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.([^.]+)\\.consul\\.)",
+        "tag_name": "consul.trust_domain"
+      },
+      {
+        "regex": "^cluster\\.(((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+)\\.[^.]+\\.[^.]+\\.consul\\.)",
+        "tag_name": "consul.target"
+      },
+      {
+        "regex": "^cluster\\.(((?:[^.]+~)?(?:[^.]+\\.)?[^.]+\\.[^.]+\\.[^.]+\\.[^.]+\\.[^.]+)\\.consul\\.)",
+        "tag_name": "consul.full_target"
+      },
+      {
+        "tag_name": "local_cluster",
+        "fixed_value": "test-proxy"
+      },
+      {
+        "tag_name": "consul.source.service",
+        "fixed_value": "test-proxy"
+      },
+      {
+        "tag_name": "consul.source.namespace",
+        "fixed_value": "default"
+      },
+      {
+        "tag_name": "consul.source.partition",
+        "fixed_value": "default"
+      },
+      {
+        "tag_name": "consul.source.datacenter",
+        "fixed_value": "dc1"
+      }
+    ],
+    "use_all_default_tags": true
+  },
+  "dynamic_resources": {
+    "lds_config": {
+      "ads": {},
+      "resource_api_version": "V3"
+    },
+    "cds_config": {
+      "ads": {},
+      "resource_api_version": "V3"
+    },
+    "ads_config": {
+      "api_type": "DELTA_GRPC",
+      "transport_api_version": "V3",
+      "grpc_services": {
+        "initial_metadata": [
+          {
+            "key": "x-consul-token",
+            "value": ""
+          }
+        ],
+        "envoy_grpc": {
+          "cluster_name": "local_agent"
+        }
+      }
+    }
+  }
+}
+