You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Svet <no...@github.com> on 2017/07/11 09:25:26 UTC
[jclouds/jclouds] Use separate credential stores per context (#1119)
With a shared credential store the configuration of one compute service leaks in all others, causing the wrong credentials to be used when not overridden.
The particular problem this fixes - [Azure sets default username/password for images](https://github.com/jclouds/jclouds-labs/blob/master/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/AzureComputeProviderMetadata.java#L98) which leaks to other providers (through [GetLoginForProviderFromPropertiesAndStoreCredentialsOrReturnNull](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/config/GetLoginForProviderFromPropertiesAndStoreCredentialsOrReturnNull.java#L51)), causing the wrong username/password to be used.
Even if the credentials are scoped to a specific provider in the map, it still will cause cross-talk between differently configured compute services. The cache should only apply to the current compute service.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1119
-- Commit Summary --
* Use separate credential stores per context
-- File Changes --
M core/src/main/java/org/jclouds/rest/config/CredentialStoreModule.java (11)
M core/src/test/java/org/jclouds/rest/CredentialStoreModuleTest.java (42)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1119.patch
https://github.com/jclouds/jclouds/pull/1119.diff
--
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/1119
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Svet <no...@github.com>.
@nacx That's a great idea, thanks for the example.
Don't disagree having the shared cache could be useful. It's the way it's used in [GetLoginForProviderFromPropertiesAndStoreCredentialsOrReturnNull](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/config/GetLoginForProviderFromPropertiesAndStoreCredentialsOrReturnNull.java) that I don't like. And that's used in all providers.
Having the configuration of one compute service affect all remaining compute services even when they have different configuration is surprising to say the least :)
I still think that it's the wrong thing to do. Perhaps it should be the other way around - users which really need the cross-service cache should inject it themselves.
Can you give some good examples of using the cache? From looking at the code it seems that its not even needed - no heavy computation is done to extract the username/password in the default case.
--
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/1119#issuecomment-315095395
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Andrew Gaul <no...@github.com>.
> I'm OK with making the per-context store the default one. My only concern is that users might be relying on this without really knowing it.
When I removed a similar static from the transient blobstore in 4ac7629f44e70bf4fd4a17da541745a9769fcdb1 one user complained about it. I can not find the reference but we should mention the changed behavior in the release notes at least.
--
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/1119#issuecomment-315434717
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Andrew Gaul <no...@github.com>.
andrewgaul commented on this pull request.
> @@ -59,13 +59,8 @@ protected void configure() {
}).to(CredentialsToJsonByteSource.class);
bind(new TypeLiteral<Function<ByteSource, Credentials>>() {
}).to(CredentialsFromJsonByteSource.class);
- if (backing != null) {
- bind(new TypeLiteral<Map<String, ByteSource>>() {
- }).toInstance(backing);
- } else {
- bind(new TypeLiteral<Map<String, ByteSource>>() {
- }).toInstance(BACKING);
Remove unused `BACKING`?
--
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/1119#pullrequestreview-49554402
Re: [jclouds/jclouds] JCLOUDS-1321: Use separate credential stores
per context (#1119)
Posted by Ignasi Barrera <no...@github.com>.
Good to merge this and publish the changes to the site with the docs?
--
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/1119#issuecomment-317335744
Re: [jclouds/jclouds] JCLOUDS-1321: Use separate credential stores
per context (#1119)
Posted by Svet <no...@github.com>.
* Jira (to be included in release notes): https://issues.apache.org/jira/browse/JCLOUDS-1321
* Heads up to the mailing lists:
* [user@j.a.o](https://lists.apache.org/thread.html/c65711e3ca3b3ffe068fc6562f0db98336772984924f24857ee0bdcd@%3Cuser.jclouds.apache.org%3E)
* [dev@j.a.o](https://lists.apache.org/thread.html/b3edeca6b742d122908c570dd779af4bb1c93c6405e0bf5b52c17267@%3Cdev.jclouds.apache.org%3E)
* Documenting a work around in the [compute guide](https://github.com/jclouds/jclouds-site/pull/200)
--
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/1119#issuecomment-315765265
Re: [jclouds/jclouds] JCLOUDS-1321: Use separate credential stores
per context (#1119)
Posted by Svet <no...@github.com>.
Web site updated.
--
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1119#issuecomment-318432246
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Ignasi Barrera <no...@github.com>.
I'm OK with making the per-context store the default one. My only concern is that users might be relying on this without really knowing it.
If we do that, let's make sure we properly document in the release notes how to keep the old behavior. We should also add, as soon as we merge the change, a `credential store` section to the [jclouds compute guide](http://jclouds.apache.org/start/compute/) to explain what it is, what it is used for, and how you can leverage the shared store.
--
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/1119#issuecomment-315324242
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Andrea Turli <no...@github.com>.
Same here it is very surprising. I agree with @neykov the default should probably be an isolated credentials store, and give the user the ability to override if they really know what they are doing!
--
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/1119#issuecomment-315145076
Re: [jclouds/jclouds] JCLOUDS-1321: Use separate credential stores
per context (#1119)
Posted by Svet <no...@github.com>.
+1. I'll merge this and publish the changes by the end of the week due to passive consensus.
--
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/1119#issuecomment-317720168
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Andrew Phillips <no...@github.com>.
> give the user the ability to override if they really know what they are doing!
Worth asking in `user@` and/or `dev@` how many users are aware of and/or are using the feature? Irrespective of whether this is surprising or not, given that it's a breaking change it would be useful to know what the (intentional) usage out there of this is..?
--
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/1119#issuecomment-315229496
Re: [jclouds/jclouds] JCLOUDS-1321: Use separate credential stores
per context (#1119)
Posted by Svet <no...@github.com>.
Merged to [master](https://git1-us-west.apache.org/repos/asf/jclouds/?p=jclouds.git;a=commit;h=2487b0c5132191de5e68413e33dfa551b5679ebb).
--
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1119#issuecomment-318342375
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Ignasi Barrera <no...@github.com>.
I'm not sure we should fix it this way and remove the static. The default static backing map was chosen as a practical way to provide some basic "credential persistence" and allow credentials to be cached and survive context creation/destruction, which is convenient in many cases.
There is a trivial configuration to be applied when you need context isolation. Just create the context with a `CredentialStoreModule` initialized with a local `Map`. It has a public constructor just for this purpose.
```java
Module isolatedCredStore = new CredentialStoreModule(new ConcurrentHashMap<String, ByteSource>());
ContextBuilder.newBuilder("provider")
.modules(ImmutableSet.of(isolatedCredStore, ...))
...
```
I think the current default is a good compromise of convenience vs conflicting use cases (since most users will be using one single account for each provider), and the existing implementation already provides a clean way to isolate the credential store between contexts. I don't see the need to change that default.
--
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/1119#issuecomment-315006631
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Svet <no...@github.com>.
neykov commented on this pull request.
> @@ -59,13 +59,8 @@ protected void configure() {
}).to(CredentialsToJsonByteSource.class);
bind(new TypeLiteral<Function<ByteSource, Credentials>>() {
}).to(CredentialsFromJsonByteSource.class);
- if (backing != null) {
- bind(new TypeLiteral<Map<String, ByteSource>>() {
- }).toInstance(backing);
- } else {
- bind(new TypeLiteral<Map<String, ByteSource>>() {
- }).toInstance(BACKING);
Good spot, removed.
--
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/1119#discussion_r127129277
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Svet <no...@github.com>.
These are all great ideas @nacx. I'll do the necessary updates.
I'll send an email to the mailing list so the change gets wider exposure.
It's also a breaking change so should be merged only on master.
--
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/1119#issuecomment-315331196
Re: [jclouds/jclouds] Use separate credential stores per context
(#1119)
Posted by Andrew Gaul <no...@github.com>.
I find the shared credential store surprising. Instead users should configure via the constructor parameter if they desire this behavior?
--
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/1119#issuecomment-315126326