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.
This commit is contained in:
Daniel Nephin 2021-05-31 18:10:58 -04:00
parent 177a504e9f
commit 0a39ba2c54
4 changed files with 71 additions and 26 deletions

3
.changelog/10324.txt Normal file
View File

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

View File

@ -3,8 +3,10 @@
package envoy package envoy
import ( import (
"bytes"
"encoding/json" "encoding/json"
"fmt" "fmt"
"io"
"io/ioutil" "io/ioutil"
"os" "os"
"os/exec" "os/exec"
@ -189,11 +191,7 @@ func TestHelperProcess(t *testing.T) {
limitProcessLifetime(2 * time.Minute) 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( err := execEnvoy(
os.Args[0], os.Args[0],
[]string{ []string{
@ -264,3 +262,34 @@ func limitProcessLifetime(dur time.Duration) {
os.Exit(99) 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())
}

View File

@ -49,6 +49,22 @@ func hasHotRestartOption(argSets ...[]string) bool {
return false 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) { func makeBootstrapPipe(bootstrapJSON []byte) (string, error) {
pipeFile := filepath.Join(os.TempDir(), pipeFile := filepath.Join(os.TempDir(),
fmt.Sprintf("envoy-%x-bootstrap.json", time.Now().UnixNano()+int64(os.Getpid()))) 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 return pipeFile, err
} }
// Get our own executable path. binary, args, err := execArgs("connect", "envoy", "pipe-bootstrap", pipeFile)
execPath, err := os.Executable()
if err != nil { if err != nil {
return pipeFile, err 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 // 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 // from STDIN to the named pipe (once Envoy opens it) and then clean up the
// file for us. // 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() stdin, err := cmd.StdinPipe()
if err != nil { if err != nil {
return pipeFile, err return pipeFile, err
} }
err = cmd.Start()
if err != nil {
return pipeFile, err
}
// Write the config // Write the config
n, err := stdin.Write(bootstrapJSON) n, err := stdin.Write(bootstrapJSON)
// Close STDIN whether it was successful or not // Close STDIN whether it was successful or not
stdin.Close() _ = stdin.Close()
if err != nil { if err != nil {
return pipeFile, err 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) 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 // 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 // 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 // killed then will be reaped by the init process (pid 0). This is all a bit

View File

@ -1,8 +1,8 @@
package pipebootstrap package pipebootstrap
import ( import (
"bytes"
"flag" "flag"
"io"
"os" "os"
"time" "time"
@ -43,6 +43,12 @@ func (c *cmd) Run(args []string) int {
os.Exit(99) 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 // 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 // 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 // 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 return 1
} }
_, err = io.Copy(f, os.Stdin) if _, err := buf.WriteTo(f); err != nil {
if err != nil {
c.UI.Error(err.Error()) c.UI.Error(err.Error())
return 1 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 // 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 // know it has opened it and has the file descriptor since our write above
// will block until there is a reader. // 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]) os.RemoveAll(args[0])