Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consul/connect: fix regression where client connect images ignored #9624

Merged
merged 1 commit into from
Dec 14, 2020

Conversation

shoenig
Copy link
Member

@shoenig shoenig commented Dec 11, 2020

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 the pseudo envoy version variable.

Fixes #9618

@shoenig
Copy link
Member Author

shoenig commented Dec 14, 2020

Spot check every combination.

From lowest to highest precedence:
consul version: consul_old (envoy v1.11.2), consul_new (envoy v1.16.0)
client.meta: meta_unset, meta_set (envoy v1.14.4)
sidecar_task.image: image_unset, image_set (envoy v1.15.3)

Consul      meta.       image.         Expect    OK
----------------------------------------------------
consul_old  meta_unset  image_unset -> v1.11.2   [y]
consul_old  meta_unset  image_set   -> v1.15.3   [y]
consul_old  meta_set    image_unset -> v1.14.4   [y]
consul_old  meta_set    image_set   -> v1.15.3   [y]
consul_new  meta_unset  image_unset -> v1.16.0   [y]
consul_new  meta_unset  image_set   -> v1.15.3   [y]
consul_new  meta_set    image_unset -> v1.14.4   [y]
consul_new  meta_set    image_set   -> v1.15.3   [y]

@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 14, 2020 15:31 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 14, 2020 15:31 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 14, 2020 15:31 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 14, 2020 15:31 Inactive
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.
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 14, 2020 15:48 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 14, 2020 15:48 Inactive
@shoenig shoenig marked this pull request as ready for review December 14, 2020 16:12
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -96,6 +105,54 @@ func TestEnvoyVersionHook_tweakImage(t *testing.T) {
})
}

func TestEnvoyVersionHook_interpolateImage(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid test coverage here 👍

@shoenig shoenig merged commit 14aca2f into master Dec 14, 2020
@shoenig shoenig deleted the b-connect-meta-regression branch December 14, 2020 17:03
schmichael pushed a commit that referenced this pull request Dec 16, 2020
consul/connect: fix regression where client connect images ignored
shoenig added a commit that referenced this pull request Dec 16, 2020
backspace pushed a commit that referenced this pull request Jan 22, 2021
@HarryHarcourt
Copy link

HarryHarcourt commented Feb 11, 2021

We are running Nomad v1.0.2
And either this does not work, or we are trying to set the value in the wrong place.
Any help would be appreciated, we are putting this in our client agent configuration:
options {
meta.connect.sidecar_image = "artifactory.xxxx.com/docker/envoy:1.11.2"
}
But do not see any change when running nomad node status -self -verbose
Meta
connect.gateway_image = envoyproxy/envoy:v${NOMAD_envoy_version}
connect.log_level = info
connect.proxy_concurrency = 1
connect.sidecar_image = envoyproxy/envoy:v${NOMAD_envoy_version}

@tgross tgross added this to the 1.0.2 milestone Feb 11, 2021
@tgross
Copy link
Member

tgross commented Feb 11, 2021

Hi @HarryHarcourt! I think you want:

client {
  meta {
    "connect.sidecar_image" = "artifactory.standard.com/docker/envoy:1.11.2"
  }
}

But if that doesn't work for you please open a new issue. By the way, the options block is deprecated since Nomad 0.9 and IIRC it doesn't get the new configuration fields.

@HarryHarcourt
Copy link

Confirmed that this works.
If this is moving to an options approach, what plugin would this be configured against? There is no documentation to support this approach.

@idrennanvmware
Copy link
Contributor

idrennanvmware commented Feb 11, 2021

@HarryHarcourt its just the standard nomad "client" configuration. Not specific plugins or drivers around them.

{
    "client": {
        "enabled": true,
        "network_interface" : "???",
        "meta": ["connect.sidecar_image": "{{ default_docker_registry }}/{{ images_dict_data.envoy }}", "connect.gateway_image": "{{ default_docker_registry }}/{{ images_dict_data.envoy }}"
        ]
    }
}

Please ignore our templating lol

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug/regression] nomad 1.0.0 ignores nomad meta.connect.(sidecar|gateway)_image
4 participants