cli: envoy command default gRPC port (#4768)

* Default gRPC port; Start on some basic tests for argument and ENV handling; Make Exec test less platform-dependent.

* Allow hot-restarts

* Remove debug

command/connect/envoy/bootstrap_tpl.go

@@ -2,8 +2,8 @@ package envoy
 
 type templateArgs struct {
 	ProxyCluster, ProxyID string
-	AgentHTTPAddress      string
-	AgentHTTPPort         string
+	AgentAddress          string
+	AgentPort             string
 	AgentTLS              bool
 	AgentCAFile           string
 	AdminBindAddress      string
@@ -12,8 +12,7 @@ type templateArgs struct {
 	Token                 string
 }
 
-const bootstrapTemplate = `
-{
+const bootstrapTemplate = `{
   "admin": {
     "access_log_path": "/dev/null",
     "address": {
@@ -33,7 +32,7 @@ const bootstrapTemplate = `
         "name": "{{ .LocalAgentClusterName }}",
         "connect_timeout": "1s",
         "type": "STATIC",
-        {{ if .AgentTLS -}}
+        {{- if .AgentTLS -}}
         "tls_context": {
           "common_tls_context": {
             "validation_context": {
@@ -48,8 +47,8 @@ const bootstrapTemplate = `
         "hosts": [
           {
             "socket_address": {
-              "address": "{{ .AgentHTTPAddress }}",
-              "port_value": {{ .AgentHTTPPort }}
+              "address": "{{ .AgentAddress }}",
+              "port_value": {{ .AgentPort }}
             }
           }
         ]

command/connect/envoy/envoy.go

@@ -100,8 +100,9 @@ func (c *cmd) Run(args []string) int {
 		c.grpcAddr = os.Getenv(api.GRPCAddrEnvName)
 	}
 	if c.grpcAddr == "" {
-		c.UI.Error("Either -grpc-addr or CONSUL_GRPC_ADDR must be specified")
-		return 1
+		// This is the dev mode default and recommended production setting if
+		// enabled.
+		c.grpcAddr = "localhost:8502"
 	}
 	if c.http.Token() == "" {
 		// Extra check needed since CONSUL_HTTP_TOKEN has not been consulted yet but
@@ -178,11 +179,7 @@ func (c *cmd) findBinary() (string, error) {
 	return exec.LookPath("envoy")
 }
 
-// TODO(banks) this method ended up with a few subtleties that should be unit
-// tested.
-func (c *cmd) generateConfig() ([]byte, error) {
-	var t = template.Must(template.New("bootstrap").Parse(bootstrapTemplate))
-
+func (c *cmd) templateArgs() (*templateArgs, error) {
 	httpCfg := api.DefaultConfig()
 	c.http.MergeOntoConfig(httpCfg)
 
@@ -238,19 +235,26 @@ func (c *cmd) generateConfig() ([]byte, error) {
 		return nil, fmt.Errorf("Failed to resolve admin bind address: %s", err)
 	}
 
-	args := templateArgs{
+	return &templateArgs{
 		ProxyCluster:          c.proxyID,
 		ProxyID:               c.proxyID,
-		AgentHTTPAddress:      agentIP.String(),
-		AgentHTTPPort:         agentPort,
+		AgentAddress:          agentIP.String(),
+		AgentPort:             agentPort,
 		AgentTLS:              useTLS,
 		AgentCAFile:           httpCfg.TLSConfig.CAFile,
 		AdminBindAddress:      adminBindIP.String(),
 		AdminBindPort:         adminPort,
 		Token:                 httpCfg.Token,
 		LocalAgentClusterName: xds.LocalAgentClusterName,
-	}
+	}, nil
+}
 
+func (c *cmd) generateConfig() ([]byte, error) {
+	args, err := c.templateArgs()
+	if err != nil {
+		return nil, err
+	}
+	var t = template.Must(template.New("bootstrap").Parse(bootstrapTemplate))
 	var buf bytes.Buffer
 	err = t.Execute(&buf, args)
 	if err != nil {

command/connect/envoy/envoy_test.go

@@ -1,13 +1,167 @@
 package envoy
 
 import (
+	"flag"
+	"io/ioutil"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
+
+	"github.com/hashicorp/consul/agent/xds"
+	"github.com/mitchellh/cli"
+	"github.com/stretchr/testify/require"
 )
 
+var update = flag.Bool("update", false, "update golden files")
+
 func TestCatalogCommand_noTabs(t *testing.T) {
 	t.Parallel()
 	if strings.ContainsRune(New(nil).Help(), '\t') {
 		t.Fatal("help has tabs")
 	}
 }
+
+// testSetAndResetEnv sets the env vars passed as KEY=value strings in the
+// current ENV and returns a func() that will undo it's work at the end of the
+// test for use with defer.
+func testSetAndResetEnv(t *testing.T, env []string) func() {
+	old := make(map[string]*string)
+	for _, e := range env {
+		pair := strings.SplitN(e, "=", 2)
+		current := os.Getenv(pair[0])
+		if current != "" {
+			old[pair[0]] = &current
+		} else {
+			// save it as a nil so we know to remove again
+			old[pair[0]] = nil
+		}
+		os.Setenv(pair[0], pair[1])
+	}
+	// Return a func that will reset to old values
+	return func() {
+		for k, v := range old {
+			if v == nil {
+				os.Unsetenv(k)
+			} else {
+				os.Setenv(k, *v)
+			}
+		}
+	}
+}
+
+// This tests the args we use to generate the template directly because they
+// encapsulate all the argument and default handling code which is where most of
+// the logic is. We also allow generating golden files but only for cases that
+// pass the test of having their template args generated as expected.
+func TestGenerateConfig(t *testing.T) {
+	cases := []struct {
+		Name     string
+		Flags    []string
+		Env      []string
+		WantArgs templateArgs
+		WantErr  string
+	}{
+		{
+			Name:    "no-args",
+			Flags:   []string{},
+			Env:     []string{},
+			WantErr: "No proxy ID specified",
+		},
+		{
+			Name:  "defaults",
+			Flags: []string{"-proxy-id", "test-proxy"},
+			Env:   []string{},
+			WantArgs: templateArgs{
+				ProxyCluster:          "test-proxy",
+				ProxyID:               "test-proxy",
+				AgentAddress:          "127.0.0.1",
+				AgentPort:             "8502", // Note this is the gRPC port
+				AdminBindAddress:      "127.0.0.1",
+				AdminBindPort:         "19000",
+				LocalAgentClusterName: xds.LocalAgentClusterName,
+			},
+		},
+		{
+			Name: "grpc-addr-flag",
+			Flags: []string{"-proxy-id", "test-proxy",
+				"-grpc-addr", "localhost:9999"},
+			Env: []string{},
+			WantArgs: templateArgs{
+				ProxyCluster: "test-proxy",
+				ProxyID:      "test-proxy",
+				// 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.
+				AgentAddress:          "127.0.0.1",
+				AgentPort:             "9999",
+				AdminBindAddress:      "127.0.0.1",
+				AdminBindPort:         "19000",
+				LocalAgentClusterName: xds.LocalAgentClusterName,
+			},
+		},
+		{
+			Name:  "grpc-addr-env",
+			Flags: []string{"-proxy-id", "test-proxy"},
+			Env: []string{
+				"CONSUL_GRPC_ADDR=localhost:9999",
+			},
+			WantArgs: templateArgs{
+				ProxyCluster: "test-proxy",
+				ProxyID:      "test-proxy",
+				// 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.
+				AgentAddress:          "127.0.0.1",
+				AgentPort:             "9999",
+				AdminBindAddress:      "127.0.0.1",
+				AdminBindPort:         "19000",
+				LocalAgentClusterName: xds.LocalAgentClusterName,
+			},
+		},
+		// TODO(banks): all the flags/env manipulation cases
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.Name, func(t *testing.T) {
+			require := require.New(t)
+
+			ui := cli.NewMockUi()
+			c := New(ui)
+
+			defer testSetAndResetEnv(t, tc.Env)()
+
+			// Run the command
+			args := append([]string{"-bootstrap"}, tc.Flags...)
+			code := c.Run(args)
+			if tc.WantErr == "" {
+				require.Equal(0, code, ui.ErrorWriter.String())
+			} else {
+				require.Equal(1, code, ui.ErrorWriter.String())
+				require.Contains(ui.ErrorWriter.String(), tc.WantErr)
+				return
+			}
+
+			// Verify we handled the env and flags right first to get correct template
+			// args.
+			got, err := c.templateArgs()
+			require.NoError(err) // Error cases should have returned above
+			require.Equal(&tc.WantArgs, got)
+
+			// Actual template output goes to stdout direct to avoid prefix in UI, so
+			// generate it again here to assert on.
+			actual, err := c.generateConfig()
+			require.NoError(err)
+
+			// If we got the arg handling write, verify output
+			golden := filepath.Join("testdata", tc.Name+".golden")
+			if *update {
+				ioutil.WriteFile(golden, actual, 0644)
+			}
+
+			expected, err := ioutil.ReadFile(golden)
+			require.NoError(err)
+			require.Equal(string(expected), string(actual))
+		})
+	}
+}

command/connect/envoy/exec_test.go

@@ -8,6 +8,7 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"strings"
 	"testing"
 	"time"
 
@@ -15,33 +16,124 @@ import (
 )
 
 func TestExecEnvoy(t *testing.T) {
-	require := require.New(t)
-
-	cmd, destroy := helperProcess("exec-fake-envoy")
-	defer destroy()
-
-	cmd.Stderr = os.Stderr
-	outBytes, err := cmd.Output()
-	if err != nil {
-		t.Fatalf("error launching child process: %v", err)
+	cases := []struct {
+		Name     string
+		Args     []string
+		WantArgs []string
+	}{
+		{
+			Name: "default",
+			Args: []string{},
+			WantArgs: []string{
+				"--v2-config-only",
+				"--config-path",
+				// Different platforms produce different file descriptors here so we use the
+				// value we got back. This is somewhat tautological but we do sanity check
+				// that value further below.
+				"{{ got.ConfigPath }}",
+				"--disable-hot-restart",
+				"--fake-envoy-arg",
+			},
+		},
+		{
+			Name: "hot-restart-epoch",
+			Args: []string{"--restart-epoch", "1"},
+			WantArgs: []string{
+				"--v2-config-only",
+				"--config-path",
+				// Different platforms produce different file descriptors here so we use the
+				// value we got back. This is somewhat tautological but we do sanity check
+				// that value further below.
+				"{{ got.ConfigPath }}",
+				// No --disable-hot-restart
+				"--fake-envoy-arg",
+				"--restart-epoch",
+				"1",
+			},
+		},
+		{
+			Name: "hot-restart-version",
+			Args: []string{"--drain-time-s", "10"},
+			WantArgs: []string{
+				"--v2-config-only",
+				"--config-path",
+				// Different platforms produce different file descriptors here so we use the
+				// value we got back. This is somewhat tautological but we do sanity check
+				// that value further below.
+				"{{ got.ConfigPath }}",
+				// No --disable-hot-restart
+				"--fake-envoy-arg",
+				// Restart epoch defaults to 0 if not given and not disabled.
+				"--drain-time-s",
+				"10",
+			},
+		},
+		{
+			Name: "hot-restart-version",
+			Args: []string{"--parent-shutdown-time-s", "20"},
+			WantArgs: []string{
+				"--v2-config-only",
+				"--config-path",
+				// Different platforms produce different file descriptors here so we use the
+				// value we got back. This is somewhat tautological but we do sanity check
+				// that value further below.
+				"{{ got.ConfigPath }}",
+				// No --disable-hot-restart
+				"--fake-envoy-arg",
+				// Restart epoch defaults to 0 if not given and not disabled.
+				"--parent-shutdown-time-s",
+				"20",
+			},
+		},
+		{
+			Name: "hot-restart-version",
+			Args: []string{"--hot-restart-version", "foobar1"},
+			WantArgs: []string{
+				"--v2-config-only",
+				"--config-path",
+				// Different platforms produce different file descriptors here so we use the
+				// value we got back. This is somewhat tautological but we do sanity check
+				// that value further below.
+				"{{ got.ConfigPath }}",
+				// No --disable-hot-restart
+				"--fake-envoy-arg",
+				// Restart epoch defaults to 0 if not given and not disabled.
+				"--hot-restart-version",
+				"foobar1",
+			},
+		},
 	}
 
-	var got FakeEnvoyExecData
-	require.NoError(json.Unmarshal(outBytes, &got))
+	for _, tc := range cases {
+		t.Run(tc.Name, func(t *testing.T) {
+			require := require.New(t)
 
-	expectArgs := []string{
-		"--v2-config-only",
-		"--disable-hot-restart",
-		"--config-path",
-		"/dev/fd/3",
-		"--fake-envoy-arg",
+			args := append([]string{"exec-fake-envoy"}, tc.Args...)
+			cmd, destroy := helperProcess(args...)
+			defer destroy()
+
+			cmd.Stderr = os.Stderr
+			outBytes, err := cmd.Output()
+			require.NoError(err)
+
+			var got FakeEnvoyExecData
+			require.NoError(json.Unmarshal(outBytes, &got))
+
+			expectConfigData := fakeEnvoyTestData
+
+			// Substitute the right FD path
+			for idx := range tc.WantArgs {
+				tc.WantArgs[idx] = strings.Replace(tc.WantArgs[idx],
+					"{{ got.ConfigPath }}", got.ConfigPath, 1)
+			}
+
+			require.Equal(tc.WantArgs, got.Args)
+			require.Equal(expectConfigData, got.ConfigData)
+			// Sanity check the config path in a non-brittle way since we used it to
+			// generate expectation for the args.
+			require.Regexp(`^/dev/fd/\d+$`, got.ConfigPath)
+		})
 	}
-	expectConfigPath := "/dev/fd/3"
-	expectConfigData := fakeEnvoyTestData
-
-	require.Equal(expectArgs, got.Args)
-	require.Equal(expectConfigPath, got.ConfigPath)
-	require.Equal(expectConfigData, got.ConfigData)
 }
 
 type FakeEnvoyExecData struct {
@@ -109,7 +201,7 @@ func TestHelperProcess(t *testing.T) {
 				helperProcessSentinel,
 				"fake-envoy",
 			},
-			[]string{"--fake-envoy-arg"},
+			append([]string{"--fake-envoy-arg"}, args...),
 			[]byte(fakeEnvoyTestData),
 		)
 		if err != nil {

command/connect/envoy/exec_unix.go

@@ -9,10 +9,40 @@ import (
 	"os"
 	"path/filepath"
 	"strconv"
+	"strings"
 
 	"golang.org/x/sys/unix"
 )
 
+func isHotRestartOption(s string) bool {
+	restartOpts := []string{
+		"--restart-epoch",
+		"--hot-restart-version",
+		"--drain-time-s",
+		"--parent-shutdown-time-s",
+	}
+	for _, opt := range restartOpts {
+		if s == opt {
+			return true
+		}
+		if strings.HasPrefix(s, opt+"=") {
+			return true
+		}
+	}
+	return false
+}
+
+func hasHotRestartOption(argSets ...[]string) bool {
+	for _, args := range argSets {
+		for _, opt := range args {
+			if isHotRestartOption(opt) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 func execEnvoy(binary string, prefixArgs, suffixArgs []string, bootstrapJson []byte) error {
 	// Write the Envoy bootstrap config file out to disk in a pocket universe
 	// visible only to the current process (and exec'd future selves).
@@ -34,14 +64,23 @@ func execEnvoy(binary string, prefixArgs, suffixArgs []string, bootstrapJson []b
 	// /dev/fd/$FDNUMBER.
 	magicPath := filepath.Join("/dev/fd", strconv.Itoa(int(fd)))
 
+	// We default to disabling hot restart because it makes it easier to run
+	// multiple envoys locally for testing without them trying to share memory and
+	// unix sockets and complain about being different IDs. But if user is
+	// actually configuring hot-restart explicitly with the --restart-epoch option
+	// then don't disable it!
+	disableHotRestart := !hasHotRestartOption(prefixArgs, suffixArgs)
+
 	// First argument needs to be the executable name.
 	envoyArgs := []string{binary}
 	envoyArgs = append(envoyArgs, prefixArgs...)
 	envoyArgs = append(envoyArgs, "--v2-config-only",
-		"--disable-hot-restart",
 		"--config-path",
 		magicPath,
 	)
+	if disableHotRestart {
+		envoyArgs = append(envoyArgs, "--disable-hot-restart")
+	}
 	envoyArgs = append(envoyArgs, suffixArgs...)
 
 	// Exec

command/connect/envoy/testdata/defaults.golden

@@ -0,0 +1,51 @@
+{
+  "admin": {
+    "access_log_path": "/dev/null",
+    "address": {
+      "socket_address": {
+        "address": "127.0.0.1",
+        "port_value": 19000
+      }
+    }
+  },
+  "node": {
+    "cluster": "test-proxy",
+    "id": "test-proxy"
+  },
+  "static_resources": {
+    "clusters": [
+      {
+        "name": "local_agent",
+        "connect_timeout": "1s",
+        "type": "STATIC",
+        "http2_protocol_options": {},
+        "hosts": [
+          {
+            "socket_address": {
+              "address": "127.0.0.1",
+              "port_value": 8502
+            }
+          }
+        ]
+      }
+    ]
+  },
+  "dynamic_resources": {
+    "lds_config": { "ads": {} },
+    "cds_config": { "ads": {} },
+    "ads_config": {
+      "api_type": "GRPC",
+      "grpc_services": {
+        "initial_metadata": [
+          {
+            "key": "x-consul-token",
+            "value": ""
+          }
+        ],
+        "envoy_grpc": {
+          "cluster_name": "local_agent"
+        }
+      }
+    }
+  }
+}

command/connect/envoy/testdata/grpc-addr-env.golden

@@ -0,0 +1,51 @@
+{
+  "admin": {
+    "access_log_path": "/dev/null",
+    "address": {
+      "socket_address": {
+        "address": "127.0.0.1",
+        "port_value": 19000
+      }
+    }
+  },
+  "node": {
+    "cluster": "test-proxy",
+    "id": "test-proxy"
+  },
+  "static_resources": {
+    "clusters": [
+      {
+        "name": "local_agent",
+        "connect_timeout": "1s",
+        "type": "STATIC",
+        "http2_protocol_options": {},
+        "hosts": [
+          {
+            "socket_address": {
+              "address": "127.0.0.1",
+              "port_value": 9999
+            }
+          }
+        ]
+      }
+    ]
+  },
+  "dynamic_resources": {
+    "lds_config": { "ads": {} },
+    "cds_config": { "ads": {} },
+    "ads_config": {
+      "api_type": "GRPC",
+      "grpc_services": {
+        "initial_metadata": [
+          {
+            "key": "x-consul-token",
+            "value": ""
+          }
+        ],
+        "envoy_grpc": {
+          "cluster_name": "local_agent"
+        }
+      }
+    }
+  }
+}

command/connect/envoy/testdata/grpc-addr-flag.golden

@@ -0,0 +1,51 @@
+{
+  "admin": {
+    "access_log_path": "/dev/null",
+    "address": {
+      "socket_address": {
+        "address": "127.0.0.1",
+        "port_value": 19000
+      }
+    }
+  },
+  "node": {
+    "cluster": "test-proxy",
+    "id": "test-proxy"
+  },
+  "static_resources": {
+    "clusters": [
+      {
+        "name": "local_agent",
+        "connect_timeout": "1s",
+        "type": "STATIC",
+        "http2_protocol_options": {},
+        "hosts": [
+          {
+            "socket_address": {
+              "address": "127.0.0.1",
+              "port_value": 9999
+            }
+          }
+        ]
+      }
+    ]
+  },
+  "dynamic_resources": {
+    "lds_config": { "ads": {} },
+    "cds_config": { "ads": {} },
+    "ads_config": {
+      "api_type": "GRPC",
+      "grpc_services": {
+        "initial_metadata": [
+          {
+            "key": "x-consul-token",
+            "value": ""
+          }
+        ],
+        "envoy_grpc": {
+          "cluster_name": "local_agent"
+        }
+      }
+    }
+  }
+}

command/connect/proxy/proxy_test.go

@@ -112,7 +112,7 @@ func TestCommandConfigWatcher(t *testing.T) {
 		t.Run(tc.Name, func(t *testing.T) {
 			require := require.New(t)
 
-			// Registere a few services with 0, 1 and 2 sidecars
+			// Register a few services with 0, 1 and 2 sidecars
 			a := agent.NewTestAgent(t.Name(), `
 			services {
 				name = "no-sidecar"