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

@@ -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.
+```

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())
+}

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

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])