Follow-up fixes to consul connect envoy command (#16530)

This commit is contained in:
Chris S. Kim 2023-03-06 10:32:06 -05:00 committed by GitHub
parent 7f6f12089f
commit 39dc305143
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 46 additions and 20 deletions

7
.changelog/16530.txt Normal file
View File

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

View File

@ -11,7 +11,6 @@ import (
"strings" "strings"
"time" "time"
"github.com/hashicorp/consul/api"
"github.com/hashicorp/go-hclog" "github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-version" "github.com/hashicorp/go-version"
"github.com/mitchellh/cli" "github.com/mitchellh/cli"
@ -22,6 +21,7 @@ import (
"github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/agent/xds" "github.com/hashicorp/consul/agent/xds"
"github.com/hashicorp/consul/agent/xds/accesslogs" "github.com/hashicorp/consul/agent/xds/accesslogs"
"github.com/hashicorp/consul/api"
proxyCmd "github.com/hashicorp/consul/command/connect/proxy" proxyCmd "github.com/hashicorp/consul/command/connect/proxy"
"github.com/hashicorp/consul/command/flags" "github.com/hashicorp/consul/command/flags"
"github.com/hashicorp/consul/envoyextensions/xdscommon" "github.com/hashicorp/consul/envoyextensions/xdscommon"
@ -810,8 +810,7 @@ func (c *cmd) xdsAddress() (GRPC, error) {
port, protocol, err := c.lookupXDSPort() port, protocol, err := c.lookupXDSPort()
if err != nil { if err != nil {
if strings.Contains(err.Error(), "Permission denied") { if strings.Contains(err.Error(), "Permission denied") {
// Token did not have agent:read. Log and proceed with defaults. // Token did not have agent:read. Suppress and proceed with defaults.
c.UI.Info(fmt.Sprintf("Could not query /v1/agent/self for xDS ports: %s", err))
} else { } else {
// If not a permission denied error, gRPC is explicitly disabled // If not a permission denied error, gRPC is explicitly disabled
// or something went fatally wrong. // 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 // This is the dev mode default and recommended production setting if
// enabled. // enabled.
port = 8502 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) addr = fmt.Sprintf("%vlocalhost:%v", protocol, port)
} }
@ -887,9 +886,12 @@ func (c *cmd) lookupXDSPort() (int, string, error) {
var resp response var resp response
if err := mapstructure.Decode(self, &resp); err == nil { if err := mapstructure.Decode(self, &resp); err == nil {
if resp.XDS.Ports.TLS < 0 && resp.XDS.Ports.Plaintext < 0 { // When we get rid of the 1.10 compatibility code below we can uncomment
return 0, "", fmt.Errorf("agent has grpc disabled") // 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 { if resp.XDS.Ports.TLS > 0 {
return resp.XDS.Ports.TLS, "https://", nil 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 // If above TLS and Plaintext ports are both 0, it could mean
// old API for the case where a new consul CLI is being used // gRPC is disabled on the agent or we are using an older API.
// with an older API version. // 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"] cfg, ok := self["DebugConfig"]
if !ok { if !ok {
return 0, "", fmt.Errorf("unexpected agent response: no debug config") 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") 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 return int(portN), "", nil
} }

View File

@ -13,7 +13,6 @@ import (
"strings" "strings"
"testing" "testing"
"github.com/hashicorp/consul/envoyextensions/xdscommon"
"github.com/mitchellh/cli" "github.com/mitchellh/cli"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
@ -22,6 +21,7 @@ import (
"github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/agent"
"github.com/hashicorp/consul/agent/xds" "github.com/hashicorp/consul/agent/xds"
"github.com/hashicorp/consul/api" "github.com/hashicorp/consul/api"
"github.com/hashicorp/consul/envoyextensions/xdscommon"
"github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil"
) )
@ -123,6 +123,7 @@ type generateConfigTestCase struct {
NamespacesEnabled bool NamespacesEnabled bool
XDSPorts agent.GRPCPorts // used to mock an agent's configured gRPC ports. Plaintext defaults to 8502 and TLS defaults to 8503. 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 AgentSelf110 bool // fake the agent API from versions v1.10 and earlier
GRPCDisabled bool
WantArgs BootstrapTplArgs WantArgs BootstrapTplArgs
WantErr string WantErr string
WantWarn string WantWarn string
@ -146,13 +147,10 @@ func TestGenerateConfig(t *testing.T) {
WantErr: "'-node-name' requires '-proxy-id'", WantErr: "'-node-name' requires '-proxy-id'",
}, },
{ {
Name: "gRPC disabled", Name: "gRPC disabled",
Flags: []string{"-proxy-id", "test-proxy"}, Flags: []string{"-proxy-id", "test-proxy"},
XDSPorts: agent.GRPCPorts{ GRPCDisabled: true,
Plaintext: -1, WantErr: "agent has grpc disabled",
TLS: -1,
},
WantErr: "agent has grpc disabled",
}, },
{ {
Name: "defaults", Name: "defaults",
@ -1387,7 +1385,7 @@ func testMockAgent(tc generateConfigTestCase) http.HandlerFunc {
case strings.Contains(r.URL.Path, "/agent/service"): case strings.Contains(r.URL.Path, "/agent/service"):
testMockAgentProxyConfig(tc.ProxyConfig, tc.NamespacesEnabled)(w, r) testMockAgentProxyConfig(tc.ProxyConfig, tc.NamespacesEnabled)(w, r)
case strings.Contains(r.URL.Path, "/agent/self"): 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"): case strings.Contains(r.URL.Path, "/catalog/node-services"):
testMockCatalogNodeServiceList()(w, r) testMockCatalogNodeServiceList()(w, r)
case strings.Contains(r.URL.Path, "/config/proxy-defaults/global"): 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 // testMockAgentSelf returns an empty /v1/agent/self response except GRPC
// port is filled in to match the given wantXDSPort argument. // 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) { return func(w http.ResponseWriter, r *http.Request) {
resp := agent.Self{ resp := agent.Self{
Config: map[string]interface{}{ Config: map[string]interface{}{
@ -1670,6 +1672,12 @@ func testMockAgentSelf(wantXDSPorts agent.GRPCPorts, agentSelf110 bool) http.Han
resp.DebugConfig = map[string]interface{}{ resp.DebugConfig = map[string]interface{}{
"GRPCPort": wantXDSPorts.Plaintext, "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 { } else {
resp.XDS = &agent.XDSSelf{ resp.XDS = &agent.XDSSelf{
// The deprecated Port field should default to TLS if it's available. // The deprecated Port field should default to TLS if it's available.