Add ability to dial API via unix socket #3779
Conversation
| // which would use that non-default behavior is right here, and it doesn't | ||
| // seem worth the cognitive overhead everywhere else just to serve this one | ||
| // use case. | ||
| unixSocket, err := cfg.Get("", "http_unix_socket") |
There was a problem hiding this comment.
The test data suggests that this should be a per-hostname setting which makes sense. However, that makes implementing this more complex -- we don't know what host a user wants to use until we have a request in hand we can extract a hostname from.
You're welcome to propose a refactor that will make using the socket hostname aware; otherwise, we can discuss the trade-off of making this a global setting instead of a per-host one.
There was a problem hiding this comment.
That's a good point -- thanks for raising it. How would you feel about me changing things to make it a global setting? Making it per-host does seem ideal, but I suspect it is a non-trivial refactor to get us into that position first. @mislav mentioned some work may be ongoing to that effect, but I wasn't clear what it is or where it is tracked (discussion).
In my experience at least, having it on the global config is still valuable. In fact that's how I am using it, as the code here indicates, and it seems like having a global config to start and later adding per-host overrides would be possible and let us make partial progress now. Would you be open to an update in which I make the test code align with this, and support it only at the host level?
There was a problem hiding this comment.
I'm totally fine with this being global to start 👍
There was a problem hiding this comment.
Thanks. I backed out the test assertions which suggested there was any meaning to this value on a host-level :)
One thing that I can't tell for sure is whether config entries are supposed to be specifically marked somehow as "config-only", "host-only", or "cascading", etc. I don't think there is, and IIUC the config schema itself doesn't enforce where a config can be found -- that is enforced by convention only. If I overlooked it though please let me know.
There was a problem hiding this comment.
No, there is no way to be explicit about those config categories, unfortunately -- it's convention and documentation right now.
This change adds the ability to route HTTP traffic over a unix domain socket. It does so by reading an optional configuration value,
http_unix_socket, containing the path of the socket.The code here is broken into some smaller changes to (hopefully) aid in its review. We first refactor
NewHTTPClientto allow for error returns, and add the newhttp.RoundTripperimplementation returned byhttpunix.NewRoundTripper. Next we add configuration support for the new valuehttp_unix_socket. Finally, we read and use this configuration value if present.In addition to the unit tests around configuration, this change has been used by me internally in my normal work over the last few weeks.
Fixes #3268