Skip to content
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

Go client instance cannot be created with a custom http.DefaultClient #3436

Closed
fatih opened this issue Oct 9, 2017 · 2 comments
Closed

Go client instance cannot be created with a custom http.DefaultClient #3436

fatih opened this issue Oct 9, 2017 · 2 comments

Comments

@fatih
Copy link

fatih commented Oct 9, 2017

  • Vault Version: Current HEAD
  • Go Version: 1.9

Expected Behavior:
I should be able to construct a Go client instance with Go's http.DefaultClient

Actual Behavior:
The client doesn't return an error and just panics.

Steps to Reproduce:

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

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

you should see the following

=== RUN   TestClientDefaultHttpClient
--- FAIL: TestClientDefaultHttpClient (0.00s)
panic: interface conversion: http.RoundTripper is nil, not *http.Transport [recovered]
	panic: interface conversion: http.RoundTripper is nil, not *http.Transport

goroutine 34 [running]:
testing.tRunner.func1(0xc4202bc2d0)
	/usr/local/go/src/testing/testing.go:711 +0x2d2
panic(0x19f5ee0, 0xc42070c480)
	/usr/local/go/src/runtime/panic.go:491 +0x283
github.com/hashicorp/vault/api.NewClient(0xc42070c440, 0xc42070c440, 0xc400000008, 0xc4201980c0)
	/Users/fatih/go/src/github.com/hashicorp/vault/api/client.go:271 +0x379
github.com/hashicorp/vault/api.TestClientDefaultHttpClient(0xc4202bc2d0)
	/Users/fatih/go/src/github.com/hashicorp/vault/api/client_test.go:51 +0x5c
testing.tRunner(0xc4202bc2d0, 0x1b696f8)
	/usr/local/go/src/testing/testing.go:746 +0xd0
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:789 +0x2de
exit status 2
FAIL	github.com/hashicorp/vault/api	0.163s

Important Factoids:

This because of the following code in NewClient() that tries to access .Transport directly:

tp := c.HttpClient.Transport.(*http.Transport)

The Go doc for this http.Client has the following:

type Client struct {
	// Transport specifies the mechanism by which individual
	// HTTP requests are made.
	// If nil, DefaultTransport is used.
	Transport RoundTripper
...
}

However the statement If nil, DefaultTransport is used, is only valid inside the net/http package. The net/http has the following method internally that makes sure it returns a non-nil transport (from $GOROOT/src/net/http/client.go:192:

func (c *Client) transport() RoundTripper {
	if c.Transport != nil {
		return c.Transport
	}
	return DefaultTransport
}

References:

When I've tried to fix it by passing a http.DefaultTransport, I've encountered another issue, which is reported here: #3435

@jefferai
Copy link
Member

jefferai commented Oct 9, 2017

I'll fix the panic, but how come you're trying to construct a client with http.DefaultClient? This is often a bad thing to do.

@fatih
Copy link
Author

fatih commented Oct 9, 2017

I'll fix the panic, but how come you're trying to construct a client with http.DefaultClient? This is often a bad thing to do.

I know it's bad but our case is totally valid (and probably for many others as well).

Our internal client fallbacks to http.DefaultClient when no custom clients is passed. And this is always the case for our Go tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants