You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2014/11/29 23:20:04 UTC
[jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the OkHttp
driver (#619)
Makes the ConnectionSpecs injectable so providers and users can provide a custom one.
This should also fix [JCLOUDS-753](https://issues.apache.org/jira/browse/JCLOUDS-753), as the change provides a clean way of setting the supported TLS protocols.
/cc @adriancole @andreaturli
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds okhttp-tls
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/619
-- Commit Summary --
* JCLOUDS-753: Make ConnectionSpec configurable in the OkHttp driver
-- File Changes --
M core/src/test/java/org/jclouds/http/BaseMockWebServerTest.java (9)
M drivers/okhttp/src/main/java/org/jclouds/http/okhttp/config/OkHttpCommandExecutorServiceModule.java (16)
M drivers/okhttp/src/test/java/org/jclouds/http/okhttp/OkHttpCommandExecutorServiceTest.java (92)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/619.patch
https://github.com/jclouds/jclouds/pull/619.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1425](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1425/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-64968750
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Adrian Cole <no...@github.com>.
suggestion on how to rejig out of needing to use guice inheritance.
sidebar..
Another way, which could avoid constantly punching holes could be to allow the user a callback to customize the base okhttp client.
Ex.
```java
interface ConfigureOkHttpClient {
/** Use this to customize the base okhttp client used for all connection. For example, set a custom ssl context, cache, or connection specs here. **/
OkHttpClient beforeProvides(OkHttpClient client);
}
```
Then, you could use set bindings on something similarly named to above to ensure that providers like docker set the ssl context in a way that doesn't require inheritance, optional injection, etc, yet still allowing users to customize further as needed. Only problem is that not all set bindings retain insertion order, so if both the base provider tried to alter protocols and also the user did.. it may be unclear who would win. That's the downside of using more than one callback with set bindings.
Another way is to allow only one to be configured, and use composition instead.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-64991836
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1427](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1427/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65003472
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -85,12 +85,19 @@ protected static MockWebServer mockWebServer(Dispatcher dispatcher) throws IOExc
> * Creates a test api for the given class and URL.
> */
> protected <T extends Closeable> T api(Class<T> apiClass, String url) {
> + return api(apiClass, url, createConnectionModule());
> + }
> +
> + /**
> + * Creates a test api for the given class, URI and Module.
> + */
> + protected <T extends Closeable> T api(Class<T> apiClass, String url, Module... connectionModules) {
Note that this class exists only in the test jar, so it would be redundant.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619/files#r21222696
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Adrian Cole <no...@github.com>.
I have commented many times about ssl context, that it isn't actually
exposed in all http drivers, and that we should beware of attempting to own
ssl configuration. Ex as mentioned before the optional injection for
default ssl context is only labs and not in apachehc.
Http drivers should be a gateway to allowing the user to provide
configuration, not an attempt to be portable on all things.
I am not going to repost the same position again.
Anyway +1 ssl aside
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65076730
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Andrew Phillips <no...@github.com>.
> @@ -50,6 +50,11 @@
> <scope>test</scope>
> </dependency>
> <dependency>
> + <groupId>com.google.inject.extensions</groupId>
> + <artifactId>guice-multibindings</artifactId>
> + <version>3.0</version>
[minor] Indent. We can clean this up during the merge.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619/files#r21065168
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Andrew Phillips <no...@github.com>.
@nacx: Just some minor comments from me. As regards the question you and @adriancole have been discussing: I like the suggestion of saying "we'll configure safe stuff for the provider you choose, but if you want to make arbitrary other configuration changes to the HTTP client, here you go!"
This allows jclouds to get rid of the responsibility of correctly handling SSL configuration based on a provided SSLContext that effectively becomes a "config spec", and also allows for all kinds of other weird and wonderful HTTP client configurations that an API, provider or end user might need.
As you say, though, SSLContext _is_ what is being used currently, so as long as we have an idea of how we want to do this "properly" going forward, I'd be OK with getting this in now as work-in-progress and then moving on to a different PR that moves to a different style of non-default HTTP client configuration.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65006775
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Andrew Phillips <no...@github.com>.
> What if instead, OkHttpClientProvider used OkHttpClientSupplier first
I guess all we would have to ensure in that case is that we don't stomp on any changes/configuration settings the user has made in a custom `OkHttpClientSupplier`. Nice!
Thanks, @adriancole!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65185232
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1971](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1971/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65167725
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Andrew Phillips <no...@github.com>.
> @@ -85,12 +85,19 @@ protected static MockWebServer mockWebServer(Dispatcher dispatcher) throws IOExc
> * Creates a test api for the given class and URL.
> */
> protected <T extends Closeable> T api(Class<T> apiClass, String url) {
> + return api(apiClass, url, createConnectionModule());
> + }
> +
> + /**
> + * Creates a test api for the given class, URI and Module.
> + */
> + protected <T extends Closeable> T api(Class<T> apiClass, String url, Module... connectionModules) {
[minor] Do we intend this to be `@VisibleForTesting` or generally used?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619/files#r21203553
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #1424](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1424/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-64969406
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Andrew Phillips <no...@github.com>.
Thanks, @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65290826
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Ignasi Barrera <no...@github.com>.
I thought about where to hook the user config and chose to put it at the end, to make sure it won't be changes by jclouds. This will also help changing and improving the driver without worrying about breaking user customizations.
I like the supplier approach, but there are properties, such as timeouts or the follow redirects flag, that are primitives. We won't be able to know if they have been set by the user supplier or not and we might be changing its config by mistake.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65199507
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Andrea Turli <no...@github.com>.
Big +1
Thanks @nacx I'll test it asap with Docker!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65272200
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Adrian Cole <no...@github.com>.
> private final HostnameVerifier verifier;
> private final Supplier<SSLContext> untrustedSSLContextProvider;
> private final HttpUtils utils;
>
> + @Inject(optional = true)
> + private Supplier<SSLContext> sslContextSupplier;
> +
> + @Inject(optional = true)
optional injection is something we should move away from. [set bindings](http://google.github.io/guice/api-docs/latest/javadoc/index.html?com/google/inject/multibindings/Multibinder.html
) are supported in guice (even 4) and new injectors like [dagger](https://github.com/google/dagger/blob/master/core/src/main/java/dagger/Provides.java#L49). It is probably better to use that instead, particularly for things like this.
Ex. add a ctor param for `Set<ConnectionSpec> connectionSpecs`. In the okhttp module, use Multibinder.newSetBinder [without binding anything](https://groups.google.com/forum/#!topic/google-guice/5Rnm-d7MU34). This will apply no specs as defaults, and allow users to supply additional specs without using optional injection or module overrides. In the ctor, only apply connectionSpecs if not empty.
p.s. Another example of things to move to set bindings are [authentication methods](https://github.com/jclouds/jclouds/blob/master/apis/openstack-swift/src/main/java/org/jclouds/openstack/swift/v1/config/SwiftAuthenticationModule.java#L76), as that eliminates needing to use inheritance just to add another mechanism in a subtype.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619/files#r21061573
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1968](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1968/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65003004
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Andrew Phillips <no...@github.com>.
> @@ -85,12 +85,19 @@ protected static MockWebServer mockWebServer(Dispatcher dispatcher) throws IOExc
> * Creates a test api for the given class and URL.
> */
> protected <T extends Closeable> T api(Class<T> apiClass, String url) {
> + return api(apiClass, url, createConnectionModule());
> + }
> +
> + /**
> + * Creates a test api for the given class, URI and Module.
> + */
> + protected <T extends Closeable> T api(Class<T> apiClass, String url, Module... connectionModules) {
> Note that this class exists only in the test jar, so it would be redundant.
Doh! Thanks for pointing that out...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619/files#r21231808
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1966](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1966/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-64970631
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Ignasi Barrera <no...@github.com>.
I've updated the PR to use multi bindings to provide the connection specs.
Regarding the `Supplier<SSLContext>` optional injection, my main concern about providing a callback and removing the supplier is that the supplier approach is used in the other drivers. I agree that it would be more flexible to provide a specific callback for each driver, and even delete the "untrusted" configuration and delegate that configuration to callers, but that has the downside of having N different ways to configure the SSL connections, one for each driver.
In the end, SSL configuration is common to all HTTP drivers, and that config happens in the SSLContext in all them, so it makes sense to me to have a way of just providing a custom SSLContext, and the current `Supplier<SSLContext>` seems convenient to me. That should not be incompatible with creating post-configuration callbacks for each driver to let users have specific configurations, but having this common way of configuring SSL seems convenient to me. Is the real problem you see the optional injection, or the fact that the injection provides a gate just to configure a concrete thing in the HTTP driver?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65001268
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Ignasi Barrera <no...@github.com>.
> private final HostnameVerifier verifier;
> private final Supplier<SSLContext> untrustedSSLContextProvider;
> private final HttpUtils utils;
>
> + @Inject(optional = true)
> + private Supplier<SSLContext> sslContextSupplier;
> +
> + @Inject(optional = true)
Oh, that makes sense. This approach is already used in Chef to [bind Ohai info](https://github.com/jclouds/jclouds/blob/master/apis/chef/src/main/java/org/jclouds/ohai/config/OhaiModule.java#L80-L88) and makes perfect sense to use it here too. Thanks for the suggestion!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619/files#r21063124
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Adrian Cole <no...@github.com>.
@nacx so I didn't think of an easier way out before.
So, right now, there's an okhttp client provider, and this news up an OkHttpClient. As a last step, it configures it.
What if instead, OkHttpClientProvider used OkHttpClientSupplier first, as opposed to ConfigureOkHttpClient last? The interface would be much simpler.
```java
@ImplementedBy(NewOkHttpClient.class)
interface OkHttpClientSupplier {
/** Returns a new instance, with any TLS configuration needed for all requests. */
OkHttpClient get();
static final class NewOkHttpClient implements OkHttpClientSupplier {
public OkHttpClient get() {
return new OkHttpClient();
}
}
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65172414
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Adrian Cole <no...@github.com>.
Again, my suggestion is to limit this to api config, not general purpose
settings such as timeouts, and also to not document this as a "user"
feature rather an internal hook for api writing.
It gets complicated otherwise, as you've mentioned, and the primary goal is
to address tls settings for docker and azure types.
Starting with an internal beta approach solves the TLS problem simply and
allows us to wait for users to ask before we make it more complicated.
Right now, no user has asked to configure okhttp directly yet.
Make sense?
On Dec 2, 2014 3:53 AM, "Ignasi Barrera" <no...@github.com> wrote:
> I thought about where to hook the user config and chose to put it at the
> end, to make sure it won't be changes by jclouds. This will also help
> changing and improving the driver without worrying about breaking user
> customizations.
>
> I like the supplier approach, but there are properties, such as timeouts
> or the follow redirects flag, that are primitives. We won't be able to know
> if they have been set by the user supplier or not and we might be changing
> its config by mistake.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/jclouds/jclouds/pull/619#issuecomment-65199507>.
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65227422
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1972](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1972/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65264416
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1965](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1965/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-64969023
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Andrew Phillips <no...@github.com>.
> private final HostnameVerifier verifier;
> private final Supplier<SSLContext> untrustedSSLContextProvider;
> private final HttpUtils utils;
> + private final List<ConnectionSpec> connectionSpecs;
> +
> + @Inject(optional = true)
> + private Supplier<SSLContext> sslContextSupplier;
Recalling @adriancole's recent comments on optional injection, is there any way we can get rid of this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619/files#r21065198
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Ignasi Barrera <no...@github.com>.
Squashed and pushed to master as [958d09e](https://git-wip-us.apache.org/repos/asf?p=jclouds.git;a=commitdiff;h=958d09ecbd2956f6cedfadfa2c67040c386bd105).
Thanks for the reviews and feedback!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65270544
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Andrew Phillips <no...@github.com>.
> @@ -85,12 +85,19 @@ protected static MockWebServer mockWebServer(Dispatcher dispatcher) throws IOExc
> * Creates a test api for the given class and URL.
> */
> protected <T extends Closeable> T api(Class<T> apiClass, String url) {
> + return api(apiClass, url, createConnectionModule());
> + }
> +
> + /**
> + * Creates a test api for the given class, URI and Module.
> + */
> + protected <T extends Closeable> T api(Class<T> apiClass, String url, Module connectionModule) {
[minor] If we're going to add an overloaded version anyway, allow the caller to pass an arbitrary set/list/collection of additional modules?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619/files#r21065183
Re: [jclouds] JCLOUDS-753: Make ConnectionSpec configurable in the
OkHttp driver (#619)
Posted by Ignasi Barrera <no...@github.com>.
That makes sense. I'll change the PR to use the supplier approach and fix the javadocs. If someone want to use this feature for anything else, PRs are welcome :)
Thanks for your feedback @adriancole. It is very much appreciated!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/619#issuecomment-65248218