-
Notifications
You must be signed in to change notification settings - Fork 124
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
Adds nomad provider #248
base: master
Are you sure you want to change the base?
Adds nomad provider #248
Conversation
Fixes #187 |
if args["region"] != "" { | ||
clientConfig.Region = args["region"] | ||
} | ||
if args["secret_id"] != "" { |
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.
Does something need to be done to pass the WI token, if present, here or is there hidden magic?
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.
This is available as an ENV, as is the unix socket to talk to Nomad, so it should just be an issue of passing in the env var as the secret ID properly.
// TODO: Is all of this needed if using the socket and the WI token? | ||
|
||
// handle clientConfig.HttpAuth.Username input from args | ||
if args["http_auth_username"] != "" { |
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.
Is HTTP auth relevant for Nomad? And I didn't see arguments for it
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.
If there's a proxy with http auth between us and Nomad.
If you're contacting Nomad from a Nomad task, which is the 95% use case, then I think its not useful - and same with all the TLS stuff. So maybe I just rip it out and say "this is the way to use this"?
I spoke with Nomad Engineers about this, and we decided to rip out all the custom input handling for TLS/Auth etc and just rely on Env vars being set. This should make this even simpler. Going to block this until the API package supports unix sockets though, as that will be the most common way of communicating with Nomad - the PR should be merged early next week. |
How does the feature-complete notice here impact this functionality? |
Hey @jose-fully-ported, we'll still get it in once the sdk PR gets merged. This (plus some upcoming auth changes in Nomad) should get us closer to running Vault/Consul on Nomad as jobs, so this satisfies for the "as needed" condition IMO. Will add Nomad to the as-needed list in the Readme too. |
It would be cool to be able to find Consul and Vault agents running on Nomad. This adds Nomad support to go-discover to enable this.
Note: I think we can avoid all the TLS stuff by forcing users to use the unix socket to interact with the API. Maybe there's a good reason to have all the TLS and HTTP Auth stuff tho?
To Dos: