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.
| @@ -214,20 +228,15 @@ func deleteCachedCredential(ctx *cli.Context) error { | |||
| for _, key := range ctx.Args() { | |||
| customPrint(fmt.Sprintf("removing [%s]", key)) | |||
| for _, server := range cf.Servers { | |||
| server.KubeCredentials[key] = nil | |||
There was a problem hiding this comment.
This was prone to panics.
| return token, nil | ||
| } | ||
|
|
||
| func samlAuth(input *LoginInput, client *http.Client) (managementClient.Token, error) { | ||
| func samlAuth(client *http.Client, input *LoginInput) (managementClient.Token, error) { |
There was a problem hiding this comment.
The idiomatic approach is to list arguments in the order of increasing specificity. This way all dependencies come first.
| // timeout for user to login and get token | ||
| timeout := time.NewTicker(15 * time.Minute) | ||
| // Timeout for the login flow. | ||
| timeout := time.NewTimer(15 * time.Minute) |
There was a problem hiding this comment.
Use the time.Timer instead as we expect to receive the overall flow timeout signal only once.
There was a problem hiding this comment.
I feel there is a better way to do this poll and timeout, but I wouldn't try to change the logic without tests here.
|
|
||
| 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.
| if ctx.Bool("delete") { | ||
| return deleteCachedCredential(ctx) | ||
| } |
There was a problem hiding this comment.
Seems like it might've been a flag in the past, which is no longer true. We use the delete subcommand for this.
There was a problem hiding this comment.
One of the reason because we should try to avoid passing the *cli.Context, and have a Config struct to pass. But that's a refactor for another day.
There was a problem hiding this comment.
IMHO passing the context or not is a different story.
The issue here is that it seems like that delete was a flag
rancher token --delete <name>
and then turned into an argument (subcommand)
rancher token delete <name>
| 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.
| 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.
| // timeout for user to login and get token | ||
| timeout := time.NewTicker(15 * time.Minute) | ||
| // Timeout for the login flow. | ||
| timeout := time.NewTimer(15 * time.Minute) |
There was a problem hiding this comment.
I feel there is a better way to do this poll and timeout, but I wouldn't try to change the logic without tests here.
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.