Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional OkHttp transport (instead of Netty) #710

Merged
merged 34 commits into from
Jun 13, 2018
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
455b8b3
add OkHttp transport
bsideup Apr 26, 2018
8edcf85
Merge branch 'master' into okhttp
bsideup May 18, 2018
a0bde0c
implement chunked response handling based on Netty implementation
bsideup May 19, 2018
85a84c2
handle response errors
bsideup May 19, 2018
d6a59bf
do not block caller's thread
bsideup May 20, 2018
ebd4920
split files, support more protocols, fix blocking call
bsideup May 20, 2018
7727b3c
mark OkHttpInvocationBuilder & OkHttpWebTarget as internal classes
bsideup May 20, 2018
788543f
re-throw ExecutionException's cause
bsideup May 20, 2018
0166126
always close the response
bsideup May 20, 2018
e1021b1
reuse OkHttpClient instance
bsideup May 20, 2018
360efb6
shade okhttp / okio, disable pooling of unix-socket based transport
bsideup May 21, 2018
b905ecf
do not block caller's thread (well, again :D)
bsideup May 21, 2018
ff8a1a2
retry on connection failure
bsideup May 21, 2018
5fe2dee
do not use mutable "." directory in DirectoryTarResourceTest
bsideup May 21, 2018
086c4db
replace docker-filesocket with plain AFUNIXSocket factory
bsideup May 21, 2018
f64be0f
fix shaded dependencies, rename handleStreamedResponse -> executeAndS…
bsideup May 21, 2018
da52424
fix shading
bsideup May 21, 2018
7cc4c78
remove isOpen check
bsideup May 21, 2018
23fdb7f
try jnr-unixsocket
bsideup May 21, 2018
65aa80f
remove cglib
bsideup May 21, 2018
3a82fac
try org.scala-sbt.ipcsocket:ipcsocket
bsideup May 21, 2018
d9c6db7
experimental npipe support
bsideup May 21, 2018
32ac15d
fix pipe name
bsideup May 22, 2018
40b47c2
fix double escaping
bsideup May 22, 2018
6386bd4
Merge branch 'master' into okhttp
bsideup May 22, 2018
d005f08
Merge branch 'master' into okhttp
bsideup May 22, 2018
6ed7479
shade ipcsocket
bsideup May 22, 2018
c5eda4f
disable read timeout
bsideup May 23, 2018
8cabd49
make OkHttp opt-in-able
bsideup May 28, 2018
9a0e302
fix properties file name
bsideup May 28, 2018
c6f962b
fix typo
bsideup May 28, 2018
5cf871b
fix Codacy warnings
bsideup May 28, 2018
d384b16
workaround docker-java (does not support npipe)
bsideup Jun 11, 2018
2e1bc26
support npipe in DockerClientConfigUtils
bsideup Jun 11, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,10 @@ subprojects {
"com.google",
"io.netty",
"org.bouncycastle",
"org.newsclub",
"org.zeroturnaround"
"org.zeroturnaround",
"okhttp3",
"okio",
"org.scalasbt.ipcsocket",
].each { relocate(it, "org.testcontainers.shaded.$it") }
}

