Backport of NET-5186 Fix issue where consul-dataplane attempts to write to a read-only file system location into release/1.0.x

Dockerfile

@@ -8,8 +8,8 @@
 # prebuilt binaries in any other form.
 FROM envoyproxy/envoy-distroless:v1.24.10 as envoy-binary
 
-# Modify the envoy binary to be able to bind to privileged ports (< 1024)
-FROM alpine:latest AS setcap
+# Modify the envoy binary to be able to bind to privileged ports (< 1024).
+FROM debian:bullseye-slim AS setcap-envoy-binary
 
 ARG BIN_NAME=consul-dataplane
 ARG TARGETARCH
@@ -18,7 +18,7 @@ ARG TARGETOS
 COPY --from=envoy-binary /usr/local/bin/envoy /usr/local/bin/
 COPY dist/$TARGETOS/$TARGETARCH/$BIN_NAME /usr/local/bin/
 
-RUN apk add libcap
+RUN apt-get update && apt install -y libcap2-bin
 RUN setcap CAP_NET_BIND_SERVICE=+ep /usr/local/bin/envoy
 RUN setcap CAP_NET_BIND_SERVICE=+ep /usr/local/bin/$BIN_NAME
 
@@ -56,8 +56,9 @@ LABEL name=${BIN_NAME}\
 
 COPY --from=dumb-init /usr/bin/dumb-init /usr/local/bin/
 COPY --from=go-discover /go/bin/discover /usr/local/bin/
-COPY --from=setcap /usr/local/bin/envoy /usr/local/bin/
-COPY --from=setcap /usr/local/bin/$BIN_NAME /usr/local/bin/
+COPY --from=setcap-envoy-binary /usr/local/bin/envoy /usr/local/bin/
+COPY --from=setcap-envoy-binary /usr/local/bin/$BIN_NAME /usr/local/bin/
+COPY LICENSE /licenses/copyright.txt
 
 USER 100
 
@@ -95,8 +96,8 @@ RUN groupadd --gid 1000 $PRODUCT_NAME && \
 
 COPY --from=dumb-init /usr/bin/dumb-init /usr/local/bin/
 COPY --from=go-discover /go/bin/discover /usr/local/bin/
-COPY --from=setcap /usr/local/bin/envoy /usr/local/bin/
-COPY --from=setcap /usr/local/bin/$BIN_NAME /usr/local/bin/
+COPY --from=setcap-envoy-binary /usr/local/bin/envoy /usr/local/bin/
+COPY --from=setcap-envoy-binary /usr/local/bin/$BIN_NAME /usr/local/bin/
 COPY LICENSE /licenses/copyright.txt
 
 USER 100

pkg/envoy/proxy.go

@@ -6,9 +6,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 	"sync/atomic"
 	"syscall"
@@ -134,14 +132,8 @@ func (p *Proxy) Run(ctx context.Context) error {
 		return errors.New("proxy may only be run once")
 	}
 
-	// Write the bootstrap config to a pipe.
-	configPath, cleanup, err := writeBootstrapConfig(p.cfg.BootstrapConfig)
-	if err != nil {
-		return err
-	}
-
 	// Run the Envoy process.
-	p.cmd = p.buildCommand(ctx, configPath)
+	p.cmd = p.buildCommand(ctx, string(p.cfg.BootstrapConfig))
 
 	// Start Envoy in its own process group to avoid directly receiving
 	// SIGTERM intended for consul-dataplane, let proxy manager handle
