From 9f656a2dc8bcc8ffb735fd9ce7e764eb8dbd41da Mon Sep 17 00:00:00 2001 From: Paul Banks Date: Fri, 21 Jun 2019 16:06:25 +0100 Subject: [PATCH] Fix envoy 1.10 exec (#5964) * Make exec test assert Envoy version - it was not rebuilding before and so often ran against wrong version. This makes 1.10 fail consistenty. * Switch Envoy exec to use a named pipe rather than FD magic since Envoy 1.10 doesn't support that. * Refactor to use an internal shim command for piping the bootstrap through. * Fmt. So sad that vscode golang fails so often these days. * go mod tidy * revert go mod tidy changes * Revert "ignore consul-exec tests until fixed (#5986)" This reverts commit 683262a6869033cb79e68fa1dba0f9ea83e9187d. * Review cleanups --- command/commands_oss.go | 2 + command/connect/envoy/exec_test.go | 16 +- command/connect/envoy/exec_unix.go | 173 ++++++++---------- .../connect_envoy_pipe-bootstrap.go | 90 +++++++++ .../connect_envoy_pipe-bootstrap_test.go | 15 ++ .../connect/envoy/case-basic/verify.bats | 4 + .../setup.sh | 7 +- .../verify.bats | 7 +- .../connect/envoy/docker-compose.yml | 3 + test/integration/connect/envoy/helpers.bash | 20 ++ test/integration/connect/envoy/run-tests.sh | 4 +- 11 files changed, 228 insertions(+), 113 deletions(-) create mode 100644 command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go create mode 100644 command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap_test.go rename test/integration/connect/envoy/{xcase-consul-exec => case-consul-exec}/setup.sh (62%) rename test/integration/connect/envoy/{xcase-consul-exec => case-consul-exec}/verify.bats (80%) diff --git a/command/commands_oss.go b/command/commands_oss.go index c33ffc4271..d5d04a4402 100644 --- a/command/commands_oss.go +++ b/command/commands_oss.go @@ -51,6 +51,7 @@ import ( caget "github.com/hashicorp/consul/command/connect/ca/get" caset "github.com/hashicorp/consul/command/connect/ca/set" "github.com/hashicorp/consul/command/connect/envoy" + "github.com/hashicorp/consul/command/connect/envoy/pipe-bootstrap" "github.com/hashicorp/consul/command/connect/proxy" "github.com/hashicorp/consul/command/debug" "github.com/hashicorp/consul/command/event" @@ -167,6 +168,7 @@ func init() { Register("connect ca set-config", func(ui cli.Ui) (cli.Command, error) { return caset.New(ui), nil }) Register("connect proxy", func(ui cli.Ui) (cli.Command, error) { return proxy.New(ui, MakeShutdownCh()), nil }) Register("connect envoy", func(ui cli.Ui) (cli.Command, error) { return envoy.New(ui), nil }) + Register("connect envoy pipe-bootstrap", func(ui cli.Ui) (cli.Command, error) { return pipebootstrap.New(ui), nil }) Register("debug", func(ui cli.Ui) (cli.Command, error) { return debug.New(ui, MakeShutdownCh()), nil }) Register("event", func(ui cli.Ui) (cli.Command, error) { return event.New(ui), nil }) Register("exec", func(ui cli.Ui) (cli.Command, error) { return exec.New(ui, MakeShutdownCh()), nil }) diff --git a/command/connect/envoy/exec_test.go b/command/connect/envoy/exec_test.go index 482337f5b7..25039c8f6c 100644 --- a/command/connect/envoy/exec_test.go +++ b/command/connect/envoy/exec_test.go @@ -16,6 +16,7 @@ import ( ) func TestExecEnvoy(t *testing.T) { + cases := []struct { Name string Args []string @@ -25,11 +26,7 @@ func TestExecEnvoy(t *testing.T) { Name: "default", Args: []string{}, WantArgs: []string{ - "--v2-config-only", "--config-path", - // Different platforms produce different file descriptors here so we use the - // value we got back. This is somewhat tautological but we do sanity check - // that value further below. "{{ got.ConfigPath }}", "--disable-hot-restart", "--fake-envoy-arg", @@ -39,7 +36,6 @@ func TestExecEnvoy(t *testing.T) { Name: "hot-restart-epoch", Args: []string{"--restart-epoch", "1"}, WantArgs: []string{ - "--v2-config-only", "--config-path", // Different platforms produce different file descriptors here so we use the // value we got back. This is somewhat tautological but we do sanity check @@ -55,7 +51,6 @@ func TestExecEnvoy(t *testing.T) { Name: "hot-restart-version", Args: []string{"--drain-time-s", "10"}, WantArgs: []string{ - "--v2-config-only", "--config-path", // Different platforms produce different file descriptors here so we use the // value we got back. This is somewhat tautological but we do sanity check @@ -72,7 +67,6 @@ func TestExecEnvoy(t *testing.T) { Name: "hot-restart-version", Args: []string{"--parent-shutdown-time-s", "20"}, WantArgs: []string{ - "--v2-config-only", "--config-path", // Different platforms produce different file descriptors here so we use the // value we got back. This is somewhat tautological but we do sanity check @@ -89,7 +83,6 @@ func TestExecEnvoy(t *testing.T) { Name: "hot-restart-version", Args: []string{"--hot-restart-version", "foobar1"}, WantArgs: []string{ - "--v2-config-only", "--config-path", // Different platforms produce different file descriptors here so we use the // value we got back. This is somewhat tautological but we do sanity check @@ -131,7 +124,7 @@ func TestExecEnvoy(t *testing.T) { require.Equal(expectConfigData, got.ConfigData) // Sanity check the config path in a non-brittle way since we used it to // generate expectation for the args. - require.Regexp(`^/dev/fd/\d+$`, got.ConfigPath) + require.Regexp(`-bootstrap.json$`, got.ConfigPath) }) } } @@ -193,6 +186,11 @@ 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" + err := execEnvoy( os.Args[0], []string{ diff --git a/command/connect/envoy/exec_unix.go b/command/connect/envoy/exec_unix.go index f1b9cc1c92..0914d36659 100644 --- a/command/connect/envoy/exec_unix.go +++ b/command/connect/envoy/exec_unix.go @@ -4,16 +4,22 @@ package envoy import ( "errors" - "io" - "io/ioutil" + "fmt" "os" + "os/exec" "path/filepath" - "strconv" "strings" + "syscall" + "time" "golang.org/x/sys/unix" ) +// testSelfExecOverride is a way for the tests to no fork-bomb themselves by +// self-executing the whole test suite for each case recursively. It's gross but +// the least gross option I could think of. +var testSelfExecOverride string + func isHotRestartOption(s string) bool { restartOpts := []string{ "--restart-epoch", @@ -43,26 +49,72 @@ func hasHotRestartOption(argSets ...[]string) bool { return false } -func execEnvoy(binary string, prefixArgs, suffixArgs []string, bootstrapJson []byte) error { - // Write the Envoy bootstrap config file out to disk in a pocket universe - // visible only to the current process (and exec'd future selves). - fd, err := writeEphemeralEnvoyTempFile(bootstrapJson) +func makeBootstrapPipe(bootstrapJSON []byte) (string, error) { + pipeFile := filepath.Join(os.TempDir(), + fmt.Sprintf("envoy-%x-bootstrap.json", time.Now().UnixNano()+int64(os.Getpid()))) + + err := syscall.Mkfifo(pipeFile, 0600) if err != nil { - return errors.New("Could not write envoy bootstrap config to a temp file: " + err.Error()) + return pipeFile, err } - // On unix systems after exec the file descriptors that we should see: - // - // 0: stdin - // 1: stdout - // 2: stderr - // ... any open file descriptors from the parent without CLOEXEC set - // - // Above we explicitly disabled CLOEXEC for our temp file, so assuming - // FD numbers survive across execs, it should just be the value of - // `fd`. This is accessible as a file itself (trippy!) under - // /dev/fd/$FDNUMBER. - magicPath := filepath.Join("/dev/fd", strconv.Itoa(int(fd))) + // Get our own executable path. + 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) + stdin, err := cmd.StdinPipe() + if err != nil { + return pipeFile, err + } + + // Write the config + n, err := stdin.Write(bootstrapJSON) + // Close STDIN whether it was successful or not + stdin.Close() + if err != nil { + return pipeFile, err + } + if n < len(bootstrapJSON) { + 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 + // gross but the cleanest workaround I can think of for Envoy 1.10 not + // supporting /dev/fd/ config paths any more. So we are done and leaving + // the child to run it's course without reaping it. + return pipeFile, nil +} + +func execEnvoy(binary string, prefixArgs, suffixArgs []string, bootstrapJSON []byte) error { + pipeFile, err := makeBootstrapPipe(bootstrapJSON) + if err != nil { + os.RemoveAll(pipeFile) + return err + } + // We don't defer a cleanup since we are about to Exec into Envoy which means + // defer will never fire. The child process cleans up for us in the happy + // path. // We default to disabling hot restart because it makes it easier to run // multiple envoys locally for testing without them trying to share memory and @@ -74,10 +126,7 @@ func execEnvoy(binary string, prefixArgs, suffixArgs []string, bootstrapJson []b // First argument needs to be the executable name. envoyArgs := []string{binary} envoyArgs = append(envoyArgs, prefixArgs...) - envoyArgs = append(envoyArgs, "--v2-config-only", - "--config-path", - magicPath, - ) + envoyArgs = append(envoyArgs, "--config-path", pipeFile) if disableHotRestart { envoyArgs = append(envoyArgs, "--disable-hot-restart") } @@ -90,79 +139,3 @@ func execEnvoy(binary string, prefixArgs, suffixArgs []string, bootstrapJson []b return nil } - -func writeEphemeralEnvoyTempFile(b []byte) (uintptr, error) { - f, err := ioutil.TempFile("", "envoy-ephemeral-config") - if err != nil { - return 0, err - } - - errFn := func(err error) (uintptr, error) { - _ = f.Close() - return 0, err - } - - // TempFile already does this, but it's cheap to reinforce that we - // WANT the default behavior. - if err := f.Chmod(0600); err != nil { - return errFn(err) - } - - // Immediately unlink the file as we are going to just pass the - // file descriptor, not the path. - if err = os.Remove(f.Name()); err != nil { - return errFn(err) - } - if _, err = f.Write(b); err != nil { - return errFn(err) - } - // Rewind the file descriptor so Envoy can read it. - if _, err = f.Seek(0, io.SeekStart); err != nil { - return errFn(err) - } - - // Disable CLOEXEC so that this file descriptor is available - // to the exec'd Envoy. - if err := setCloseOnExec(f.Fd(), false); err != nil { - return errFn(err) - } - - return f.Fd(), nil -} - -// isCloseOnExec checks the provided file descriptor to see if the CLOEXEC flag -// is set. -func isCloseOnExec(fd uintptr) (bool, error) { - flags, err := getFdFlags(fd) - if err != nil { - return false, err - } - return flags&unix.FD_CLOEXEC != 0, nil -} - -// setCloseOnExec sets or unsets the CLOEXEC flag on the provided file descriptor -// depending upon the value of the enabled arg. -func setCloseOnExec(fd uintptr, enabled bool) error { - flags, err := getFdFlags(fd) - if err != nil { - return err - } - - newFlags := flags - if enabled { - newFlags |= unix.FD_CLOEXEC - } else { - newFlags &= ^unix.FD_CLOEXEC - } - - if newFlags == flags { - return nil // noop - } - - _, err = unix.FcntlInt(fd, unix.F_SETFD, newFlags) - return err -} - -func getFdFlags(fd uintptr) (int, error) { - return unix.FcntlInt(fd, unix.F_GETFD, 0) -} diff --git a/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go new file mode 100644 index 0000000000..71d0969f1f --- /dev/null +++ b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap.go @@ -0,0 +1,90 @@ +package pipebootstrap + +import ( + "flag" + "io" + "os" + "time" + + "github.com/hashicorp/consul/command/flags" + "github.com/mitchellh/cli" +) + +func New(ui cli.Ui) *cmd { + c := &cmd{UI: ui} + c.init() + return c +} + +type cmd struct { + UI cli.Ui + flags *flag.FlagSet + help string +} + +func (c *cmd) init() { + c.help = flags.Usage(help, c.flags) +} + +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 + } + + // 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 + // valid reader. + f, err := os.OpenFile(args[0], os.O_WRONLY|os.O_APPEND, 0700) + if err != nil { + c.UI.Error(err.Error()) + return 1 + } + + _, err = io.Copy(f, os.Stdin) + if err != nil { + c.UI.Error(err.Error()) + return 1 + } + + err = f.Close() + if err != nil { + c.UI.Error(err.Error()) + return 1 + } + + // 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 +} + +func (c *cmd) Synopsis() string { + return synopsis +} + +func (c *cmd) Help() string { + return c.help +} + +const synopsis = "Internal shim for delivering Envoy bootstrap without writing to file system" +const help = ` +Usage: should only be used internally by consul connect envoy +` 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 new file mode 100644 index 0000000000..8f6ee44516 --- /dev/null +++ b/command/connect/envoy/pipe-bootstrap/connect_envoy_pipe-bootstrap_test.go @@ -0,0 +1,15 @@ +package pipebootstrap + +import ( + "strings" + "testing" + + "github.com/mitchellh/cli" +) + +func TestConnectEnvoyPipeBootstrapCommand_noTabs(t *testing.T) { + t.Parallel() + if strings.ContainsRune(New(cli.NewMockUi()).Help(), '\t') { + t.Fatal("help has tabs") + } +} diff --git a/test/integration/connect/envoy/case-basic/verify.bats b/test/integration/connect/envoy/case-basic/verify.bats index 21dfc37d49..6e915f947d 100644 --- a/test/integration/connect/envoy/case-basic/verify.bats +++ b/test/integration/connect/envoy/case-basic/verify.bats @@ -2,6 +2,10 @@ load helpers +@test "s1 proxy is running correct version" { + assert_envoy_version 19000 +} + @test "s1 proxy admin is up on :19000" { retry_default curl -f -s localhost:19000/stats -o /dev/null } diff --git a/test/integration/connect/envoy/xcase-consul-exec/setup.sh b/test/integration/connect/envoy/case-consul-exec/setup.sh similarity index 62% rename from test/integration/connect/envoy/xcase-consul-exec/setup.sh rename to test/integration/connect/envoy/case-consul-exec/setup.sh index a3a2cd1d6c..01442b11c6 100644 --- a/test/integration/connect/envoy/xcase-consul-exec/setup.sh +++ b/test/integration/connect/envoy/case-consul-exec/setup.sh @@ -2,10 +2,15 @@ set -euo pipefail +# Force rebuild of the exec container since this doesn't happen if only the +# version argument changed which means we end up testing the wrong version of +# Envoy. +docker-compose build s1-sidecar-proxy-consul-exec + # Bring up s1 and it's proxy as well because the check that it has a cert causes # a proxy connection to be opened and having the backend not be available seems # to cause Envoy to fail non-deterministically in CI (rarely on local machine). # It might be related to this know issue # https://github.com/envoyproxy/envoy/issues/2800 where TcpProxy will error if # the backend is down sometimes part way through the handshake. -export REQUIRED_SERVICES="s1 s1-sidecar-proxy-consul-exec" \ No newline at end of file +export REQUIRED_SERVICES="s1 s1-sidecar-proxy-consul-exec" diff --git a/test/integration/connect/envoy/xcase-consul-exec/verify.bats b/test/integration/connect/envoy/case-consul-exec/verify.bats similarity index 80% rename from test/integration/connect/envoy/xcase-consul-exec/verify.bats rename to test/integration/connect/envoy/case-consul-exec/verify.bats index 94b98bc1dc..d37240edfe 100644 --- a/test/integration/connect/envoy/xcase-consul-exec/verify.bats +++ b/test/integration/connect/envoy/case-consul-exec/verify.bats @@ -11,4 +11,9 @@ load helpers @test "s1 proxy listener should be up and have right cert" { assert_proxy_presents_cert_uri localhost:21000 s1 -} \ No newline at end of file +} + +@test "s1 proxy is running correct version" { + assert_envoy_version 19000 +} + diff --git a/test/integration/connect/envoy/docker-compose.yml b/test/integration/connect/envoy/docker-compose.yml index bdc6d0b8e4..35d0f58634 100644 --- a/test/integration/connect/envoy/docker-compose.yml +++ b/test/integration/connect/envoy/docker-compose.yml @@ -115,6 +115,8 @@ services: context: . dockerfile: Dockerfile-bats tty: true + environment: + - ENVOY_VERSION command: - "--pretty" - "/workdir/bats" @@ -130,6 +132,7 @@ services: dockerfile: Dockerfile-consul-envoy args: ENVOY_VERSION: ${ENVOY_VERSION:-1.8.0} + image: consul-dev-envoy:${ENVOY_VERSION:-1.8.0} command: - "consul" - "connect" diff --git a/test/integration/connect/envoy/helpers.bash b/test/integration/connect/envoy/helpers.bash index 1bf27d2642..7cd1bf89ec 100755 --- a/test/integration/connect/envoy/helpers.bash +++ b/test/integration/connect/envoy/helpers.bash @@ -76,6 +76,26 @@ function assert_proxy_presents_cert_uri { echo "$CERT" | grep -Eo "URI:spiffe://([a-zA-Z0-9-]+).consul/ns/default/dc/dc1/svc/$SERVICENAME" } +function assert_envoy_version { + local ADMINPORT=$1 + run retry_default curl -f -s localhost:$ADMINPORT/server_info + [ "$status" -eq 0 ] + # Envoy 1.8.0 returns a plain text line like + # envoy 5d25f466c3410c0dfa735d7d4358beb76b2da507/1.8.0/Clean/DEBUG live 3 3 0 + # Later versions return JSON. + if (echo $output | grep '^envoy') ; then + VERSION=$(echo $output | cut -d ' ' -f 2) + else + VERSION=$(echo $output | jq -r '.version') + fi + echo "Status=$status" + echo "Output=$output" + echo "---" + echo "Got version=$VERSION" + echo "Want version=$ENVOY_VERSION" + echo $VERSION | grep "/$ENVOY_VERSION/" +} + function get_envoy_listener_filters { local HOSTPORT=$1 run retry_default curl -s -f $HOSTPORT/config_dump diff --git a/test/integration/connect/envoy/run-tests.sh b/test/integration/connect/envoy/run-tests.sh index 64c8f38d29..ba2f4a08bb 100755 --- a/test/integration/connect/envoy/run-tests.sh +++ b/test/integration/connect/envoy/run-tests.sh @@ -115,12 +115,12 @@ for c in ./case-*/ ; do # Start containers required if [ ! -z "$REQUIRED_SERVICES" ] ; then - docker-compose up -d $REQUIRED_SERVICES + docker-compose up --build -d $REQUIRED_SERVICES fi # Execute tests THISRESULT=1 - if docker-compose up --build --abort-on-container-exit --exit-code-from verify verify ; then + if docker-compose up --build --exit-code-from verify verify ; then echo "- - - - - - - - - - - - - - - - - - - - - - - -" echoblue -n "CASE $CASE_STR" echo -n ": "