api: allow configuring http client #5275
Conversation
api/api.go
Outdated
| @@ -147,7 +147,7 @@ func (c *Config) ClientConfig(region, address string, tlsEnabled bool) *Config { | |||
| Address: fmt.Sprintf("%s://%s", scheme, address), | |||
| Region: region, | |||
| Namespace: c.Namespace, | |||
| httpClient: defaultConfig.httpClient, | |||
| HttpClient: defaultConfig.HttpClient, | |||
There was a problem hiding this comment.
ClientConfig will overwrite any custom http client with the default client. If we support setting a custom client we should propagate it to config copies.
notnoop
force-pushed
the
f-api-config-httpclient
branch
2 times, most recently
from
1d5c294 to
f278760
Compare
Allow clients to configure httpClient, e.g. set a pooled/keep-alive client. When caller configures HttpClient explicitly, we aim to use as-is; e.g. we assume it's configured with TLS already. Expose `ConfigureTLS` to aid api consumers with configuring their http client. Also, removes `SetTimeout` call that I believe is internal only and has odd side-effects when called on already created config. Also deprecates `config.ConfigureTLS` in preference to the new `ConfigureTLS`.
notnoop
force-pushed
the
f-api-config-httpclient
branch
from
f278760 to
cbc1555
Compare
There was a problem hiding this comment.
@schmichael Updated so we don't modify transports or tls config, and be able to handle client connections without modifying the initial state.
Few questions though inlined that I'd love your input on.
| func DefaultConfig() *Config { | ||
| config := &Config{ | ||
| Address: "http://127.0.0.1:4646", | ||
| httpClient: cleanhttp.DefaultClient(), |
There was a problem hiding this comment.
Previously, DefaultConfig initializes httpClient for client to use. This PR changes it so default httpClient creation occurs in NewClient instead and have it be a field of the api client rather than config.
| // SetTimeout is used to place a timeout for connecting to Nomad. A negative | ||
| // duration is ignored, a duration of zero means no timeout, and any other value | ||
| // will add a timeout. | ||
| func (c *Config) SetTimeout(t time.Duration) error { |
There was a problem hiding this comment.
I removed this function, as I believe we only intend to use internally for creating client connections and it's hard for me to imagine someone using it as-is now for other purposes; and if they do, they can set their http client.
If we don't want to remove a public function here, I can reintroduce it but with adding some additional bookkeeping to discern on applying tls config whether it's to default client with timeout or a custom http client.
| } | ||
|
|
||
| // copy all public fields, to avoid copying transient state and locks | ||
| ntr := &http.Transport{ |
There was a problem hiding this comment.
Here, I want to preserve as much of the http client the user provided (including proxies, etc). Doing copying seems to copy transient state and locks; so opted to copy over all fields explicitly. Is there a better way?
Alternatively, we can have users set a HttpClient constructor rather than a simple HttpClient, so we can modify instances easily. I opted not to, because I felt it complicated the API too much.
api/api.go
Outdated
| } | ||
|
|
||
| // ConfigureTLS applies a set of TLS configurations to the the HTTP client. | ||
| // | ||
| // Deprecated: This method is called internally. Consider using ConfigureTLS instead. |
There was a problem hiding this comment.
Consider using ConfigureTLS instead.
This method is ConfigureTLS. Should we just unexport it since we're already changing the API?
_Update: Of course 3s later I spot the ConfigureTLS func 😅. I think it's probably worth adding "func" to the comment or something though -- or unexport it.
There was a problem hiding this comment.
I'll just remove it then. API consumers can just remove invocation without any change in behavior as it's called in NewClient anyway.
`*Config.ConfigureTLS()` is invoked internally by `NewClient` and API consumers should not invoke directly. Now that http client is created in `api.NewClient`, `*Config.ConfigureTLS` makes no sense. API consumers that call it explicitly can remove the invocation and preserve the behavior.
notnoop
force-pushed
the
f-api-config-httpclient
branch
from
f4bf14a to
10ab705
Compare
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Allow clients to configure httpClient, e.g. set a pooled/keep-alive
client.
When caller configures HttpClient explicitly, we aim to use as-is; e.g.
we assume it's configured with TLS already. Expose
ConfigureTLStoaid api consumers with configuring their http client.
Also, removes
SetTimeoutcall that I believe is internal only and hasodd side-effects when called on already created config. Also deprecates
config.ConfigureTLSin preference to the newConfigureTLS.