Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
Closed due to inactivity. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1009#issuecomment-332911517
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
Closed #1009. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1009#event-1270278806
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
@olivierlemasle do we have a path forward on this pull request? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1009#issuecomment-331355104
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
Thanks! Changes LGTM. Just pending to add the rests for the `KeystonApiExpectTest` and its base class as @zack-shoylev suggested. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1009#issuecomment-250133041
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
olivierlemasle commented on this pull request.
> @@ -234,6 +234,13 @@ public String get() {
return authenticationMethods.get(credentialType);
}
+ @Provides
+ @Singleton
+ @Named("credentialsRetryable")
+ protected final boolean credentialsRetryable(Function
getAccess) {
@nacx You're right. I've updated the pull request following your advice
(injecting `CredentialType`).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1009
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
nacx commented on this pull request.
> @@ -234,6 +234,13 @@ public String get() {
return authenticationMethods.get(credentialType);
}
+ @Provides
+ @Singleton
+ @Named("credentialsRetryable")
+ protected final boolean credentialsRetryable(Function
getAccess) {
Hmm... Nope, the code should call `credentialType.retryable()`, but it makes
more sense to object a CredentialType object than a boolean named "something".
The code is easier to read and understand if injecting the object (and easier
to extend in the future).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1009
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
zack-shoylev commented on this pull request.
> @@ -234,6 +234,13 @@ public String get() {
return authenticationMethods.get(credentialType);
}
+ @Provides
+ @Singleton
+ @Named("credentialsRetryable")
+ protected final boolean credentialsRetryable(Function
getAccess) {
So the code could directly check the type of credentials provided instead of
relying on the boolean? Makes sense.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1009
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
nacx commented on this pull request.
> @@ -234,6 +234,13 @@ public String get() {
return authenticationMethods.get(credentialType);
}
+ @Provides
+ @Singleton
+ @Named("credentialsRetryable")
+ protected final boolean credentialsRetryable(Function
getAccess) {
Better expose the `CredentialType` object instead of just a boolean
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1009#pullrequestreview-1521229
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
@nacx You're right, and I've added a commit to address that. It still binds to `retryOnRenew` (we still have to use it for HTTP 408 errors), but I've modified this RetryHandler to skip retrying in the case of a token-based authentication. @zack-shoylev I've added some basic unit tests. Please tell me if you had some specific tests on mind. Of course, I've also tested this with a live OpenStack installation (using OpenStack Kilo). -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1009#issuecomment-249317107
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
@olivierlemasle pushed 1 commit. 8679f28 Disable retry for keystone token auth + unit tests -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds/pull/1009/files/901d897d79b6a3f0459d9f5e8c3593d01633f9b6..8679f282defaf91e90c047ae661c12787bb44e3f
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
> Yes, I'm aware that the retry mechanisms will not work if the authentication > is performed with a token. In that case, we should better configure [this binding](https://github.com/olivierlemasle/jclouds/blob/master/apis/openstack-keystone/src/main/java/org/jclouds/openstack/keystone/v2_0/config/KeystoneAuthenticationModule.java#L193) **only if** the credential type is **not** `token`? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1009#issuecomment-24915
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
@olivierlemasle Yes, the code will still be quite useful for a few situations! I mostly wanted to add a comment to the code to ensure the limitations are documented. As for the tests, I think at least unit tests would be nice. For live tests, you can use devstack to test locally, but let's get the unit tests done first. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1009#issuecomment-248720659
Re: [jclouds/jclouds] Add keystone authentication with an existing token (#1009)
Not a bad idea to have that as an option! However, I have to point out that jclouds retry mechanisms will not be able to re-authenticate and get a new token once the token expires, so this type of authentication might not work in some user environments. I think it's still worth it to have it. Are there any additional tests that need to be added to this? (see https://github.com/jclouds/jclouds/blob/master/apis/openstack-keystone/src/test/java/org/jclouds/openstack/keystone/v2_0/internal/BaseKeystoneRestApiExpectTest.java) Also, always good to see more AutoValue refactoring. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1009#issuecomment-246529141
