-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Improving Handling of Unix Domain Socket Addresses #11904
Conversation
changelog/11904.txt
Outdated
@@ -0,0 +1,3 @@ | |||
```release-note:bug | |||
api: extracting Unix domain socket handling to separate function |
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.
I would describe the change from the user perspective, i.e. what's the behavioural change? If there's none, probably no changelog is needed. In your case, I would say something like "support UNIX domain sockets in Client.SetAddress".
api/client.go
Outdated
@@ -519,14 +530,14 @@ func (c *Client) SetAddress(addr string) error { | |||
c.modifyLock.Lock() | |||
defer c.modifyLock.Unlock() | |||
|
|||
parsedAddr, err := url.Parse(addr) | |||
c.config.modifyLock.Lock() |
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 bit should probably be reverted: parse the address, return err if any, if not then grab the config's lock and store the resulting address.
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.
There's more than just storing the address, though. The ParseAddress function will also modify the c.config.HTTPClient.Transport.
api/client.go
Outdated
u.Path = "" | ||
} else { | ||
transport := config.HttpClient.Transport.(*http.Transport) | ||
transport.DialContext = (&net.Dialer{ |
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 doesn't look quite like how we setup the transport in DefaultConfig.
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.
I went deeper than DefaultConfig and peeked into cleanhttp.DefaultPooledTransport (DefaultConfig uses cleanhttp.DefaultPooledClient, which uses cleanhttp.DefaultPooledTransport). In general, I don't really like this, since I'm getting way down into the implementation details of cleanhttp. However, I don't want to squash any of the other unaffected fields of the Transport.
Hi @marcboudreau, This looks worthwhile to me, thanks for working on it. |
I think that the last commit that I pushed addresses what I was stumbling on, which was there was a requirement to allow a http.RoundTripper that's not an http.Transport as the value of the c.HttpClient.Transport field (based on the test function TestClientNonTransportRoundTripper). |
I'm still thinking about how to deal with the reported race conditions. |
I think this is ready for review now. |
@@ -317,12 +315,6 @@ func (c *Config) ReadEnvironment() error { | |||
if err != nil { | |||
return fmt.Errorf("could not parse VAULT_SKIP_VERIFY") | |||
} | |||
} else if v := os.Getenv(EnvVaultInsecure); v != "" { |
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.
What's the rationale for removing this?
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.
Because both the constants EnvVaultInsecure and EnvVaultSkipVerify are set to the same value: VAULT_SKIP_VERIFY
. So the else if check was testing for the exact same condition as the if check.
api/client.go
Outdated
// ParseAddress transforms the provided address into a url.URL and handles | ||
// the case of Unix domain sockets by setting the DialContext in the | ||
// configuration's HttpClient.Transport. | ||
func ParseAddress(address string, config *Config) (*url.URL, error) { |
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.
I wonder whether it would be better to make this a method on Config
, similar to ConfigureTLS
- say Config.ConfigureTransport
. That way it would be less surprising that it had other side-effects which mutate the Config.
Alternatively, what about making the DialContext
currently being assigned to transport
be another return value?
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.
Yeah, I'm totally open to changing exploring alternatives. This looked straightforward at first and just kept getting uglier from there. I'll explore those two ideas and see what it looks like.
api/client.go
Outdated
// address in the Config did, change the transport's DialContext back to | ||
// use the default configuration that cleanhttp uses. | ||
if transport, ok := config.HttpClient.Transport.(*http.Transport); ok { | ||
transport.DialContext = (&net.Dialer{ |
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.
Instead of hardcoding this, why not use cleanhttp.DefaultPooledTransport().DialContext
?
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.
Of course that assumes that the user didn't provide a nonstandard HttpClient
in their Config
. I wonder if NewClient should copy the DialContext
from the config it's given so that SetAddress
can access it in this scenario?
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.
On your first comment, yes I like that a lot better. On the second comment, this is what I've struggled with on this PR. The current code unconditionally mutates the DialContext when the address being set starts with unix://
. I tried reducing the number of code paths where the DialContext is overwritten. Since DialContext is set to a function, I just don't know of a good way to determine if the current DialContext is adequate or needs to be changed.
@ncabatoff Thanks for the feedback. I will explore your suggestions and to see where it leads me. |
I've pushed changes that makes the ParseAdress a method of Config. If this approach looks good, I'll take a closer look at the current tests to see if any more are needed. |
@marcboudreau would you please merge in the latest changes in main, and make sure the conflicts are resolved? |
Hi @marcboudreau - just a nudge to see if you'd have a chance to re-base off of main and resolve these conflicts, and we can get this merged. Thanks in advance! |
@hsimon-hashicorp Hi, I'm sorry that I didn't notice the activity on this PR until now. I will rebase the code now. |
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.
@marcboudreau Thanks for working on this! I left a few minor comments. I think @ncabatoff still has an outstanding comment that needs resolving around the changelog. Once those are all taken care of, I think we can merge this!
api/client.go
Outdated
@@ -1068,8 +1085,8 @@ func (c *Client) clone(cloneHeaders bool) (*Client, error) { | |||
defer c.modifyLock.RUnlock() | |||
|
|||
config := c.config | |||
config.modifyLock.RLock() | |||
defer config.modifyLock.RUnlock() |
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.
Why is this being changed from a read lock to a write lock?
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.
It's been way too long for me to remember my rational, but looking at it now, I agree with you. This change makes no sense.
…ed Unix domain socket logic to function, and made use of this logic in SetAddress. Adjusted unit tests to verify proper Unix domain socket handling.
f8c8f32
to
8d51741
Compare
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.
Thanks for working on this @marcboudreau!
This PR started out as a fix for issue #11865. After realizing that not much can be done to address the issue, it shifted into an attempt to provide a workaround. That lead to extracting the Unix domain socket handling found in the NewClient(*Config) function into its own function, ParseAddress(string, *Config). In doing so, it became apparent that if a Client is originally created using a TCP based address but then the SetAddress is used to change to a Unix domain socket, the DialContext function in the HttpClient.Transport won't be updated as it is in the NewClient(*Config) function.
The current state of this PR has the SetAddress(string) function also using this function, however, in order to support changing from a Unix domain socket back to a TCP based address, the best that can be done is setting the DialContext function the same way that the cleanhttp.DefaultPooledTransport() function does.
This has one unit test failing, because of a panic. But before looking at addressing that, I like some feedback as to whether this is worthwhile or if I'm just going down a rabbit hole.