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 683262a686.

* Review cleanups

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

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{

command/connect/envoy/exec_unix.go

@@ -4,16 +4,22 @@ package envoy
 
 import (
 	"errors"
-	"io"
+	"fmt"
-	"io/ioutil"
 	"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 {
+func makeBootstrapPipe(bootstrapJSON []byte) (string, error) {
-	// Write the Envoy bootstrap config file out to disk in a pocket universe
+	pipeFile := filepath.Join(os.TempDir(),
-	// visible only to the current process (and exec'd future selves).
+		fmt.Sprintf("envoy-%x-bootstrap.json", time.Now().UnixNano()+int64(os.Getpid())))
-	fd, err := writeEphemeralEnvoyTempFile(bootstrapJson)
+
+	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:
+	// Get our own executable path.
-	//
+	execPath, err := os.Executable()
-	//   0: stdin
+	if err != nil {
-	//   1: stdout
+		return pipeFile, err
-	//   2: stderr
+	}
-	//   ... any open file descriptors from the parent without CLOEXEC set
+
-	//
+	if testSelfExecOverride != "" {
-	// Above we explicitly disabled CLOEXEC for our temp file, so assuming
+		execPath = testSelfExecOverride
-	// FD numbers survive across execs, it should just be the value of
+	} else if strings.HasSuffix(execPath, "/envoy.test") {
-	// `fd`. This is accessible as a file itself (trippy!) under
+		return pipeFile, fmt.Errorf("I seem to be running in a test binary without " +
-	// /dev/fd/$FDNUMBER.
+			"overriding the self-executable. Not doing that - it will make you sad. " +
-	magicPath := filepath.Join("/dev/fd", strconv.Itoa(int(fd)))
+			"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/<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",
+	envoyArgs = append(envoyArgs, "--config-path", pipeFile)
-		"--config-path",
-		magicPath,
-	)
 	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)
-}

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

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

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
 }

test/integration/connect/envoy/case-consul-exec/setup.sh

@@ -2,6 +2,11 @@
 
 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).

test/integration/connect/envoy/case-consul-exec/verify.bats

@@ -12,3 +12,8 @@ load helpers
 @test "s1 proxy listener should be up and have right cert" {
   assert_proxy_presents_cert_uri localhost:21000 s1
 }
+
+@test "s1 proxy is running correct version" {
+  assert_envoy_version 19000
+}
+

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"

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

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 ": "