-
Notifications
You must be signed in to change notification settings - Fork 2k
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
check: Add support for Consul field tls_server_name #17334
Conversation
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 |
There was a problem hiding this 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.
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! |
Ah, so buried in the |
Ok, done! This will ship in the upcoming Nomad 1.6.0. Thanks @beautifulentropy! |
Description
The
tls_server_name
field was added to Consul in hashicorp/consul#9475. This PR adds support for the same field in Nomadservice -> check
stanzas.Motivation
There are two common situations where specifying this field can be beneficial:
When the check address is an IP,
tls_server_name
can be specified for SNI. Note: settingtls_server_name
will also override the hostname used to verify the certificate presented by the server being checked.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
tls_server_name
toservice -> check
stanzas.tls_skip_verify
to indicate support forhttps
andgrpc
withgrpc_use_tls
checks andgrpc_use_tls
to indicate thattls_server_name
can be combined with this field.Links
Fixes #2166