consul/connect: fix regression where client connect images ignored

Nomad v1.0.0 introduced a regression where the client configurations
for `connect.sidecar_image` and `connect.gateway_image` would be
ignored despite being set. This PR restores that functionality.

There was a missing layer of interpolation that needs to occur for
these parameters. Since Nomad 1.0 now supports dynamic envoy versioning
through the ${NOMAD_envoy_version} psuedo variable, we basically need
to first interpolate

  ${connect.sidecar_image} => envoyproxy/envoy:v${NOMAD_envoy_version}

then use Consul at runtime to resolve to a real image, e.g.

  envoyproxy/envoy:v${NOMAD_envoy_version} => envoyproxy/envoy:v1.16.0

Of course, if the version of Consul is too old to provide an envoy
version preference, we then need to know to fallback to the old
version of envoy that we used before.

  envoyproxy/envoy:v${NOMAD_envoy_version} => envoyproxy/envoy:v1.11.2@sha256:a7769160c9c1a55bb8d07a3b71ce5d64f72b1f665f10d81aa1581bc3cf850d09

Beyond that, we also need to continue to support jobs that set the
sidecar task themselves, e.g.

  sidecar_task { config { image: "custom/envoy" } }

which itself could include teh pseudo envoy version variable.

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,10 @@ 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: taskenv.NewTaskEnv(nil, nil, map[string]string{
+			"connect.sidecar_image": envoy.ImageFormat,
+			"connect.gateway_image": envoy.ImageFormat,
+		}),
 	}
 	require.NoError(t, request.TaskDir.Build(false, nil))
 
@@ -231,7 +291,10 @@ 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: taskenv.NewTaskEnv(nil, nil, map[string]string{
+			"connect.sidecar_image": envoy.ImageFormat,
+			"connect.gateway_image": envoy.ImageFormat,
+		}),
 	}
 	require.NoError(t, request.TaskDir.Build(false, nil))
 
@@ -278,7 +341,10 @@ 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: taskenv.NewTaskEnv(nil, nil, map[string]string{
+			"connect.sidecar_image": envoy.ImageFormat,
+			"connect.gateway_image": envoy.ImageFormat,
+		}),
 	}
 	require.NoError(t, request.TaskDir.Build(false, nil))
 
@@ -319,7 +385,10 @@ 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: taskenv.NewTaskEnv(nil, nil, map[string]string{
+			"connect.sidecar_image": envoy.ImageFormat,
+			"connect.gateway_image": envoy.ImageFormat,
+		}),
 	}
 	require.NoError(t, request.TaskDir.Build(false, nil))
 
@@ -360,7 +429,10 @@ 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: taskenv.NewTaskEnv(nil, nil, map[string]string{
+			"connect.sidecar_image": envoy.ImageFormat,
+			"connect.gateway_image": envoy.ImageFormat,
+		}),
 	}
 	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