diff --git a/.changelog/10324.txt b/.changelog/10324.txt new file mode 100644 index 000000000..3498748fd --- /dev/null +++ b/.changelog/10324.txt @@ -0,0 +1,3 @@ +```release-note:bug +envoy: fixes a bug where a large envoy config could cause the `consul connect envoy` command to deadlock when attempting to start envoy. +``` diff --git a/command/connect/envoy/exec_test.go b/command/connect/envoy/exec_test.go index 8de39da50..1c08ff7f8 100644 --- a/command/connect/envoy/exec_test.go +++ b/command/connect/envoy/exec_test.go @@ -3,8 +3,10 @@ package envoy import ( + "bytes" "encoding/json" "fmt" + "io" "io/ioutil" "os" "os/exec" @@ -189,11 +191,7 @@ func TestHelperProcess(t *testing.T) { limitProcessLifetime(2 * time.Minute) - // This is another level of gross - we are relying on `consul` being on path - // and being the correct version but in general that is true under `make - // test`. We already make the same assumption for API package tests. - testSelfExecOverride = "consul" - + patchExecArgs(t) err := execEnvoy( os.Args[0], []string{ @@ -264,3 +262,34 @@ func limitProcessLifetime(dur time.Duration) { os.Exit(99) }) } + +// patchExecArgs to use a version that will execute the commands using 'go run'. +// Also sets up a cleanup function to revert the patch when the test exits. +func patchExecArgs(t *testing.T) { + orig := execArgs + execArgs = func(args ...string) (string, []string, error) { + args = append([]string{"run", "../../.."}, args...) + return "go", args, nil + } + t.Cleanup(func() { + execArgs = orig + }) +} + +func TestMakeBootstrapPipe_DoesNotBlockOnAFullPipe(t *testing.T) { + // A named pipe can buffer up to 64k, use a value larger than that + bootstrap := bytes.Repeat([]byte("a"), 66000) + + patchExecArgs(t) + pipe, err := makeBootstrapPipe(bootstrap) + require.NoError(t, err) + + // Read everything from the named pipe, to allow the sub-process to exit + f, err := os.Open(pipe) + require.NoError(t, err) + + var buf bytes.Buffer + _, err = io.Copy(&buf, f) + require.NoError(t, err) + require.Equal(t, bootstrap, buf.Bytes()) +} diff --git a/command/connect/envoy/exec_unix.go b/command/connect/envoy/exec_unix.go index 0914d3665..e64a0098a 100644 --- a/command/connect/envoy/exec_unix.go +++ b/command/connect/envoy/exec_unix.go @@ -49,6 +49,22 @@ func hasHotRestartOption(argSets ...[]string) bool { return false } +// execArgs returns the command and args used to execute a binary. By default it +// will return a command of os.Executable with the args unmodified. This is a shim +// for testing, and can be overridden to execute using 'go run' instead. +var execArgs = func(args ...string) (string, []string, error) { + execPath, err := os.Executable() + if err != nil { + return "", nil, err + } + + if strings.HasSuffix(execPath, "/envoy.test") { + return "", nil, fmt.Errorf("set execArgs to use 'go run' instead of doing a self-exec") + } + + return execPath, args, nil +} + func makeBootstrapPipe(bootstrapJSON []byte) (string, error) { pipeFile := filepath.Join(os.TempDir(), fmt.Sprintf("envoy-%x-bootstrap.json", time.Now().UnixNano()+int64(os.Getpid()))) @@ -58,33 +74,30 @@ func makeBootstrapPipe(bootstrapJSON []byte) (string, error) { return pipeFile, err } - // Get our own executable path. - execPath, err := os.Executable() + binary, args, err := execArgs("connect", "envoy", "pipe-bootstrap", pipeFile) if err != nil { return pipeFile, err } - if testSelfExecOverride != "" { - execPath = testSelfExecOverride - } else if strings.HasSuffix(execPath, "/envoy.test") { - return pipeFile, fmt.Errorf("I seem to be running in a test binary without " + - "overriding the self-executable. Not doing that - it will make you sad. " + - "See testSelfExecOverride.") - } - // Exec the pipe-bootstrap internal sub-command which will write the bootstrap // from STDIN to the named pipe (once Envoy opens it) and then clean up the // file for us. - cmd := exec.Command(execPath, "connect", "envoy", "pipe-bootstrap", pipeFile) + cmd := exec.Command(binary, args...) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr stdin, err := cmd.StdinPipe() if err != nil { return pipeFile, err } + err = cmd.Start() + if err != nil { + return pipeFile, err + } // Write the config n, err := stdin.Write(bootstrapJSON) // Close STDIN whether it was successful or not - stdin.Close() + _ = stdin.Close() if err != nil { return pipeFile, err } @@ -92,11 +105,6 @@ func makeBootstrapPipe(bootstrapJSON []byte) (string, error) { return pipeFile, fmt.Errorf("failed writing boostrap to child STDIN: %s", err) } - err = cmd.Start() - if err != nil { - return pipeFile, err - } - // We can't wait for the process since we need to exec into Envoy before it // will be able to complete so it will be remain as a zombie until Envoy is // killed then will be reaped by the init process (pid 0). This is all a bit diff --git a/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go index d9b816f69..25ef6ad3d 100644 --- a/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go +++ b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go @@ -1,8 +1,8 @@ package pipebootstrap import ( + "bytes" "flag" - "io" "os" "time" @@ -43,6 +43,12 @@ func (c *cmd) Run(args []string) int { os.Exit(99) }) + var buf bytes.Buffer + if _, err := buf.ReadFrom(os.Stdin); err != nil { + c.UI.Error(err.Error()) + return 1 + } + // WRONLY is very important here - the open() call will block until there is a // reader (Envoy) if we open it with RDWR though that counts as an opener and // we will just send the data to ourselves as the first opener and so only @@ -53,8 +59,7 @@ func (c *cmd) Run(args []string) int { return 1 } - _, err = io.Copy(f, os.Stdin) - if err != nil { + if _, err := buf.WriteTo(f); err != nil { c.UI.Error(err.Error()) return 1 } @@ -67,7 +72,7 @@ func (c *cmd) Run(args []string) int { // Removed named pipe now we sent it. Even if Envoy has not yet read it, we // know it has opened it and has the file descriptor since our write above // will block until there is a reader. - c.UI.Output("Bootstrap sent, unlinking named pipe") + c.UI.Warn("Bootstrap sent, unlinking named pipe") os.RemoveAll(args[0])