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..5ee29d147c 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,37 @@ 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 + // 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 + } + 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 71d0969f1f..2685a6bb6a 100644 --- a/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go +++ b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go @@ -1,13 +1,14 @@ package pipebootstrap import ( + "bytes" "flag" - "io" "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,20 +28,24 @@ func (c *cmd) init() { } func (c *cmd) Run(args []string) int { + // 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 - if len(args) > 0 { - os.RemoveAll(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") + var buf bytes.Buffer + if _, err := buf.ReadFrom(os.Stdin); err != nil { + c.UI.Error(err.Error()) return 1 } @@ -54,23 +59,22 @@ 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 } - err = f.Close() - if err != nil { + if err = f.Close(); err != nil { c.UI.Error(err.Error()) 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.Output("Bootstrap sent, unlinking named pipe") - os.RemoveAll(args[0]) return 0 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") }