Expand Down
16 changes: 16 additions & 0 deletions circle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,21 @@ jobs:
when: always
- store_test_results:
path: ~/junit
okhttp:
steps:
- checkout
- run:
command: |
echo "transport.type=okhttp" >> core/src/test/resources/testcontainers.properties
./gradlew testcontainers:check
- run:
name: Save test results
command: |
mkdir -p ~/junit/
find . -type f -regex ".*/build/test-results/.*xml" -exec cp {} ~/junit/ \;
when: always
- store_test_results:
path: ~/junit
modules-no-jdbc-test-no-selenium:
steps:
- checkout
Expand Down Expand Up @@ -65,6 +80,7 @@ workflows:
test_all:
jobs:
- core
- okhttp
- modules-no-jdbc-test-no-selenium
- modules-jdbc-test
- selenium
15 changes: 14 additions & 1 deletion core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ shadowJar {

mergeServiceFiles()

exclude 'org/newsclub/**'
Copy link
Member Author

Choose a reason for hiding this comment

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

docker-java shades it without the relocation o_O we're not using it anyways


[
'META-INF/io.netty.versions.properties',
'META-INF/NOTICE',
Expand Down Expand Up @@ -46,11 +48,13 @@ shadowJar {
include(dependency('com.google.guava:.*'))
include(dependency('io.netty:.*'))
include(dependency('org.bouncycastle:.*'))
include(dependency('org.newsclub.*:.*'))
include(dependency('org.zeroturnaround:zt-exec'))
include(dependency('commons-lang:commons-lang'))
include(dependency('commons-io:commons-io'))
include(dependency('commons-codec:commons-codec'))
include(dependency('com.squareup.okhttp3:.*'))
include(dependency('com.squareup.okio:.*'))
include(dependency('org.scala-sbt.ipcsocket:ipcsocket'))
}
}

Expand Down Expand Up @@ -84,12 +88,21 @@ dependencies {
exclude(group: "log4j", module: "log4j")
}

compile "net.java.dev.jna:jna-platform:4.5.1"

shaded ('org.scala-sbt.ipcsocket:ipcsocket:1.0.0') {
exclude(group: "net.java.dev.jna")
}

shaded ('com.github.docker-java:docker-java:3.1.0-rc-3') {
exclude(group: 'org.glassfish.jersey.core')
exclude(group: 'org.glassfish.jersey.connectors')
exclude(group: 'log4j')
exclude(group: 'com.google.code.findbug')
exclude(group: 'com.kohlschutter.junixsocket')
}
shaded 'com.squareup.okhttp3:okhttp:3.10.0'

shaded 'javax.ws.rs:javax.ws.rs-api:2.0.1'
shaded 'org.zeroturnaround:zt-exec:1.8'
shaded 'commons-lang:commons-lang:2.6'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.dockerclient.transport.TestcontainersDockerCmdExecFactory;
import org.testcontainers.dockerclient.transport.okhttp.OkHttpDockerCmdExecFactory;
import org.testcontainers.utility.TestcontainersConfiguration;

import java.util.ArrayList;
Expand Down Expand Up @@ -164,10 +165,23 @@ public DockerClient getClient() {
}

protected DockerClient getClientForConfig(DockerClientConfig config) {
return DockerClientBuilder
.getInstance(config)
.withDockerCmdExecFactory(new TestcontainersDockerCmdExecFactory())
.build();
DockerClientBuilder clientBuilder = DockerClientBuilder
.getInstance(config);

String transportType = TestcontainersConfiguration.getInstance().getTransportType();
if ("okhttp".equals(transportType)) {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicky, but constants for transport types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it really makes sense to use constants for single-use things?

Copy link
Member

Choose a reason for hiding this comment

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

IMO yes, but that's a question of personal taste 😉

clientBuilder
.withDockerCmdExecFactory(new OkHttpDockerCmdExecFactory());
} else if ("netty".equals(transportType)) {
clientBuilder
.withDockerCmdExecFactory(new TestcontainersDockerCmdExecFactory());
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the TestcontainersDockerCmdExecFactory should be renamed to NettyDockerCmdExecFactory in one of the next major releases?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

} else {
throw new IllegalArgumentException("Unknown transport type: " + transportType);
}

LOGGER.info("Will use '{}' transport", transportType);

return clientBuilder.build();
}

protected void ping(DockerClient client, int timeoutInSeconds) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package org.testcontainers.dockerclient;

import com.github.dockerjava.core.DefaultDockerClientConfig;
import com.github.dockerjava.core.DockerClientConfig;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang.SystemUtils;
import org.jetbrains.annotations.NotNull;

@Slf4j
public class NpipeSocketClientProviderStrategy extends DockerClientProviderStrategy {
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about splitting the Npipe support into seperate PR?
Would of course also mean removing the switch cases for npipe.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do that, but why?

Copy link
Member

Choose a reason for hiding this comment

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

To make the whole PR smaller, but I see that it's even more effort to put it into a separate PR now.


protected static final String DOCKER_SOCK_PATH = "//./pipe/docker_engine";
private static final String SOCKET_LOCATION = "npipe://" + DOCKER_SOCK_PATH;

private static final String PING_TIMEOUT_DEFAULT = "10";
private static final String PING_TIMEOUT_PROPERTY_NAME = "testcontainers.npipesocketprovider.timeout";

public static final int PRIORITY = EnvironmentAndSystemPropertyClientProviderStrategy.PRIORITY - 20;

@Override
protected boolean isApplicable() {
return SystemUtils.IS_OS_WINDOWS;
}

@Override
public void test() throws InvalidConfigurationException {
try {
config = tryConfiguration(SOCKET_LOCATION);
log.info("Accessing docker with {}", getDescription());
} catch (Exception | UnsatisfiedLinkError e) {
throw new InvalidConfigurationException("ping failed", e);
}
}

@NotNull
protected DockerClientConfig tryConfiguration(String dockerHost) {
config = DefaultDockerClientConfig.createDefaultConfigBuilder()
.withDockerHost(dockerHost)
.withDockerTlsVerify(false)
.build();
client = getClientForConfig(config);

final int timeout = Integer.parseInt(System.getProperty(PING_TIMEOUT_PROPERTY_NAME, PING_TIMEOUT_DEFAULT));
ping(client, timeout);

return config;
}

@Override
public String getDescription() {
return "local Npipe socket (" + SOCKET_LOCATION + ")";
}

@Override
protected int getPriority() {
return PRIORITY;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
package org.testcontainers.dockerclient.transport.okhttp;

import lombok.SneakyThrows;
import lombok.Value;
import org.scalasbt.ipcsocket.Win32NamedPipeSocket;

import javax.net.SocketFactory;
import java.io.FilterInputStream;
import java.io.FilterOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.InetAddress;
import java.net.Socket;
import java.net.SocketAddress;

@Value
public class NamedPipeSocketFactory extends SocketFactory {

String socketPath;

@Override
@SneakyThrows
public Socket createSocket() {
return new Win32NamedPipeSocket(socketPath.replace("/", "\\")) {

@Override
public void connect(SocketAddress endpoint, int timeout) throws IOException {
// Do nothing since it's not "connectable"
}

@Override
public InputStream getInputStream() {
return new FilterInputStream(super.getInputStream()) {
@Override
public void close() throws IOException {
shutdownInput();
}
};
}

@Override
public OutputStream getOutputStream() {
return new FilterOutputStream(super.getOutputStream()) {

@Override
public void write(byte[] b, int off, int len) throws IOException {
out.write(b, off, len);
}

@Override
public void close() throws IOException {
shutdownOutput();
}
};
}
};
}

@Override
public Socket createSocket(String s, int i) {
throw new UnsupportedOperationException();
}

@Override
public Socket createSocket(String s, int i, InetAddress inetAddress, int i1) {
throw new UnsupportedOperationException();
}

@Override
public Socket createSocket(InetAddress inetAddress, int i) {
throw new UnsupportedOperationException();
}

@Override
public Socket createSocket(InetAddress inetAddress, int i, InetAddress inetAddress1, int i1) {
throw new UnsupportedOperationException();
}
}
Loading