consul/connect: fix regression where client connect images ignored

client/allocrunner/taskrunner/envoy_version_hook.go

@@ -8,20 +8,15 @@ import (
 	"github.com/hashicorp/go-version"
 	ifs "github.com/hashicorp/nomad/client/allocrunner/interfaces"
 	"github.com/hashicorp/nomad/client/consul"
+	"github.com/hashicorp/nomad/client/taskenv"
+	"github.com/hashicorp/nomad/helper/envoy"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/pkg/errors"
 )
 
 const (
 	// envoyVersionHookName is the name of this hook and appears in logs.
 	envoyVersionHookName = "envoy_version"
-
-	// envoyLegacyImage is used when the version of Consul is too old to support
-	// the SupportedProxies field in the self API.
-	//
-	// This is the version defaulted by Nomad before v1.0 and/or when using versions
-	// of Consul before v1.7.8, v1.8.5, and v1.9.0.
-	envoyLegacyImage = "envoyproxy/envoy:v1.11.2@sha256:a7769160c9c1a55bb8d07a3b71ce5d64f72b1f665f10d81aa1581bc3cf850d09"
 )
 
 type envoyVersionHookConfig struct {
@@ -68,6 +63,15 @@ func (envoyVersionHook) Name() string {
 }
 
 func (h *envoyVersionHook) Prestart(_ context.Context, request *ifs.TaskPrestartRequest, response *ifs.TaskPrestartResponse) error {
+	// First interpolation of the task image. Typically this turns the default
+	// ${meta.connect.sidecar_task} into envoyproxy/envoy:v${NOMAD_envoy_version}
+	// but could be a no-op or some other value if so configured.
+	h.interpolateImage(request.Task, request.TaskEnv)
+
+	// Detect whether this hook needs to run and return early if not. Only run if:
+	// - task uses docker driver
+	// - task is a connect sidecar or gateway
+	// - task image needs ${NOMAD_envoy_version} resolved
 	if h.skip(request) {
 		response.Done = true
 		return nil
@@ -80,8 +84,9 @@ func (h *envoyVersionHook) Prestart(_ context.Context, request *ifs.TaskPrestart
 		return errors.Wrap(err, "error retrieving supported Envoy versions from Consul")
 	}
 
-	// Determine the concrete Envoy image identifier by applying version string
-	// substitution (${NOMAD_envoy_version}).
+	// Second [pseudo] interpolation of task image. This determines the concrete
+	// Envoy image identifier by applying version string substitution of
+	// ${NOMAD_envoy_version} acquired from Consul.
 	image, err := h.tweakImage(h.taskImage(request.Task.Config), proxies)
 	if err != nil {
 		return errors.Wrap(err, "error interpreting desired Envoy version from Consul")
@@ -94,6 +99,24 @@ func (h *envoyVersionHook) Prestart(_ context.Context, request *ifs.TaskPrestart
 	return nil
 }
 
+// interpolateImage applies the first pass of interpolation on the task's
+// config.image value. This is where ${meta.connect.sidecar_image} or
+// ${meta.connect.gateway_image} becomes something that might include the
+// ${NOMAD_envoy_version} pseudo variable for further resolution.
+func (_ *envoyVersionHook) interpolateImage(task *structs.Task, env *taskenv.TaskEnv) {
+	value, exists := task.Config["image"]
+	if !exists {
+		return
+	}
+
+	image, ok := value.(string)
+	if !ok {
+		return
+	}
+
+	task.Config["image"] = env.ReplaceEnv(image)
+}
+
 // skip will return true if the request does not contain a task that should have
 // its envoy proxy version resolved automatically.
 func (h *envoyVersionHook) skip(request *ifs.TaskPrestartRequest) bool {
@@ -112,22 +135,22 @@ func (h *envoyVersionHook) skip(request *ifs.TaskPrestartRequest) bool {
 // If the image is empty or not a string, Nomad will fallback to the normal
 // official Envoy image as if the setting was not configured. This is also what
 // Nomad would do if the sidecar_task was not set in the first place.
-func (_ *envoyVersionHook) taskImage(config map[string]interface{}) string {
+func (h *envoyVersionHook) taskImage(config map[string]interface{}) string {
 	value, exists := config["image"]
 	if !exists {
-		return structs.EnvoyImageFormat
+		return envoy.ImageFormat
 	}
 
 	image, ok := value.(string)
 	if !ok {
-		return structs.EnvoyImageFormat
+		return envoy.ImageFormat
 	}
 
 	return image
 }
 
 // needsVersion returns true if the docker.config.image is making use of the
-// ${NOMAD_envoy_version} faux environment variable.
+// ${NOMAD_envoy_version} faux environment variable, or
 // Nomad does not need to query Consul to get the preferred Envoy version, etc.)
 func (h *envoyVersionHook) needsVersion(config map[string]interface{}) bool {
 	if len(config) == 0 {
@@ -136,23 +159,23 @@ func (h *envoyVersionHook) needsVersion(config map[string]interface{}) bool {
 
 	image := h.taskImage(config)
 
-	return strings.Contains(image, structs.EnvoyVersionVar)
+	return strings.Contains(image, envoy.VersionVar)
 }
 
-// image determines the best Envoy version to use. If supported is nil or empty
+// tweakImage determines the best Envoy version to use. If supported is nil or empty
 // Nomad will fallback to the legacy envoy image used before Nomad v1.0.
-func (_ *envoyVersionHook) tweakImage(configured string, supported map[string][]string) (string, error) {
+func (h *envoyVersionHook) tweakImage(configured string, supported map[string][]string) (string, error) {
 	versions := supported["envoy"]
 	if len(versions) == 0 {
-		return envoyLegacyImage, nil
+		return envoy.FallbackImage, nil
 	}
 
 	latest, err := semver(versions[0])
 	if err != nil {
 		return "", err
 	}
 
-	return strings.ReplaceAll(configured, structs.EnvoyVersionVar, latest), nil
+	return strings.ReplaceAll(configured, envoy.VersionVar, latest), nil
 }
 
 // semver sanitizes the envoy version string coming from Consul into the format

client/allocrunner/taskrunner/envoy_version_hook_test.go

@@ -8,13 +8,21 @@ import (
 	ifs "github.com/hashicorp/nomad/client/allocrunner/interfaces"
 	"github.com/hashicorp/nomad/client/taskenv"
 	"github.com/hashicorp/nomad/command/agent/consul"
+	"github.com/hashicorp/nomad/helper/envoy"
 	"github.com/hashicorp/nomad/helper/testlog"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
 )
 
+var (
+	taskEnvDefault = taskenv.NewTaskEnv(nil, nil, map[string]string{
+		"meta.connect.sidecar_image": envoy.ImageFormat,
+		"meta.connect.gateway_image": envoy.ImageFormat,
+	})
+)
+
 func TestEnvoyVersionHook_semver(t *testing.T) {
 	t.Parallel()
 
@@ -43,14 +51,14 @@ func TestEnvoyVersionHook_taskImage(t *testing.T) {
 		result := (*envoyVersionHook)(nil).taskImage(map[string]interface{}{
 			// empty
 		})
-		require.Equal(t, structs.EnvoyImageFormat, result)
+		require.Equal(t, envoy.ImageFormat, result)
 	})
 
 	t.Run("not a string", func(t *testing.T) {
 		result := (*envoyVersionHook)(nil).taskImage(map[string]interface{}{
 			"image": 7, // not a string
 		})
-		require.Equal(t, structs.EnvoyImageFormat, result)
+		require.Equal(t, envoy.ImageFormat, result)
 	})
 
 	t.Run("normal", func(t *testing.T) {
@@ -63,12 +71,13 @@ func TestEnvoyVersionHook_taskImage(t *testing.T) {
 
 func TestEnvoyVersionHook_tweakImage(t *testing.T) {
 	t.Parallel()
-	image := structs.EnvoyImageFormat
+
+	image := envoy.ImageFormat
 
 	t.Run("legacy", func(t *testing.T) {
 		result, err := (*envoyVersionHook)(nil).tweakImage(image, nil)
 		require.NoError(t, err)
-		require.Equal(t, envoyLegacyImage, result)
+		require.Equal(t, envoy.FallbackImage, result)
 	})
 
 	t.Run("unexpected", func(t *testing.T) {
@@ -96,6 +105,54 @@ func TestEnvoyVersionHook_tweakImage(t *testing.T) {
 	})
 }
 
+func TestEnvoyVersionHook_interpolateImage(t *testing.T) {
+	t.Parallel()
+
+	hook := (*envoyVersionHook)(nil)
+
+	t.Run("default sidecar", func(t *testing.T) {
+		task := &structs.Task{
+			Config: map[string]interface{}{"image": envoy.SidecarConfigVar},
+		}
+		hook.interpolateImage(task, taskEnvDefault)
+		require.Equal(t, envoy.ImageFormat, task.Config["image"])
+	})
+
+	t.Run("default gateway", func(t *testing.T) {
+		task := &structs.Task{
+			Config: map[string]interface{}{"image": envoy.GatewayConfigVar},
+		}
+		hook.interpolateImage(task, taskEnvDefault)
+		require.Equal(t, envoy.ImageFormat, task.Config["image"])
+	})
+
+	t.Run("custom static", func(t *testing.T) {
+		task := &structs.Task{
+			Config: map[string]interface{}{"image": "custom/envoy"},
+		}
+		hook.interpolateImage(task, taskEnvDefault)
+		require.Equal(t, "custom/envoy", task.Config["image"])
+	})
+
+	t.Run("custom interpolated", func(t *testing.T) {
+		task := &structs.Task{
+			Config: map[string]interface{}{"image": "${MY_ENVOY}"},
+		}
+		hook.interpolateImage(task, taskenv.NewTaskEnv(map[string]string{
+			"MY_ENVOY": "my/envoy",
+		}, nil, nil))
+		require.Equal(t, "my/envoy", task.Config["image"])
+	})
+
+	t.Run("no image", func(t *testing.T) {
+		task := &structs.Task{
+			Config: map[string]interface{}{},
+		}
+		hook.interpolateImage(task, taskEnvDefault)
+		require.Empty(t, task.Config)
+	})
+}
+
 func TestEnvoyVersionHook_skip(t *testing.T) {
 	t.Parallel()
 
@@ -153,7 +210,7 @@ func TestEnvoyVersionHook_skip(t *testing.T) {
 				Driver: "docker",
 				Kind:   structs.NewTaskKind(structs.ConnectProxyPrefix, "task"),
 				Config: map[string]interface{}{
-					"image": structs.EnvoyImageFormat,
+					"image": envoy.ImageFormat,
 				},
 			},
 		})
@@ -187,7 +244,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_standard(t *testing.T) {
 	request := &ifs.TaskPrestartRequest{
 		Task:    alloc.Job.TaskGroups[0].Tasks[0],
 		TaskDir: allocDir.NewTaskDir(alloc.Job.TaskGroups[0].Tasks[0].Name),
-		TaskEnv: taskenv.NewEmptyTaskEnv(),
+		TaskEnv: taskEnvDefault,
 	}
 	require.NoError(t, request.TaskDir.Build(false, nil))
 
@@ -231,7 +288,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_custom(t *testing.T) {
 	request := &ifs.TaskPrestartRequest{
 		Task:    alloc.Job.TaskGroups[0].Tasks[0],
 		TaskDir: allocDir.NewTaskDir(alloc.Job.TaskGroups[0].Tasks[0].Name),
-		TaskEnv: taskenv.NewEmptyTaskEnv(),
+		TaskEnv: taskEnvDefault,
 	}
 	require.NoError(t, request.TaskDir.Build(false, nil))
 
@@ -278,7 +335,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_skip(t *testing.T) {
 	request := &ifs.TaskPrestartRequest{
 		Task:    alloc.Job.TaskGroups[0].Tasks[0],
 		TaskDir: allocDir.NewTaskDir(alloc.Job.TaskGroups[0].Tasks[0].Name),
-		TaskEnv: taskenv.NewEmptyTaskEnv(),
+		TaskEnv: taskEnvDefault,
 	}
 	require.NoError(t, request.TaskDir.Build(false, nil))
 
@@ -319,7 +376,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_fallback(t *testing.T) {
 	request := &ifs.TaskPrestartRequest{
 		Task:    alloc.Job.TaskGroups[0].Tasks[0],
 		TaskDir: allocDir.NewTaskDir(alloc.Job.TaskGroups[0].Tasks[0].Name),
-		TaskEnv: taskenv.NewEmptyTaskEnv(),
+		TaskEnv: taskEnvDefault,
 	}
 	require.NoError(t, request.TaskDir.Build(false, nil))
 
@@ -360,7 +417,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_error(t *testing.T) {
 	request := &ifs.TaskPrestartRequest{
 		Task:    alloc.Job.TaskGroups[0].Tasks[0],
 		TaskDir: allocDir.NewTaskDir(alloc.Job.TaskGroups[0].Tasks[0].Name),
-		TaskEnv: taskenv.NewEmptyTaskEnv(),
+		TaskEnv: taskEnvDefault,
 	}
 	require.NoError(t, request.TaskDir.Build(false, nil))
 

client/client.go

@@ -18,6 +18,7 @@ import (
 	"github.com/hashicorp/consul/lib"
 	hclog "github.com/hashicorp/go-hclog"
 	multierror "github.com/hashicorp/go-multierror"
+	"github.com/hashicorp/nomad/helper/envoy"
 	vaultapi "github.com/hashicorp/vault/api"
 	"github.com/pkg/errors"
 	"github.com/shirou/gopsutil/host"
@@ -97,18 +98,6 @@ const (
 	// the status of the allocation
 	allocSyncRetryIntv = 5 * time.Second
 
-	// defaultConnectSidecarImage is the image set in the node meta by default
-	// to be used by Consul Connect sidecar tasks. As of Nomad 1.0, this value
-	// is only used as a fallback when the version of Consul does not yet support
-	// dynamic envoy versions.
-	defaultConnectSidecarImage = "envoyproxy/envoy:v1.11.2@sha256:a7769160c9c1a55bb8d07a3b71ce5d64f72b1f665f10d81aa1581bc3cf850d09"
-
-	// defaultConnectGatewayImage is the image set in the node meta by default
-	// to be used by Consul Connect Gateway tasks. As of Nomad 1.0, this value
-	// is only used as a fallback when the version of Consul does not yet support
-	// dynamic envoy versions.
-	defaultConnectGatewayImage = defaultConnectSidecarImage
-
 	// defaultConnectLogLevel is the log level set in the node meta by default
 	// to be used by Consul Connect sidecar tasks.
 	defaultConnectLogLevel = "info"
@@ -1403,11 +1392,11 @@ func (c *Client) setupNode() error {
 	node.Status = structs.NodeStatusInit
 
 	// Setup default meta
-	if _, ok := node.Meta["connect.sidecar_image"]; !ok {
-		node.Meta["connect.sidecar_image"] = defaultConnectSidecarImage
+	if _, ok := node.Meta[envoy.SidecarMetaParam]; !ok {
+		node.Meta[envoy.SidecarMetaParam] = envoy.ImageFormat
 	}
-	if _, ok := node.Meta["connect.gateway_image"]; !ok {
-		node.Meta["connect.gateway_image"] = defaultConnectGatewayImage
+	if _, ok := node.Meta[envoy.GatewayMetaParam]; !ok {
+		node.Meta[envoy.GatewayMetaParam] = envoy.ImageFormat
 	}
 	if _, ok := node.Meta["connect.log_level"]; !ok {
 		node.Meta["connect.log_level"] = defaultConnectLogLevel

helper/envoy/envoy.go

@@ -0,0 +1,49 @@
+// Package envoy provides a high level view of the variables that go into
+// selecting an envoy version.
+package envoy
+
+const (
+	// SidecarMetaParam is the parameter name used to configure the connect sidecar
+	// at the client level. Setting this option in client configuration takes the
+	// lowest precedence.
+	//
+	// If this meta option is not set in client configuration, it defaults to
+	// ImageFormat, so that Nomad will defer envoy version selection to Consul.
+	SidecarMetaParam = "connect.sidecar_image"
+
+	// SidecarConfigVar is used as the default config.image value for connect
+	// sidecar proxies, when they are injected in the job connect mutator.
+	SidecarConfigVar = "${meta." + SidecarMetaParam + "}"
+
+	// GatewayMetaParam is the parameter name used to configure the connect gateway
+	// at the client level. Setting this option in client configuration takes the
+	// lowest precedence.
+	//
+	// If this meta option is not set in client configuration, it defaults to
+	// ImageFormat, so that Nomad will defer envoy version selection to Consul.
+	GatewayMetaParam = "connect.gateway_image"
+
+	// GatewayConfigVar is used as the default config.image value for connect
+	// gateway proxies, when they are injected in the job connect mutator.
+	GatewayConfigVar = "${meta." + GatewayMetaParam + "}"
+
+	// envoyImageFormat is the default format string used for official envoy Docker
+	// images with the tag being the semver of the version of envoy. Nomad fakes
+	// interpolation of ${NOMAD_envoy_version} by replacing it with the version
+	// string for envoy that Consul reports as preferred.
+	//
+	// Folks wanting to build and use custom images while still having Nomad refer
+	// to specific versions as preferred by Consul would set meta.connect.sidecar_image
+	// to something like: "custom/envoy:${NOMAD_envoy_version}".
+	ImageFormat = "envoyproxy/envoy:v" + VersionVar
+
+	// envoyVersionVar will be replaced with the Envoy version string when
+	// used in the meta.connect.sidecar_image variable.
+	VersionVar = "${NOMAD_envoy_version}"
+
+	// FallbackImage is the image set in the node meta by default
+	// to be used by Consul Connect sidecar tasks. As of Nomad 1.0, this value
+	// is only used as a fallback when the version of Consul does not yet support
+	// dynamic envoy versions.
+	FallbackImage = "envoyproxy/envoy:v1.11.2@sha256:a7769160c9c1a55bb8d07a3b71ce5d64f72b1f665f10d81aa1581bc3cf850d09"
+)