From e1b1ab7ef6c3f4fb9db10d9e04fc33b7cd9edac0 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 31 May 2021 17:37:41 -0400 Subject: [PATCH 1/3] envoy: start timeout func after validation This removes the need to check arg length in the timeout function. --- .../connect_envoy_pipe-bootstrap.go | 26 +++++++++---------- .../connect_envoy_pipe-bootstrap_test.go | 1 - 2 files changed, 12 insertions(+), 15 deletions(-) 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 71d0969f1f..d9b816f69f 100644 --- a/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go +++ b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go @@ -6,8 +6,9 @@ import ( "os" "time" - "github.com/hashicorp/consul/command/flags" "github.com/mitchellh/cli" + + "github.com/hashicorp/consul/command/flags" ) func New(ui cli.Ui) *cmd { @@ -27,23 +28,21 @@ func (c *cmd) init() { } func (c *cmd) Run(args []string) int { - // This should never be alive for very long. In case bad things happen and - // Envoy never starts limit how long we live before just exiting so we can't - // accumulate tons of these zombie children. - time.AfterFunc(10*time.Second, func() { - // Force cleanup - if len(args) > 0 { - os.RemoveAll(args[0]) - } - os.Exit(99) - }) - // Read from STDIN, write to the named pipe provided in the only positional arg if len(args) != 1 { c.UI.Error("Expecting named pipe path as argument") return 1 } + // This should never be alive for very long. In case bad things happen and + // Envoy never starts limit how long we live before just exiting so we can't + // accumulate tons of these zombie children. + time.AfterFunc(10*time.Second, func() { + // Force cleanup + os.RemoveAll(args[0]) + os.Exit(99) + }) + // 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 @@ -60,8 +59,7 @@ func (c *cmd) Run(args []string) int { return 1 } - err = f.Close() - if err != nil { + if err = f.Close(); err != nil { c.UI.Error(err.Error()) return 1 } diff --git a/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap_test.go b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap_test.go index 8f6ee44516..76ed3157c9 100644 --- a/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap_test.go +++ b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap_test.go @@ -8,7 +8,6 @@ import ( ) func TestConnectEnvoyPipeBootstrapCommand_noTabs(t *testing.T) { - t.Parallel() if strings.ContainsRune(New(cli.NewMockUi()).Help(), '\t') { t.Fatal("help has tabs") } From c9bc5f92b7ea25635438526159933991203a3e9d Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 31 May 2021 18:10:58 -0400 Subject: [PATCH 2/3] 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 0000000000..3498748fd0 --- /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 8de39da507..1c08ff7f84 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 0914d36659..e64a0098ae 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 d9b816f69f..25ef6ad3d9 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]) From 2054402a5390d912fb8250be45473c90a5071b42 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 1 Jun 2021 11:35:32 -0400 Subject: [PATCH 3/3] envoy: improve comments --- command/connect/envoy/exec_test.go | 3 +++ .../envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go | 5 +++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/command/connect/envoy/exec_test.go b/command/connect/envoy/exec_test.go index 1c08ff7f84..5ee29d147c 100644 --- a/command/connect/envoy/exec_test.go +++ b/command/connect/envoy/exec_test.go @@ -267,6 +267,9 @@ func limitProcessLifetime(dur time.Duration) { // Also sets up a cleanup function to revert the patch when the test exits. func patchExecArgs(t *testing.T) { orig := execArgs + // go run will run the consul source from the root of the repo. The relative + // path is necessary because `go test` always sets the working directory to + // the directory of the package being tested. execArgs = func(args ...string) (string, []string, error) { args = append([]string{"run", "../../.."}, args...) return "go", args, nil 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 25ef6ad3d9..2685a6bb6a 100644 --- a/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go +++ b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go @@ -69,11 +69,12 @@ func (c *cmd) Run(args []string) int { return 1 } + // Use Warn to send to stderr, because all logs should go to stderr. + c.UI.Warn("Bootstrap sent, unlinking named pipe") + // 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.Warn("Bootstrap sent, unlinking named pipe") - os.RemoveAll(args[0]) return 0