-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[api] Add Unix domain socket support #16872
Conversation
I'm hoping that this change will work nicely with the Task HTTP API that recently got exposed. I nudged @mikenomitch on #4637 a little while back. My use case is that we've both got shell invocations of |
@gudmundur That's definitely a goal with this addition. I appreciate the extra information about your use case as well. The nice thing about this code is that it doesn't require PR #16884 to be useful and could be built and tested in your environment. If you do, feedback would be greatly appreciated. |
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.
LGTM!
@angrycub are you willing to push this one over the finish line? Or should we look for someone else to do that? |
Well, that is assuming he has the time to finish it.. Also, this PR makes a lot more changes and has a different approach.
No need for custom properties like Also it doesn't alter the struct or require a new environment variable. Anyway, either fix is fine for me, my only goal is to get access to this feature... |
// URL returns a copy of the initial parsed address and is not modified in the | ||
// case of a `unix://` URL, as opposed to Address. | ||
func (c *Config) URL() *url.URL { | ||
return c.url | ||
} |
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.
Note to other reviewers: @angrycub and I paired on this for quite a while. The code here in the api
package handles the unix://
scheme correctly now without doing a bunch of weird string manipulation. But the nomad operator api
package has some special case logic around emitting curl
-alike outputs. Rather than exposing flags or fields special-casing the UDS, we landed on this method, which is potentially more generally useful for callers and lets the nomad operator api
check the scheme directly.
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.
LGTM once the merge conflict is cleaned up.
- Expose internal HTTP client's Do() via Raw - Use URL parser to identify scheme - Align more with curl output - Add changelog - Fix test failure; add tests for socket envvars - Apply review feedback for tests - Consolidate address parsing - Address feedback from code reviews Co-authored-by: Tim Gross <tgross@hashicorp.com>
In #16872 we added support for configuring the API client with a unix domain socket. In order to set the host correctly, we parse the address before mutating the Address field in the configuration. But this prevents the configuration from being reused across multiple clients, as the next time we parse the address it will no longer be pointing to the socket. This breaks consumers like the autoscaler, which reuse the API config between plugins. Update the `NewClient` constructor to only override the `url` field if it hasn't already been parsed. Include a test demonstrating safe reuse with a unix domain socket. Ref: hashicorp/nomad-autoscaler#944 Ref: hashicorp/nomad-autoscaler#945
In #16872 we added support for configuring the API client with a unix domain socket. In order to set the host correctly, we parse the address before mutating the Address field in the configuration. But this prevents the configuration from being reused across multiple clients, as the next time we parse the address it will no longer be pointing to the socket. This breaks consumers like the autoscaler, which reuse the API config between plugins. Update the `NewClient` constructor to only override the `url` field if it hasn't already been parsed. Include a test demonstrating safe reuse with a unix domain socket. Ref: hashicorp/nomad-autoscaler#944 Ref: hashicorp/nomad-autoscaler#945
In #16872 we added support for configuring the API client with a unix domain socket. In order to set the host correctly, we parse the address before mutating the Address field in the configuration. But this prevents the configuration from being reused across multiple clients, as the next time we parse the address it will no longer be pointing to the socket. This breaks consumers like the autoscaler, which reuse the API config between plugins. Update the `NewClient` constructor to only override the `url` field if it hasn't already been parsed. Include a test demonstrating safe reuse with a unix domain socket. Ref: hashicorp/nomad-autoscaler#944 Ref: hashicorp/nomad-autoscaler#945
This PR is designed to pair with #16884 so that API clients can connect to the Nomad agent's HTTP api via a unix domain socket. It also includes support for the
nomad operator api
command and unix sockets.To test this PR manually, you will need to use socat as a proxy to the Nomad API socket
In another terminal
export NOMAD_ADDR=unix:///tmp/run/nomad.sock nomad namespace list
Fixes: #18099
Fixes: #4637