Improving Handling of Unix Domain Socket Addresses #11904

marcboudreau · 2021-06-21T01:36:59Z

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.

ncabatoff · 2021-06-21T12:22:58Z

changelog/11904.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+api: extracting Unix domain socket handling to separate function


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".

ncabatoff · 2021-06-21T12:24:24Z

api/client.go

@@ -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()


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's more than just storing the address, though. The ParseAddress function will also modify the c.config.HTTPClient.Transport.

ncabatoff · 2021-06-21T12:27:47Z

api/client.go

+		u.Path = ""
+	} else {
+		transport := config.HttpClient.Transport.(*http.Transport)
+		transport.DialContext = (&net.Dialer{


This doesn't look quite like how we setup the transport in DefaultConfig.

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.

ncabatoff · 2021-06-21T12:28:44Z

I like some feedback as to whether this is worthwhile or if I'm just going down a rabbit hole.

Hi @marcboudreau,

This looks worthwhile to me, thanks for working on it.

marcboudreau · 2021-06-26T18:08:27Z

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 think the error message I introduced on https://github.com/hashicorp/vault/pull/11904/files#diff-aa9bfd1a638fbb706f8e8920297902937011160319d9679add5dca56e5ab8277R490 could definitely use improvement.

marcboudreau · 2021-06-28T18:42:28Z

I'm still thinking about how to deal with the reported race conditions.

marcboudreau · 2021-07-03T19:32:37Z

I think this is ready for review now.

ncabatoff · 2021-07-05T16:37:37Z

api/client.go

@@ -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 != "" {


What's the rationale for removing this?

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.

ncabatoff · 2021-07-05T16:58:50Z

api/client.go

+// 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) {


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?

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.

ncabatoff · 2021-07-05T17:05:20Z

api/client.go

+		// 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{


Instead of hardcoding this, why not use cleanhttp.DefaultPooledTransport().DialContext?

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?

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.

marcboudreau · 2021-07-05T17:52:56Z

@ncabatoff Thanks for the feedback. I will explore your suggestions and to see where it leads me.

marcboudreau · 2021-07-22T13:07:21Z

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.

hghaf099 · 2021-10-12T19:59:27Z

@marcboudreau would you please merge in the latest changes in main, and make sure the conflicts are resolved?

heatherezell · 2021-12-10T21:39:18Z

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!

marcboudreau · 2022-06-07T21:07:38Z

@hsimon-hashicorp Hi, I'm sorry that I didn't notice the activity on this PR until now. I will rebase the code now.

raskchanky

@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!

raskchanky · 2022-06-13T19:05:44Z

api/client.go

@@ -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()


Why is this being changed from a read lock to a write lock?

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.

api/client.go

…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.

raskchanky

Thanks for working on this @marcboudreau!

vercel bot temporarily deployed to Preview – vault June 21, 2021 01:37 Inactive

marcboudreau marked this pull request as draft June 21, 2021 01:37

marcboudreau changed the title ~~[DRAFT] Improving Handling of Unix Domain Socket Addresses~~ Improving Handling of Unix Domain Socket Addresses Jun 21, 2021

vercel bot temporarily deployed to Preview – vault June 21, 2021 01:45 Inactive

ncabatoff reviewed Jun 21, 2021

View reviewed changes

vercel bot temporarily deployed to Preview – vault June 26, 2021 18:01 Inactive

marcboudreau marked this pull request as ready for review June 26, 2021 18:08

marcboudreau marked this pull request as draft June 26, 2021 18:31

vercel bot temporarily deployed to Preview – vault June 26, 2021 19:21 Inactive

vercel bot temporarily deployed to Preview – vault July 3, 2021 18:53 Inactive

marcboudreau marked this pull request as ready for review July 3, 2021 19:30

ncabatoff reviewed Jul 5, 2021

View reviewed changes

vercel bot temporarily deployed to Preview – vault-storybook July 21, 2021 23:53 Inactive

vercel bot temporarily deployed to Preview – vault July 21, 2021 23:53 Inactive

vercel bot temporarily deployed to Preview – vault August 14, 2021 00:20 Inactive

vercel bot temporarily deployed to Preview – vault-storybook August 14, 2021 00:20 Inactive

heatherezell added the waiting-for-response label Oct 12, 2021

nomaster mentioned this pull request Nov 26, 2021

VAULT_ADDR should allow sockets #8378

Closed

marcboudreau force-pushed the issue-11865 branch from 826768b to f8c8f32 Compare June 7, 2022 21:31

heatherezell added bug Used to indicate a potential bug and removed waiting-for-response labels Jun 8, 2022

raskchanky reviewed Jun 14, 2022

View reviewed changes

raskchanky linked an issue Jun 14, 2022 that may be closed by this pull request

golang vault client always validates config vars, even if passed config struct #11865

Closed

marcboudreau added 11 commits June 18, 2022 17:22

Removed redundant checks for same env var in ReadEnvironment, extract…

8aeb3b9

…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.

Adding case to revert from Unix domain socket dial function back to TCP

fd45a7d

Adding changelog file

4da0ee6

Only adjust DialContext if RoundTripper is an http.Transport

682f85c

Switching from read lock to normal lock

c437a66

only reset transport DialContext when setting different address type

b2f3347

made ParseAddress a method on Config

a2848c6

Adding additional tests to cover transitions to/from TCP to Unix

f24470a

Moved Config type method ParseAddress closer to type's other methods.

97dad83

make release note more end-user focused

0aa8613

adopt review feedback to add comment about holding a lock

8d51741

marcboudreau force-pushed the issue-11865 branch from f8c8f32 to 8d51741 Compare June 19, 2022 00:17

raskchanky approved these changes Jun 21, 2022

View reviewed changes

raskchanky merged commit d530550 into hashicorp:main Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving Handling of Unix Domain Socket Addresses #11904

Improving Handling of Unix Domain Socket Addresses #11904

marcboudreau commented Jun 21, 2021

ncabatoff Jun 21, 2021

ncabatoff Jun 21, 2021

marcboudreau Jun 21, 2021

ncabatoff Jun 21, 2021

marcboudreau Jun 21, 2021

ncabatoff commented Jun 21, 2021

marcboudreau commented Jun 26, 2021

marcboudreau commented Jun 28, 2021 •

edited

Loading

marcboudreau commented Jul 3, 2021

ncabatoff Jul 5, 2021

marcboudreau Jul 5, 2021

ncabatoff Jul 5, 2021

marcboudreau Jul 5, 2021

ncabatoff Jul 5, 2021

ncabatoff Jul 5, 2021

marcboudreau Jul 5, 2021

marcboudreau commented Jul 5, 2021

marcboudreau commented Jul 22, 2021

hghaf099 commented Oct 12, 2021

heatherezell commented Dec 10, 2021

marcboudreau commented Jun 7, 2022

raskchanky left a comment

raskchanky Jun 13, 2022

marcboudreau Jun 18, 2022

raskchanky left a comment

		@@ -0,0 +1,3 @@
		```release-note:bug
		api: extracting Unix domain socket handling to separate function

Improving Handling of Unix Domain Socket Addresses #11904

Improving Handling of Unix Domain Socket Addresses #11904

Conversation

marcboudreau commented Jun 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncabatoff commented Jun 21, 2021

marcboudreau commented Jun 26, 2021

marcboudreau commented Jun 28, 2021 • edited Loading

marcboudreau commented Jul 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcboudreau commented Jul 5, 2021

marcboudreau commented Jul 22, 2021

hghaf099 commented Oct 12, 2021

heatherezell commented Dec 10, 2021

marcboudreau commented Jun 7, 2022

raskchanky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raskchanky left a comment

Choose a reason for hiding this comment

marcboudreau commented Jun 28, 2021 •

edited

Loading