diff --git a/command/connect/envoy/bootstrap_tpl.go b/command/connect/envoy/bootstrap_tpl.go index 3568bbe602..9978f7f314 100644 --- a/command/connect/envoy/bootstrap_tpl.go +++ b/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 }} } } ] diff --git a/command/connect/envoy/envoy.go b/command/connect/envoy/envoy.go index f2f2b0c597..63830ca6d5 100644 --- a/command/connect/envoy/envoy.go +++ b/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 { diff --git a/command/connect/envoy/envoy_test.go b/command/connect/envoy/envoy_test.go index ed870dafb1..5ced7fd0fa 100644 --- a/command/connect/envoy/envoy_test.go +++ b/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]] = ¤t + } 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)) + }) + } +} diff --git a/command/connect/envoy/exec_test.go b/command/connect/envoy/exec_test.go index 7e50c56155..482337f5b7 100644 --- a/command/connect/envoy/exec_test.go +++ b/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 { diff --git a/command/connect/envoy/exec_unix.go b/command/connect/envoy/exec_unix.go index 2031235e64..f1b9cc1c92 100644 --- a/command/connect/envoy/exec_unix.go +++ b/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 diff --git a/command/connect/envoy/testdata/defaults.golden b/command/connect/envoy/testdata/defaults.golden new file mode 100644 index 0000000000..132be53c3b --- /dev/null +++ b/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" + } + } + } + } +} diff --git a/command/connect/envoy/testdata/grpc-addr-env.golden b/command/connect/envoy/testdata/grpc-addr-env.golden new file mode 100644 index 0000000000..f9e86c4a05 --- /dev/null +++ b/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" + } + } + } + } +} diff --git a/command/connect/envoy/testdata/grpc-addr-flag.golden b/command/connect/envoy/testdata/grpc-addr-flag.golden new file mode 100644 index 0000000000..f9e86c4a05 --- /dev/null +++ b/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" + } + } + } + } +} diff --git a/command/connect/proxy/proxy_test.go b/command/connect/proxy/proxy_test.go index e09d95f3ce..d6809a6486 100644 --- a/command/connect/proxy/proxy_test.go +++ b/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"