@@ -152,10 +144,6 @@ func (p *Proxy) Run(ctx context.Context) error {
 
 	p.cfg.Logger.Debug("running envoy proxy", "command", strings.Join(p.cmd.Args, " "))
 	if err := p.cmd.Start(); err != nil {
-		// Clean up the pipe if we weren't able to run Envoy.
-		if err := cleanup(); err != nil {
-			p.cfg.Logger.Error("failed to cleanup boostrap config", "error", err)
-		}
 		return err
 	}
 
@@ -166,9 +154,6 @@ func (p *Proxy) Run(ctx context.Context) error {
 		err := p.cmd.Wait()
 		p.cfg.Logger.Info("envoy process exited", "error", err)
 		p.transitionState(stateRunning, stateExited)
-		if err := cleanup(); err != nil {
-			p.cfg.Logger.Error("failed to cleanup boostrap config", "error", err)
-		}
 		p.exitedCh <- err
 		close(p.exitedCh)
 	}()
@@ -281,56 +266,9 @@ func (p *Proxy) transitionState(before, after state) bool {
 	return atomic.CompareAndSwapUint32((*uint32)(&p.state), uint32(before), uint32(after))
 }
 
-// writeBootstrapConfig writes the given Envoy bootstrap config to a named pipe
-// and returns the path. It also returns a cleanup function that must be called
-// when Envoy is done with it.
-//
-// We use a named pipe rather than a tempfile because it prevents writing any
-// secrets to disk. See: https://github.com/hashicorp/consul/pull/5964
-func writeBootstrapConfig(cfg []byte) (string, func() error, error) {
-	path := filepath.Join(
-		os.TempDir(),
-		fmt.Sprintf("envoy-%x-bootstrap.json", time.Now().UnixNano()+int64(os.Getpid())),
-	)
-	if err := syscall.Mkfifo(path, 0600); err != nil {
-		return "", nil, err
-	}
-
-	// O_WRONLY causes OpenFile to block until there's a reader (Envoy). Opening
-	// the pipe with O_RDWR wouldn't block but would result in just sending stuff
-	// to ourself.
-	//
-	// TODO(boxofrad): We don't have a way to cancel this goroutine. If the Envoy
-	// process never opens the other end of the pipe this will hang forever. The
-	// workaround we use in `consul connect envoy` is to write to the pipe in a
-	// subprocess that self-destructs after 10 minutes.
-	go func() {
-		file, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND, 0600)
-		if err != nil {
-			os.Remove(path)
-			return
-		}
-
-		_, err = file.Write(cfg)
-		file.Close()
-
-		if err != nil {
-			os.Remove(path)
-		}
-	}()
-
-	return path, func() error {
-		err := os.Remove(path)
-		if err == nil || errors.Is(err, os.ErrNotExist) {
-			return nil
-		}
-		return err
-	}, nil
-}
-
 // buildCommand builds the exec.Cmd to run Envoy with the relevant arguments
 // (e.g. config path) and its logs redirected to the logger.
