From 0a39ba2c543b608adbc9040f20413ebcffd5d6fe Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 31 May 2021 18:10:58 -0400 Subject: [PATCH] envoy: fix bootstrap deadlock caused by a full named pipe Normally the named pipe would buffer up to 64k, but in some cases when a soft limit is reached, they will start only buffering up to 4k. In either case, we should not deadlock. This commit changes the pipe-bootstrap command to first buffer all of stdin into the process, before trying to write it to the named pipe. This allows the process memory to act as the buffer, instead of the named pipe. Also changed the order of operations in `makeBootstrapPipe`. The new test added in this PR showed that simply buffering in the process memory was not enough to fix the issue. We also need to ensure that the `pipe-bootstrap` process is started before we try to write to its stdin. Otherwise the write will still block. Also set stdout/stderr on the subprocess, so that any errors are visible to the user. --- .changelog/10324.txt | 3 ++ command/connect/envoy/exec_test.go | 39 ++++++++++++++--- command/connect/envoy/exec_unix.go | 42 +++++++++++-------- .../connect_envoy_pipe-bootstrap.go | 13 ++++-- 4 files changed, 71 insertions(+), 26 deletions(-) create mode 100644 .changelog/10324.txt 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])