Merge pull request #10324 from hashicorp/dnephin/fix-envoy-bootstrap-exec

envoy: fix deadlock when input is larger than named pipe buffer size

.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
+		patchExecArgs(t)
-		// 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"
-
 		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())
+}

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.
+	binary, args, err := execArgs("connect", "envoy", "pipe-bootstrap", pipeFile)
-	execPath, err := os.Executable()
 	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,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
+	var buf bytes.Buffer
-	if len(args) != 1 {
+	if _, err := buf.ReadFrom(os.Stdin); err != nil {
-		c.UI.Error("Expecting named pipe path as argument")
+		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 := buf.WriteTo(f); err != nil {
-	if err != nil {
 		c.UI.Error(err.Error())
 		return 1
 	}
 
-	err = f.Close()
+	if err = f.Close(); err != nil {
-	if 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

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")
 	}