Multiple Go client instances cannot be created if a custom HTTP client is passed

[fatih]

• Vault Version: Current HEAD
• Go Version: 1.9

Expected Behavior:
I should be able to construct multiple Client instances in a Go source code with a custom HTTP client.

Actual Behavior:
After creating the first client, no more clients can be created

Steps to Reproduce:

Add the following to api/client_test.go and run go test

func TestClientMultipleInstances(t *testing.T) {
	_, err := NewClient(&Config{
		HttpClient: &http.Client{
			Transport: http.DefaultTransport,
		},
	})
	if err != nil {
		t.Fatal(err)
	}

	// this will fail
	_, err = NewClient(&Config{
		HttpClient: &http.Client{
			Transport: http.DefaultTransport,
		},
	})
	if err != nil {
		t.Fatal(err)
	}
}

you should see the following

--- FAIL: TestClientMultipleInstances (0.00s)
	client_test.go:66: protocol https already registered

Important Factoids:

This because of the following code in NewClient() that tries to register https multiple times:

tp := c.HttpClient.Transport.(*http.Transport)
if err := http2.ConfigureTransport(tp); err != nil {
	return nil, err
}

Because of the code above, the second call to NewClient() returns an error (via http2.ConfigureTransport(tp).

[jefferai]

The best practice communicated to us from the Go team is to use the x/net/http2 package when you want to use HTTP/2. This necessarily requires calling ConfigureTransport, and that in turn is surfacing this error from inside the Go net library (net/http/transport.go).

On the one hand, I could try intercepting the error and ignoring it, but on the other hand, having that error is in theory useful in case you really didn't mean to register the same protocol twice to different handlers.

This is easy to work around though; simply pass two different transports. It's often a bad idea to use DefaultTransport with more than one client anyways. I recommend https://github.com/hashicorp/go-cleanhttp which can give you clean transports.

[fatih]

Thanks for the explanation. I've also discovered it on today's debugging journey. However I believe this could be fixed by adding this as to NewClient()

	if c.HttpClient.Transport == http.DefaultTransport {
		c.HttpClient.Transport = cleanhttp.DefaultTransport()
	}

Not many know that they shouldn't use http.Default Client and Transport. Saying that it's bad unfortunately doesn't fix the issue. I suggest to rethink about including the three line fix with the test case. Thanks.

[fatih]

Or better after the fix 42953d6, just adding the case will fix this issue:

if c.HttpClient.Transport == nil || c.HttpClient.Transport == http.DefaultTransport {
    c.HttpClient.Transport = cleanhttp.DefaultTransport()
}

[jefferai]

I don't really want to do that as there are still legitimate reasons people may want to use DefaultTransport, if they already have it set up via other means (and aren't trying to create multiple Vault API clients). We already default -- if no HTTP client is passed in, or now, if you pass in a client with no transport -- to using our own transport anyways, and most users simply pass no custom HTTP client and get cleanhttp automatically.

BTW, thanks again for vim-go :-D

[fatih]

Fair enough. You're welcome and thanks for the fix Jeff :)

[johannespetzold]

I ran into this issue while using Client.Clone(), which doesn't give me an option to pass in a different Transport instance. Is the Clone() method no longer useful because of this? Is there an alternative? I've been trying to use this because I need to make certain Vault requests with a different token, and I don't want to just SetToken() on the main client before and after the request, since that client will be used by multiple Goroutines.

[jefferai]

The clone method is perfectly useful, just not if you need a custom transport. But I don't follow why you can't use SetToken after the clone. The header is added to the request by the client, the underlying transport has nothing to do with it.

[johannespetzold]

I've been getting the protocol https already registered error when calling Clone(). Not sure if it's related, but I've been seeing this on an in-memory test client, created just like in Vault's own integration tests:

vault/api/api_integration_test.go

Line 24 in 672feed

