Add optional OkHttp transport (instead of Netty) #710
Conversation
| @@ -18,6 +18,8 @@ shadowJar { | |||
|
|
|||
| mergeServiceFiles() | |||
|
|
|||
| exclude 'org/newsclub/**' | |||
There was a problem hiding this comment.
docker-java shades it without the relocation o_O we're not using it anyways
| .withDockerCmdExecFactory(new OkHttpDockerCmdExecFactory()); | ||
| } else if ("netty".equals(transportType)) { | ||
| clientBuilder | ||
| .withDockerCmdExecFactory(new TestcontainersDockerCmdExecFactory()); |
There was a problem hiding this comment.
maybe the TestcontainersDockerCmdExecFactory should be renamed to NettyDockerCmdExecFactory in one of the next major releases?
There was a problem hiding this comment.
We plan to switch to okhttp by default and keep Netty as a fallback for a few releases unless we 100% that okhttp works for everyone. Once it happen, we will delete our Netty factory completely. There is also one in docker-java, this is why I named it TestcontainersDockerCmdExecFactory back then
| public void test() throws InvalidConfigurationException { | ||
| try { | ||
| config = tryConfiguration(SOCKET_LOCATION); | ||
| log.info("Accessing docker with local Unix socket"); |
There was a problem hiding this comment.
s/local Unix socket/Named Pipe/
| SSLContext sslContext = sslConfig.getSSLContext(); | ||
| if (sslContext != null) { | ||
| clientBuilder | ||
| .sslSocketFactory(sslContext.getSocketFactory(), new TrustAllX509TrustManager()); |
There was a problem hiding this comment.
Not relevant for this PR, but I'm curious: why is the TrustAllX509TrustManager necessary?
There was a problem hiding this comment.
because it is mutual TLS and we have to trust the certs from Docker as well :)
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| @Slf4j | ||
| public class NpipeSocketClientProviderStrategy extends DockerClientProviderStrategy { |
There was a problem hiding this comment.
WDYT about splitting the Npipe support into seperate PR?
Would of course also mean removing the switch cases for npipe.
There was a problem hiding this comment.
We could do that, but why?
There was a problem hiding this comment.
To make the whole PR smaller, but I see that it's even more effort to put it into a separate PR now.
| .getInstance(config); | ||
|
|
||
| String transportType = TestcontainersConfiguration.getInstance().getTransportType(); | ||
| if ("okhttp".equals(transportType)) { |
There was a problem hiding this comment.
Nitpicky, but constants for transport types.
There was a problem hiding this comment.
Does it really makes sense to use constants for single-use things?
There was a problem hiding this comment.
IMO yes, but that's a question of personal taste 😉
| } | ||
|
|
||
| clientBuilder | ||
| // Disable pooling |
There was a problem hiding this comment.
Wouldn't it be fine to use pooling?
There was a problem hiding this comment.
file-based sockets are not poolable
| super.init(dockerClientConfig); | ||
|
|
||
| OkHttpClient.Builder clientBuilder = new OkHttpClient.Builder() | ||
| .readTimeout(0, TimeUnit.SECONDS) |
There was a problem hiding this comment.
Is it okay to have no timeout here? Are we blocking forever if the daemon isn't answering?
There was a problem hiding this comment.
Image pulling / building sometimes takes a lot of time, and we have timeouts on our side
There was a problem hiding this comment.
I was not sure if we have the timeouts already, then it makes sense.
|
|
||
| HttpUrl.Builder baseUrlBuilder; | ||
|
|
||
| switch (dockerHost.getScheme()) { |
There was a problem hiding this comment.
Since we are having to switch statements with the same argument, WDYT about rearranging the code so we only have one? Or are there good reasons to have 2 switch statements?
There was a problem hiding this comment.
I wanted to keep them separate to not mix the client & request builders. Also helps to understand that i.e. there is no special case for "http/https/tcp" on client builder but there is one for the request
|
I'll try to test npipe tonight. |
|
Quick test on Windows with this Expose daemon on TCP without TLS is disabled in my setup. |
|
🎉 :) |
#756 tested on Windows. Fixed RegistryAuthLocatorTest on Windows and also allowed better fallbacks from running credential provider (to allow lookup alternative AuthConfigs), when: 1) there is no hostName, then there is no point to ask credentials 2) when credential helper response with "credentials not found in native keychain" to try other resources Main reason for failing for me on Windows machine was #710 changes. When i used Netty or OkHttp together with npipe, then it worked fine. Yesterday evening i found out the reason and today morning i found also fix in master for that :-) - #865, breaking docker response by line breaks.
This PR introduces OkHttp transport as an alternative to Netty. It's fully shaded (including SBT's file sockets) and doesn't add dependencies to the project.
OkHttp is a modern JVM http library, it's super easy to use custom socket factories (like unix socket or npipe), easy to shade (zero dependencies, only okhttp & okio) and work with.
Coverage:
macOS:
Linux (based on CI envs):
Windows:
28.05.2018: need help to test on Windows, especially npipe