From cc310224aa24b3d7ed88e700be7fc95154dbbd9f Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 9 Jul 2021 15:17:29 -0400 Subject: [PATCH] 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 | 5 +- agent/agent_endpoint.go | 10 +- agent/xds/server.go | 8 +- command/connect/envoy/envoy.go | 25 ++- command/connect/envoy/envoy_test.go | 92 +++++---- ...den => deprecated-grpc-addr-config.golden} | 0 .../envoy/testdata/xds-addr-config.golden | 185 ++++++++++++++++++ 7 files changed, 274 insertions(+), 51 deletions(-) rename command/connect/envoy/testdata/{grpc-addr-config.golden => deprecated-grpc-addr-config.golden} (100%) create mode 100644 command/connect/envoy/testdata/xds-addr-config.golden diff --git a/agent/agent.go b/agent/agent.go index de35ba77b2..5a665c6cb5 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -692,10 +692,7 @@ func (a *Agent) listenAndServeGRPC() error { tlsConfig = nil } var err error - a.grpcServer, err = xdsServer.GRPCServer(tlsConfig) - if err != nil { - return err - } + a.grpcServer = xds.NewGRPCServer(xdsServer, tlsConfig) ln, err := a.startListeners(a.config.GRPCAddrs) if err != nil { diff --git a/agent/agent_endpoint.go b/agent/agent_endpoint.go index c389ccce59..a22a3ea9d2 100644 --- a/agent/agent_endpoint.go +++ b/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, } } diff --git a/agent/xds/server.go b/agent/xds/server.go index c4a673fcd4..346be40cb8 100644 --- a/agent/xds/server.go +++ b/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. -func (s *Server) GRPCServer(tlsConfigurator *tlsutil.Configurator) (*grpc.Server, error) { +// NewGRPCServer creates a grpc.Server, registers the Server, and then returns +// 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, nil + return srv } // authorize the xDS request using the token stored in ctx. This authorization is diff --git a/command/connect/envoy/envoy.go b/command/connect/envoy/envoy.go index 989fcdb902..1615887815 100644 --- a/command/connect/envoy/envoy.go +++ b/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") diff --git a/command/connect/envoy/envoy_test.go b/command/connect/envoy/envoy_test.go index ef999cc240..c79f4551fa 100644 --- a/command/connect/envoy/envoy_test.go +++ b/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", - Flags: []string{"-proxy-id", "test-proxy"}, - GRPCPort: 9999, + Name: "xds-addr-config", + Flags: []string{"-proxy-id", "test-proxy"}, + 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 { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "/agent/services") { - testMockAgentGatewayConfig(namespacesEnabled)(w, r) - return +func testMockAgent(tc generateConfigTestCase) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.Path, "/agent/services"): + testMockAgentGatewayConfig(tc.NamespacesEnabled)(w, r) + case strings.Contains(r.URL.Path, "/agent/service"): + testMockAgentProxyConfig(tc.ProxyConfig, tc.NamespacesEnabled)(w, r) + case strings.Contains(r.URL.Path, "/agent/self"): + testMockAgentSelf(tc.XDSPort, tc.AgentSelf110)(w, r) + default: + http.NotFound(w, r) } - - if strings.Contains(r.URL.Path, "/agent/service") { - testMockAgentProxyConfig(agentCfg, namespacesEnabled)(w, r) - return - } - - 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. -func testMockAgentSelf(wantGRPCPort int) http.HandlerFunc { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { +// port is filled in to match the given wantXDSPort argument. +func testMockAgentSelf(wantXDSPort int, agentSelf110 bool) http.HandlerFunc { + 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) - }) + } } diff --git a/command/connect/envoy/testdata/grpc-addr-config.golden b/command/connect/envoy/testdata/deprecated-grpc-addr-config.golden similarity index 100% rename from command/connect/envoy/testdata/grpc-addr-config.golden rename to command/connect/envoy/testdata/deprecated-grpc-addr-config.golden diff --git a/command/connect/envoy/testdata/xds-addr-config.golden b/command/connect/envoy/testdata/xds-addr-config.golden new file mode 100644 index 0000000000..7e50ee3082 --- /dev/null +++ b/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" + } + } + } + } +} +