func testVaultServer(t testing.TB) (*api.Client, func()) {

I'm most likely completely missing something here, but how can you use Clone without a specific Transport instance? Any client created with NewClient should have a transport instance associated with it (DefaultTransport or not), since it will set one even when nil was passed in via the original config. When that client is then cloned, it will always use that same transport instance, which from what I understand will always result in this error?

[johannespetzold]

Doesn't seem to be limited to the testVaultServer client either:

func TestClone(t *testing.T) {
	client1, err1 := vaultapi.NewClient(nil)
	if err1 != nil {
		t.Fatalf("NewClient failed: %v", err1)
	}
	client2, err2 := client1.Clone()
	if err2 != nil {
		t.Fatalf("Clone failed: %v", err2)
	}

	_ = client2
}

fails with Clone failed: protocol https already registered

[jefferai]

I'm most likely completely missing something here, but how can you use Clone without a specific Transport instance?

It does seem like attempting to do so is broken, but using clone is never a prerequisite. I didn't follow what your meant about Vault's integration tests, but they don't use clone.

[johannespetzold]

I'm still curious how Clone could ever work - see the TestClone example above. And if it does indeed never work, is there a recommended way for making requests to a Vault server with a different token, given an existing Vault client instance?

[jefferai]

I'm still curious how Clone could ever work

It's possible it doesn't; it's also possible it used to work and that the error coming from the http2 library is due to changed behavior.

And if it does indeed never work, is there a recommended way for making requests to a Vault server with a different token, given an existing Vault client instance?

Not currently, you'd have to create a new client.

[johannespetzold]

Thanks for confirming. My current workaround is to serialize all requests so I can safely set the token for each request using SetToken(), which may be acceptable for a while; alternatively I could create a new client for each request (since most will have a new token anyways). Do you know if there'd be any downsides to that approach (new client for each request)?

[johannespetzold]

@jefferai thanks for the rapid fix!

After pulling it in, unfortunately I'm now getting an error even earlier, when trying to spin up my in-memory test server (this is code copied verbatim from Vault's api_integration_test.go):

func TestVaultServer(t *testing.T) {
	testVaultServerBackends(t, testVaultServerDefaultBackends)
}

var testVaultServerDefaultBackends = map[string]logical.Factory{
	"transit": transit.Factory,
	"pki":     pki.Factory,
}

func testVaultServerBackends(t testing.TB, backends map[string]logical.Factory) (*api.Client, func()) {
	coreConfig := &vault.CoreConfig{
		DisableMlock:    true,
		DisableCache:    true,
		Logger:          logxi.NullLog,
		LogicalBackends: backends,
	}

	cluster := vault.NewTestCluster(t, coreConfig, &vault.TestClusterOptions{
		HandlerFunc: vaulthttp.Handler,
	})
	cluster.Start()

	// make it easy to get access to the active
	core := cluster.Cores[0].Core
	vault.TestWaitActive(t, core)

	client := cluster.Cores[0].Client
	client.SetToken(cluster.RootToken)

	// Sanity check
	secret, err := client.Auth().Token().LookupSelf()
	if err != nil {
		t.Fatal(err)
	}
	if secret == nil || secret.Data["id"].(string) != cluster.RootToken {
		t.Fatalf("token mismatch: %#v vs %q", secret, cluster.RootToken)
	}
	return client, func() { defer cluster.Cleanup() }
}

Running this test errors with:

2017/10/27 16:44:08 http2: server: error reading preface from client 127.0.0.1:51101: bogus greeting "GET /v1/auth/token/looku"
--- FAIL: TestVaultServer (2.59s)
        sandbox_test.go:46: Get https://127.0.0.1:51096/v1/auth/token/lookup-self: net/http: HTTP/1.x transport connection broken: malformed HTTP response "\x00\x00\x18\x04\x00\x00\x00\x00\x00\x00\x05\x00\x10\x00\x00\x00\x03\x00\x00\x00\xfa\x00\x06\x00\x10\x01@\x00\x04\x00\x10\x00\x00"
FAIL

Reverting the fix (2afbbb3) makes this pass again.

[jefferai]

Are you current against master? We use NewTestCluster in a ton of tests and CI is passing.

[johannespetzold]

I ran go get -u github.com/hashicorp/vault/... beforehand so I should be current.

This alternative fix seems to work (for my use case at least):

		if _, found := tp.TLSNextProto["h2"]; !found {
			if err := http2.ConfigureTransport(tp); err != nil {
				return nil, err
			}
		}

[jefferai]

I'm going to do something entirely different, but it may not be until Monday.

[jefferai]

@johannespetzold The error you were seeing appears to be a compatibility break between the built-in Go http2 support in Go 1.9 and the current version of the http2 libraries vendored in Vault. Note that currently you'll need to either keep the transport configured from DefaultConfig or you'll need to call http2.ConfigureTransport on your transport via the x/net/http2 package.

I've significantly updated the API code; see the redo-api-client branch and let me know how it works for you.

[jefferai]

Ref: golang/go#22481

[johannespetzold]

@jefferai thanks for the new fix, I can confirm that the redo-api-client version does work for me.