Skip to content
This repository was archived by the owner on Aug 1, 2023. It is now read-only.

Conversation

chaolou
Copy link

@chaolou chaolou commented Oct 27, 2015

  1. Now keystone v2's identity has only a Create() method, but we need to use v2's api for dynamic auth k8s, so I add a Get() method.
  2. The reauth in client.go always fail because it forget to clear the expire X-Auth-Token and should reset after reauth successful, also the recursive ReauthFunc will never end if the admin reauth fail, so it need to clear the ReauthFunc pointer before executing ReauthFunc and restore after that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're naming these functions based on their corresponding requests, so just GetURL.

@jrperritt
Copy link
Contributor

@zyren88 The reauth stuff (like limiting the number of retries) should be handled in your app code by passing in a custom client: https://github.com/rackspace/gophercloud/blob/master/provider_client.go#L62

The change you made for resetting the token in the reauth function we can keep though.

@chaolou
Copy link
Author

chaolou commented Nov 4, 2015

@jrperritt the existing reauth logic could trap into an infinite recursive loop in some cases, such as user has change their password of keystone. It's a bug which must be fixed from my perspective, instead of an optional retry limits.

Anyway, I will rework my PR and repush soon according to your suggestion. However, I highly recommend to just reauth one time, since there is no need to retry reauth several times when get a 401 http code when call ReauthFunc(not a network problem, but a Unauthorized error!).

@chaolou chaolou closed this Nov 4, 2015
@chaolou chaolou reopened this Nov 4, 2015
@jrperritt
Copy link
Contributor

I completely understand what you're saying, and it's an edge case that some users (maybe you) will have to deal with. It is easily handled by passing in a custom `http.RoundTripper, and you can see an example of how to do it by following the link in my comment here: #498 (comment). More than one reauth attempt is sometimes necessary; I've encountered it several times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to check the error here; it'll get done in Extract.

@jrperritt
Copy link
Contributor

In addition to the above considerations, we should probably include unit and acceptance tests.

@chaolou
Copy link
Author

chaolou commented Nov 30, 2015

I have understood you said "passing in a custom `http.RoundTripper" by the example https://github.com/rackspace/gophercloud/blob/master/provider_client.go#L62
The new modification is moved to #514, so I close this one.

@chaolou chaolou closed this Nov 30, 2015
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 72.764% when pulling f6e2926 on zyren88:master into 63ee53d on rackspace:master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants