Backport of Follow-up fixes to consul connect envoy command into rele…

…ase/1.15.x (#16542)

* backport of commit dc7f05b

* backport of commit d0adb5c

* backport of commit 097143b

* backport of commit db403de

---------

Co-authored-by: Chris S. Kim <ckim@hashicorp.com>

.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.
+```

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
 }
 

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.