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

check: Add support for Consul field tls_server_name #17334

Merged
merged 5 commits into from
Jun 2, 2023

Conversation

beautifulentropy
Copy link
Contributor

@beautifulentropy beautifulentropy commented May 26, 2023

Description

The tls_server_name field was added to Consul in hashicorp/consul#9475. This PR adds support for the same field in Nomad service -> check stanzas.

Motivation

There are two common situations where specifying this field can be beneficial:

  1. When the check address is an IP, tls_server_name can be specified for SNI. Note: setting tls_server_name will also override the hostname used to verify the certificate presented by the server being checked.

  2. When the hostname in the check address won't be present in the SAN (Subject Alternative Name) field of the certificate presented by the server being checked. Note: setting tls_server_name will also override the hostname used for SNI.

The latter of these is especially important given that today you would have to specify tls_skip_verify which omits TLS verification altogether, a choice which is generally unsuitable for production.

Changes

  • Add support for Consul health check field tls_server_name to service -> check stanzas.
  • Add documentation to support this field.
  • Fix up documentation for:
    • tls_skip_verify to indicate support for https and grpc with grpc_use_tls checks and
    • grpc_use_tls to indicate that tls_server_name can be combined with this field.

Links

  • Related Consul PR: includes an explanation of how this field is implemented inside of Consul, offers a slight tweak to what Consul should do when it's left unspecified, and updates their documentation similar to the docs updates in this PR.

Fixes #2166

@beautifulentropy beautifulentropy marked this pull request as ready for review May 26, 2023 20:18
@beautifulentropy
Copy link
Contributor Author

beautifulentropy commented May 26, 2023

Note sure what, if anything, I can do about the Ember Asset Sizes CI failure:

Run mainmatter/ember-asset-size-action@255fe534db3d2f731e0d70ce9a5de74b0a6b140a
No package-lock.json or yarn.lock detected! We strongly recommend committing one
/opt/hostedtoolcache/node/14.21.3/x64/bin/npm install
npm WARN saveError ENOENT: no such file or directory, open '/home/runner/work/nomad/nomad/package.json'
npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN enoent ENOENT: no such file or directory, open '/home/runner/work/nomad/nomad/package.json'
npm WARN nomad No description
npm WARN nomad No repository field.
npm WARN nomad No README data
npm WARN nomad No license field.

up to date in 0.26[9](https://github.com/hashicorp/nomad/actions/runs/5094531551/jobs/9158463528?pr=17334#step:4:10)s
found 0 vulnerabilities

/opt/hostedtoolcache/node/[14](https://github.com/hashicorp/nomad/actions/runs/5094531551/jobs/9158463528?pr=17334#step:4:15).[21](https://github.com/hashicorp/nomad/actions/runs/5094531551/jobs/9158463528?pr=17334#step:4:22).3/x64/bin/npx ember build -prod
command not found: ember
Error: The process '/opt/hostedtoolcache/node/14.21.3/x64/bin/npx' failed with exit code 1

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.

Hi @beautifulentropy! Thanks for the PR! This looks great -- I've left a couple of minor comments. Can you run make cl as well to add a changelog entry?

I'm not sure what's wrong with the ember assets tests. I know we did some shuffling around of the GitHub Actions for that recently... might be worth rebasing if you branched from main more than a few days before you opened this PR. I might drag @philrenaud in here to see if that's something he has any idea what's up. Not something for you to worry about otherwise.

nomad/structs/services.go Outdated Show resolved Hide resolved
website/content/docs/job-specification/check.mdx Outdated Show resolved Hide resolved
website/content/docs/job-specification/check.mdx Outdated Show resolved Hide resolved
website/content/docs/job-specification/check.mdx Outdated Show resolved Hide resolved
nomad/structs/services.go Show resolved Hide resolved
@beautifulentropy
Copy link
Contributor Author

Not exactly sure what to do with these integration test failures. I can't repro most of them locally and some of them fail locally even on main.

@tgross
Copy link
Member

tgross commented Jun 2, 2023

Not exactly sure what to do with these integration test failures. I can't repro most of them locally and some of them fail locally even on main.

We've got a couple of embarrassingly flaky tests which we need to fix. It looks like you're just hitting those. I've kicked off the build just to double-check but this LGTM and I'll merge once that's sufficiently green. Thanks!

@tgross
Copy link
Member

tgross commented Jun 2, 2023

Ah, so buried in the test-groups (quick) below lots of blank pages in the GHA interface was a genuine test failure in nomad/structs/diff_test.go. I've pushed up a quick fix for that test.

@tgross
Copy link
Member

tgross commented Jun 2, 2023

Ok, done! This will ship in the upcoming Nomad 1.6.0. Thanks @beautifulentropy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service registration with fqdn rather than ip address
2 participants