From 76bc2ea47d9897370dd7af8c84789711dedebc9d Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Fri, 11 Dec 2020 09:57:13 -0600 Subject: [PATCH] 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. --- .../taskrunner/envoy_version_hook.go | 58 ++++++++---- .../taskrunner/envoy_version_hook_test.go | 92 +++++++++++++++++-- client/client.go | 21 +---- helper/envoy/envoy.go | 49 ++++++++++ nomad/job_endpoint_hook_connect.go | 8 +- nomad/mock/mock.go | 3 +- nomad/structs/connect.go | 17 ---- 7 files changed, 184 insertions(+), 64 deletions(-) create mode 100644 helper/envoy/envoy.go delete mode 100644 nomad/structs/connect.go diff --git a/client/allocrunner/taskrunner/envoy_version_hook.go b/client/allocrunner/taskrunner/envoy_version_hook.go index eed00952aa18..d4b1ac6627b4 100644 --- a/client/allocrunner/taskrunner/envoy_version_hook.go +++ b/client/allocrunner/taskrunner/envoy_version_hook.go @@ -8,6 +8,8 @@ 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" ) @@ -15,13 +17,6 @@ import ( 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,16 @@ 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 +85,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 +100,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,15 +136,15 @@ 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 @@ -136,15 +160,15 @@ 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]) @@ -152,7 +176,7 @@ func (_ *envoyVersionHook) tweakImage(configured string, supported map[string][] 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 diff --git a/client/allocrunner/taskrunner/envoy_version_hook_test.go b/client/allocrunner/taskrunner/envoy_version_hook_test.go index 8587af3b78a7..04a7a4df90bb 100644 --- a/client/allocrunner/taskrunner/envoy_version_hook_test.go +++ b/client/allocrunner/taskrunner/envoy_version_hook_test.go @@ -8,6 +8,7 @@ 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" @@ -15,6 +16,13 @@ import ( "github.com/stretchr/testify/require" ) +var ( + taskEnvDefault = taskenv.NewTaskEnv(nil, nil, map[string]string{ + "connect.sidecar_image": envoy.ImageFormat, + "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)) diff --git a/client/client.go b/client/client.go index 99af8e3c7f77..911c1d5e677e 100644 --- a/client/client.go +++ b/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 diff --git a/helper/envoy/envoy.go b/helper/envoy/envoy.go new file mode 100644 index 000000000000..86c404010fbd --- /dev/null +++ b/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 = "${" + 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 = "${" + 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" +) diff --git a/nomad/job_endpoint_hook_connect.go b/nomad/job_endpoint_hook_connect.go index 2b53b723f25e..1ac2b6cb53e0 100644 --- a/nomad/job_endpoint_hook_connect.go +++ b/nomad/job_endpoint_hook_connect.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/helper" + "github.com/hashicorp/nomad/helper/envoy" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/structs" "github.com/pkg/errors" @@ -25,7 +26,7 @@ var ( // connect proxy sidecar task. connectSidecarDriverConfig = func() map[string]interface{} { return map[string]interface{}{ - "image": structs.EnvoyImageFormat, + "image": envoy.SidecarConfigVar, "args": []interface{}{ "-c", structs.EnvoyBootstrapPath, "-l", "${meta.connect.log_level}", @@ -42,7 +43,7 @@ var ( // networking is being used the network_mode driver configuration is set here. connectGatewayDriverConfig = func(hostNetwork bool) map[string]interface{} { m := map[string]interface{}{ - "image": structs.EnvoyImageFormat, + "image": envoy.GatewayConfigVar, "args": []interface{}{ "-c", structs.EnvoyBootstrapPath, "-l", "${meta.connect.log_level}", @@ -175,7 +176,6 @@ func getNamedTaskForNativeService(tg *structs.TaskGroup, serviceName, taskName s // probably need to hack this up to look for checks on the service, and if they // qualify, configure a port for envoy to use to expose their paths. func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { - // Create an environment interpolator with what we have at submission time. // This should only be used to interpolate connect service names which are // used in sidecar or gateway task names. Note that the service name might @@ -265,7 +265,9 @@ func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { if !hasGatewayTaskForService(g, service.Name) { // use the default envoy image, for now there is no support for a custom task task := newConnectGatewayTask(service.Name, netHost) + g.Tasks = append(g.Tasks, task) + task.Canonicalize(job, g) } } diff --git a/nomad/mock/mock.go b/nomad/mock/mock.go index c1b629b732c7..0273eeeead5f 100644 --- a/nomad/mock/mock.go +++ b/nomad/mock/mock.go @@ -5,6 +5,7 @@ import ( "time" "github.com/hashicorp/nomad/helper" + "github.com/hashicorp/nomad/helper/envoy" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/structs" psstructs "github.com/hashicorp/nomad/plugins/shared/structs" @@ -734,7 +735,7 @@ func ConnectSidecarTask() *structs.Task { Driver: "docker", User: "nobody", Config: map[string]interface{}{ - "image": structs.EnvoyImageFormat, + "image": envoy.SidecarConfigVar, }, Env: nil, Resources: &structs.Resources{ diff --git a/nomad/structs/connect.go b/nomad/structs/connect.go deleted file mode 100644 index 68928ea8ff19..000000000000 --- a/nomad/structs/connect.go +++ /dev/null @@ -1,17 +0,0 @@ -package structs - -const ( - // 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}". - EnvoyImageFormat = "envoyproxy/envoy:v" + EnvoyVersionVar - - // envoyVersionVar will be replaced with the Envoy version string when - // used in the meta.connect.sidecar_image variable. - EnvoyVersionVar = "${NOMAD_envoy_version}" -)