-func (p *Proxy) buildCommand(ctx context.Context, cfgPath string) *exec.Cmd {
+func (p *Proxy) buildCommand(ctx context.Context, cfgYaml string) *exec.Cmd {
 	var logFormat string
 	if p.cfg.LogJSON {
 		logFormat = logFormatJSON
@@ -368,7 +306,7 @@ func (p *Proxy) buildCommand(ctx context.Context, cfgPath string) *exec.Cmd {
 
 	args := append(
 		[]string{
-			"--config-path", cfgPath,
+			"--config-yaml", cfgYaml,
 			"--log-format", logFormat,
 			"--log-level", logLevel,
 

pkg/envoy/proxy_test.go

@@ -1,8 +1,10 @@
 package envoy
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -12,11 +14,26 @@ import (
 	"time"
 
 	"github.com/hashicorp/go-hclog"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 func TestProxy(t *testing.T) {
-	bootstrapConfig := []byte(`hello world`)
+	bootstrapConfig := []byte(`
+	{
+		"dynamic_resources": {
+			"cds_config": {
+				"ads": {},
+				"initial_fetch_timeout": "0s",
+				"resource_api_version": "V3"
+			},
+			"lds_config": {
+				"ads": {},
+				"initial_fetch_timeout": "0s",
+				"resource_api_version": "V3"
+			},
+		}
+	}`)
 
 	// This test checks that we're starting the Envoy process with the correct
 	// arguments and that it is able to read the config we provide. It does so
@@ -25,9 +42,12 @@ func TestProxy(t *testing.T) {
 	outputPath := testOutputPath()
 	t.Cleanup(func() { _ = os.Remove(outputPath) })
 
+	// Capture fake-envoy output so we know if we encounter an error case below
+	envoyOut := bytes.NewBuffer([]byte{})
+
 	p, err := NewProxy(ProxyConfig{
 		Logger:          hclog.New(&hclog.LoggerOptions{Level: hclog.Warn, Output: io.Discard}),
-		EnvoyLogOutput:  io.Discard,
+		EnvoyLogOutput:  envoyOut,
 		ExecutablePath:  "testdata/fake-envoy",
 		ExtraArgs:       []string{"--test-output", outputPath},
 		BootstrapConfig: bootstrapConfig,
@@ -42,7 +62,10 @@ func TestProxy(t *testing.T) {
 		Args       []byte
 		ConfigData []byte
 	}
-	require.Eventually(t, func() bool {
+
+	// Wait for output file to be generated by fake-envoy.
+	// Use assert so that we can check for error output below.
+	assert.Eventually(t, func() bool {
 		outputBytes, err := os.ReadFile(outputPath)
 		if err != nil {
 			t.Logf("failed to read output file: %v", err)
@@ -55,23 +78,26 @@ func TestProxy(t *testing.T) {
 		return true
 	}, 2*time.Second, 50*time.Millisecond)
 
+	// Check the output from fake-envoy to make sure we didn't hit an error case
+	require.Empty(t, envoyOut.String())
+
 	// Check that fake-envoy was able to read the config from the pipe.
-	require.Equal(t, bootstrapConfig, output.ConfigData)
+	assert.Equal(t, string(bootstrapConfig), string(output.ConfigData))
 
 	// Check that we're correctly configuring the log level.
-	require.Contains(t, string(output.Args), "--log-level warn")
+	assert.Contains(t, string(output.Args), "--log-level warn")
 
 	// Check that we're disabling hot restarts.
-	require.Contains(t, string(output.Args), "--disable-hot-restart")
+	assert.Contains(t, string(output.Args), "--disable-hot-restart")
 
 	// Check the process is still running.
-	require.NoError(t, p.cmd.Process.Signal(syscall.Signal(0)))
+	assert.NoError(t, p.cmd.Process.Signal(syscall.Signal(0)))
 
 	// Ensure Kill kills and reaps the process.
 	require.NoError(t, p.Kill())
-
 	require.Eventually(t, func() bool {
-		return p.cmd.Process.Signal(syscall.Signal(0)) == os.ErrProcessDone
+		err := p.cmd.Process.Signal(syscall.Signal(0))
+		return errors.Is(err, os.ErrProcessDone)
 	}, 2*time.Second, 50*time.Millisecond)
 }
 

pkg/envoy/testdata/fake-envoy

@@ -1,20 +1,20 @@
 #!/bin/bash
 
 # This script pretends to be Envoy in unit tests. It captures the flags and the
-# bootstrap config from the named pipe specified via `--config-path`, and writes
-# them to the file at `--test-output` (which is read and checked in the test).
+# bootstrap config specified via `--config-yaml`, and writes them to the file at
+# `--test-output` (which is read and checked in the test).
 # It then sleeps for 10 minutes to check we're correctly killing the process.
 
 set -e
 
-config_path=""
+config_yaml=""
 test_output=""
 
 prev_arg=""
 for arg in "$@"; do
   case "$prev_arg" in
-    --config-path)
-      config_path="$arg"
+    --config-yaml)
+      config_yaml="$arg"
       ;;
     --test-output)
       test_output="$arg"
@@ -23,8 +23,8 @@ for arg in "$@"; do
   prev_arg="$arg"
 done
 
-if [ -z "$config_path" ]; then
-  >&2 echo "--config-path is required"
+if [ -z "$config_yaml" ]; then
+  >&2 echo "--config-yaml is required"
   exit 1
 fi
 
@@ -35,7 +35,7 @@ fi
 
 # Base64 encode the data to avoid having to escape it in the JSON output.
 args=$(echo "$@" | base64 | tr -d \\n)
-config_data=$(base64 -i $config_path | tr -d \\n)
+config_data=$(echo -n "$config_yaml" | base64 | tr -d \\n)
 
 cat <<EOF > "$test_output"
 {