From 39dc305143c1a8d65f97b71ed23515274cf9f52b Mon Sep 17 00:00:00 2001 From: "Chris S. Kim" Date: Mon, 6 Mar 2023 10:32:06 -0500 Subject: [PATCH] Follow-up fixes to consul connect envoy command (#16530) --- .changelog/16530.txt | 7 +++++++ command/connect/envoy/envoy.go | 31 +++++++++++++++++++---------- command/connect/envoy/envoy_test.go | 28 ++++++++++++++++---------- 3 files changed, 46 insertions(+), 20 deletions(-) create mode 100644 .changelog/16530.txt diff --git a/.changelog/16530.txt b/.changelog/16530.txt new file mode 100644 index 000000000..38d98036a --- /dev/null +++ b/.changelog/16530.txt @@ -0,0 +1,7 @@ +```release-note:bug +cli: Fixes an issue with `consul connect envoy` where a log to STDOUT could malform JSON when used with `-bootstrap`. +``` + +```release-note:bug +cli: Fixes an issue with `consul connect envoy` where grpc-disabled agents were not error-handled correctly. +``` diff --git a/command/connect/envoy/envoy.go b/command/connect/envoy/envoy.go index 102c16494..0864ecc19 100644 --- a/command/connect/envoy/envoy.go +++ b/command/connect/envoy/envoy.go @@ -11,7 +11,6 @@ import ( "strings" "time" - "github.com/hashicorp/consul/api" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-version" "github.com/mitchellh/cli" @@ -22,6 +21,7 @@ import ( "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/xds" "github.com/hashicorp/consul/agent/xds/accesslogs" + "github.com/hashicorp/consul/api" proxyCmd "github.com/hashicorp/consul/command/connect/proxy" "github.com/hashicorp/consul/command/flags" "github.com/hashicorp/consul/envoyextensions/xdscommon" @@ -810,8 +810,7 @@ func (c *cmd) xdsAddress() (GRPC, error) { port, protocol, err := c.lookupXDSPort() if err != nil { if strings.Contains(err.Error(), "Permission denied") { - // Token did not have agent:read. Log and proceed with defaults. - c.UI.Info(fmt.Sprintf("Could not query /v1/agent/self for xDS ports: %s", err)) + // Token did not have agent:read. Suppress and proceed with defaults. } else { // If not a permission denied error, gRPC is explicitly disabled // or something went fatally wrong. @@ -822,7 +821,7 @@ func (c *cmd) xdsAddress() (GRPC, error) { // This is the dev mode default and recommended production setting if // enabled. port = 8502 - c.UI.Info("-grpc-addr not provided and unable to discover a gRPC address for xDS. Defaulting to localhost:8502") + c.UI.Warn("-grpc-addr not provided and unable to discover a gRPC address for xDS. Defaulting to localhost:8502") } addr = fmt.Sprintf("%vlocalhost:%v", protocol, port) } @@ -887,9 +886,12 @@ func (c *cmd) lookupXDSPort() (int, string, error) { var resp response if err := mapstructure.Decode(self, &resp); err == nil { - if resp.XDS.Ports.TLS < 0 && resp.XDS.Ports.Plaintext < 0 { - return 0, "", fmt.Errorf("agent has grpc disabled") - } + // When we get rid of the 1.10 compatibility code below we can uncomment + // this check: + // + // if resp.XDS.Ports.TLS <= 0 && resp.XDS.Ports.Plaintext <= 0 { + // return 0, "", fmt.Errorf("agent has grpc disabled") + // } if resp.XDS.Ports.TLS > 0 { return resp.XDS.Ports.TLS, "https://", nil } @@ -898,9 +900,12 @@ func (c *cmd) lookupXDSPort() (int, string, error) { } } - // If above TLS and Plaintext ports are both 0, fallback to - // old API for the case where a new consul CLI is being used - // with an older API version. + // If above TLS and Plaintext ports are both 0, it could mean + // gRPC is disabled on the agent or we are using an older API. + // In either case, fallback to reading from the DebugConfig. + // + // Next major version we should get rid of this below code. + // It exists for compatibility reasons for 1.10 and below. cfg, ok := self["DebugConfig"] if !ok { return 0, "", fmt.Errorf("unexpected agent response: no debug config") @@ -914,6 +919,12 @@ func (c *cmd) lookupXDSPort() (int, string, error) { return 0, "", fmt.Errorf("invalid grpc port in agent response") } + // This works for both <1.10 and later but we should prefer + // reading from resp.XDS instead. + if portN < 0 { + return 0, "", fmt.Errorf("agent has grpc disabled") + } + return int(portN), "", nil } diff --git a/command/connect/envoy/envoy_test.go b/command/connect/envoy/envoy_test.go index 09c02c91c..e2692c77d 100644 --- a/command/connect/envoy/envoy_test.go +++ b/command/connect/envoy/envoy_test.go @@ -13,7 +13,6 @@ import ( "strings" "testing" - "github.com/hashicorp/consul/envoyextensions/xdscommon" "github.com/mitchellh/cli" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -22,6 +21,7 @@ import ( "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/agent/xds" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/envoyextensions/xdscommon" "github.com/hashicorp/consul/sdk/testutil" ) @@ -123,6 +123,7 @@ type generateConfigTestCase struct { NamespacesEnabled bool XDSPorts agent.GRPCPorts // used to mock an agent's configured gRPC ports. Plaintext defaults to 8502 and TLS defaults to 8503. AgentSelf110 bool // fake the agent API from versions v1.10 and earlier + GRPCDisabled bool WantArgs BootstrapTplArgs WantErr string WantWarn string @@ -146,13 +147,10 @@ func TestGenerateConfig(t *testing.T) { WantErr: "'-node-name' requires '-proxy-id'", }, { - Name: "gRPC disabled", - Flags: []string{"-proxy-id", "test-proxy"}, - XDSPorts: agent.GRPCPorts{ - Plaintext: -1, - TLS: -1, - }, - WantErr: "agent has grpc disabled", + Name: "gRPC disabled", + Flags: []string{"-proxy-id", "test-proxy"}, + GRPCDisabled: true, + WantErr: "agent has grpc disabled", }, { Name: "defaults", @@ -1387,7 +1385,7 @@ func testMockAgent(tc generateConfigTestCase) http.HandlerFunc { 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.XDSPorts, tc.AgentSelf110)(w, r) + testMockAgentSelf(tc.XDSPorts, tc.AgentSelf110, tc.GRPCDisabled)(w, r) case strings.Contains(r.URL.Path, "/catalog/node-services"): testMockCatalogNodeServiceList()(w, r) case strings.Contains(r.URL.Path, "/config/proxy-defaults/global"): @@ -1658,7 +1656,11 @@ func TestEnvoyCommand_canBindInternal(t *testing.T) { // testMockAgentSelf returns an empty /v1/agent/self response except GRPC // port is filled in to match the given wantXDSPort argument. -func testMockAgentSelf(wantXDSPorts agent.GRPCPorts, agentSelf110 bool) http.HandlerFunc { +func testMockAgentSelf( + wantXDSPorts agent.GRPCPorts, + agentSelf110 bool, + grpcDisabled bool, +) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { resp := agent.Self{ Config: map[string]interface{}{ @@ -1670,6 +1672,12 @@ func testMockAgentSelf(wantXDSPorts agent.GRPCPorts, agentSelf110 bool) http.Han resp.DebugConfig = map[string]interface{}{ "GRPCPort": wantXDSPorts.Plaintext, } + } else if grpcDisabled { + resp.DebugConfig = map[string]interface{}{ + "GRPCPort": -1, + } + // the real agent does not populate XDS if grpc or + // grpc-tls ports are < 0 } else { resp.XDS = &agent.XDSSelf{ // The deprecated Port field should default to TLS if it's available.