Respect proxy settings and timeout in token command #410
Conversation
| server := ctx.String("server") | ||
| if server == "" { | ||
| return errors.New("name of rancher server is required") | ||
| } | ||
| url, err := url2.Parse(server) |
There was a problem hiding this comment.
Avoid shadowing the url package.
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := cacheCredential(ctx, newCred, fmt.Sprintf("%s_%s", userID, clusterID)); err != nil { | ||
| client, err := newHTTPClient(serverConfig, tlsConfig) |
There was a problem hiding this comment.
Reuse the common http client for all subsequent requests.
|
|
||
| return token, nil | ||
|
|
||
| case <-timeout.C: | ||
| break |
There was a problem hiding this comment.
This was no-op as it doesn't break out of the loop.
|
|
||
| case <-interrupt: | ||
| customPrint("received interrupt") | ||
| break |
| var response []byte | ||
|
|
||
| req, err := http.NewRequest(method, url, body) | ||
| func doRequest(client *http.Client, req *http.Request) (*http.Response, []byte, error) { |
There was a problem hiding this comment.
Return the http.Response as the consuming code should be checking the returned status code in most/all cases.
| req.Header.Set("content-type", "application/json") | ||
| req.Header.Set("accept", "application/json") | ||
|
|
||
| client.Timeout = 300 * time.Second |
There was a problem hiding this comment.
This seems unnecessary. There is nothing specific or unusual about requesting an auth token that might require a custom 5 min timeout.
| req.Header.Set("content-type", "application/json") | ||
| req.Header.Set("accept", "application/json") | ||
|
|
||
| client.Timeout = 150 * time.Second |
There was a problem hiding this comment.
Same here. There is nothing specific or unusual about this request that might require a custom timeout.
cmd/kubectl_token.go
Outdated
| resp, respBody, err := doRequest(client, req) | ||
| if err == nil && resp.StatusCode != http.StatusOK { | ||
| err = fmt.Errorf("unexpected http status code %d", resp.StatusCode) | ||
| } |
There was a problem hiding this comment.
I feel having the err check before is more idiomatic, wdyt?
resp, respBody, err := doRequest(client, req)
if err != nil {
return token, nil
}
if resp.StatusCode != http.StatusOK {
return token, fmt.Errorf("unexpected http status code %d", resp.StatusCode)
}I'm also confused about the return token, nil. Is it ok? Should we at least log the error?
There was a problem hiding this comment.
The only idiomatic part is to explicitly check for the error if one is returned to the caller, which we do. There is no prescribed way of doing it though.
In this particular case if the request succeeds err == nil but the http status code is anything but the OK we want to fail and so we assign an error first L393-L395.
Then we cover both cases and check for err != nil.
cmd/kubectl_token_oauth.go
Outdated
| resp, respBody, err := doRequest(client, req) | ||
| if err == nil && resp.StatusCode != http.StatusOK { | ||
| return nil, fmt.Errorf("unexpected http status code %d", resp.StatusCode) | ||
| } |
There was a problem hiding this comment.
Same as above, can we invert the check?
There was a problem hiding this comment.
This was a bad copy-paste from one of the my previous iterations. We shouldn't invert anything. All we need is to set the err. Fixed in 649c29d.
Ref rancher/rancher#48631
The dedicated http clients in the
tokencommand that are used for various needs currently don't respect proxy and timeout settings.This PR makes sure that the http client is reused and respect proxy and timeout settings.