Always obtain DOCKER_HOST from env (#1770) by usmanovbf · Pull Request #1771 · testcontainers/testcontainers-java

usmanovbf · 2019-08-22T09:05:03Z

It is based on #1770

bsideup

There is EnvironmentAndSystemPropertyClientProviderStrategy that should use the environment variables, doesn't it work for you?

usmanovbf · 2019-08-22T16:33:39Z

@bsideup Unfortunately not. Because first valid strategy choosed as the UnixSocketClientProviderStrategy not the EnvironmentAndSystemPropertyClientProviderStrategy. I don't know why so that changed UnixSocketClientProviderStrategy

rnorth · 2019-12-07T13:54:08Z

OK, I've finally had time to look into why the EnvironmentAndSystemPropertyClientProviderStrategy wasn't taking effect - @usmanovbf is quite correct that it doesn't. We have:

https://github.com/testcontainers/testcontainers-java/blob/master/core/src/main/java/org/testcontainers/dockerclient/EnvironmentAndSystemPropertyClientProviderStrategy.java#L25-L27

So, if DOCKER_HOST points to a unix socket and the system is not Linux, the EnvironmentAndSystemPropertyClientProviderStrategy will not be used and this problem will happen. We've probably been lucky that virtually all environments where DOCKER_HOST matters (e.g. CI) are Linux based.

Given that we have better code in UnixSocketClientProviderStrategy for checking the socket availability, I'm happier to let this class be environment variable aware and rely less on EnvironmentAndSystemPropertyClientProviderStrategy.

However, I'd like to make a couple of tweaks to this PR to improve the other strategies along similar lines. I'll do that myself and push shortly.

…ocket paths

rnorth · 2019-12-07T14:16:23Z

/AzurePipelines run Windows 10 - Docker for Windows

azure-pipelines · 2019-12-07T14:16:30Z

Azure Pipelines successfully started running 1 pipeline(s).

rnorth · 2019-12-23T02:44:10Z

@bsideup we discussed this very briefly in person last week, and I think you spotted a flaw. Do you remember what it was? 😬

bsideup · 2019-12-23T08:05:03Z

@rnorth yes. Strategies (like the npipe one) must filter the value of the env variable to prevent wrong selection (e.g. npipe strategy talking over TCP)

… a different scheme

…iners-java into 1770_docker_host_env

Alex-ala · 2020-01-17T09:52:20Z

Is there also a way to apply the hosts DOCKER_HOST to Ryuk, if its a unix socket? (

testcontainers-java/core/src/main/java/org/testcontainers/utility/ResourceReaper.java

Line 75 in 50ff2c5

    
           binds.add(new Bind("//var/run/docker.sock", new Volume("/var/run/docker.sock")));

)
That would resolve issues with deployents that have the docker.sock in an unusual place.

I've updated the PR since

rnorth · 2020-01-24T20:16:38Z

/AzurePipelines run Windows 10 - Docker for Windows

azure-pipelines · 2020-01-24T20:16:46Z

Azure Pipelines successfully started running 1 pipeline(s).

rnorth · 2020-02-05T09:45:23Z

Just for sanity's sake, I've tested this in our CI environment which is DinD with DOCKET_HOST set to a tcp address (which is fairly exotic). There were no surprises, and it seems to continue to just work here too.

[build:L1390:239s] org.testcontainers.images.ImagePullPolicyTest STANDARD_OUT
[build:L1391:239s]     09:39:10.531 DEBUG org.testcontainers.dockerclient.DockerClientProviderStrategy - Pinging docker daemon...
[build:L1392:240s]     09:39:11.043 INFO  org.testcontainers.dockerclient.EnvironmentAndSystemPropertyClientProviderStrategy - Found docker client settings from environment
[build:L1393:240s]     09:39:11.046 INFO  org.testcontainers.dockerclient.DockerClientProviderStrategy - Found Docker environment with Environment variables, system properties and defaults. Resolved dockerHost=tcp://172.18.0.1:2375
[build:L1394:240s]     09:39:11.047 DEBUG org.testcontainers.dockerclient.DockerClientProviderStrategy - Checking Docker OS type for Environment variables, system properties and defaults. Resolved dockerHost=tcp://172.18.0.1:2375
[build:L1395:240s]     09:39:11.364 INFO  org.testcontainers.DockerClientFactory - Docker host IP address is 172.18.0.1
[build:L1396:240s]     09:39:11.418 INFO  org.testcontainers.DockerClientFactory - Connected to docker:
[build:L1397:240s]       Server Version: 19.03.0
[build:L1398:240s]       API Version: 1.40
[build:L1399:240s]       Operating System: Alpine Linux v3.10 (containerized)
[build:L1400:240s]       Total Memory: 7963 MB

Given that this is tested on a variety of CI and local environments, I think I'm happy that this doesn't introduce any regressions. WDYT @bsideup, @kiview?

…70_docker_host_env

bsideup

current master's state removes the "TCP or Linux" condition on EnvironmentAndSystemPropertyClientProviderStrategy and now it should take precedence over UnixSocketClientProviderStrategy, which means that we don't need to read DOCKER_HOST there :D So I'd revert it.

But the getDockerUnixSocketPath() change is still helpful.

…entAndSystemPropertyClientProviderStrategy is applied on non-linux systems

bsideup · 2020-07-11T18:53:05Z

core/src/main/java/org/testcontainers/dockerclient/NpipeSocketClientProviderStrategy.java


    protected static final String DOCKER_SOCK_PATH = "//./pipe/docker_engine";
-    private static final String SOCKET_LOCATION = "npipe://" + DOCKER_SOCK_PATH;
+    private static final String SOCKET_LOCATION = System.getenv().getOrDefault("DOCKER_HOST", "npipe://" + DOCKER_SOCK_PATH);


I think this change is unnecessary now

bsideup · 2020-07-11T19:12:15Z

core/src/main/java/org/testcontainers/dockerclient/UnixSocketClientProviderStrategy.java

+    protected static final String SOCKET_LOCATION = "unix:///var/run/docker.sock";
    protected static final String DOCKER_SOCK_PATH = "/var/run/docker.sock";


nit (reducing the changeset):

Suggested change

protected static final String SOCKET_LOCATION = "unix:///var/run/docker.sock";

protected static final String DOCKER_SOCK_PATH = "/var/run/docker.sock";

protected static final String DOCKER_SOCK_PATH = "/var/run/docker.sock";

private static final String SOCKET_LOCATION = "unix://" + DOCKER_SOCK_PATH;

bsideup · 2020-07-11T19:15:20Z

core/src/main/java/org/testcontainers/dockerclient/UnixSocketClientProviderStrategy.java

    @Override
    protected boolean isApplicable() {
-        return SystemUtils.IS_OS_LINUX;
+        return SystemUtils.IS_OS_LINUX || SystemUtils.IS_OS_MAC_OSX;


nice catch!

bsideup · 2020-07-11T19:21:19Z

core/src/main/java/org/testcontainers/dockerclient/UnixSocketClientProviderStrategy.java

    @Override
    protected boolean isApplicable() {
-        return SystemUtils.IS_OS_LINUX;
+        return SystemUtils.IS_OS_LINUX || SystemUtils.IS_OS_MAC_OSX;


Brave new world:

Suggested change

return SystemUtils.IS_OS_LINUX || SystemUtils.IS_OS_MAC_OSX;

return SystemUtils.IS_OS_LINUX || SystemUtils.IS_OS_MAC;

core/src/main/java/org/testcontainers/DockerClientFactory.java

Co-authored-by: Sergei Egorov <[email protected]>

rnorth · 2020-07-12T11:59:50Z

We'll tackle this in #2985 instead, as the changes there make the majority of this PR moot.

Obtain DOCKER_HOST from env (testcontainers#1770)

9b52d26

usmanovbf requested review from bsideup, kiview and rnorth as code owners August 22, 2019 09:05

bsideup previously requested changes Aug 22, 2019

View reviewed changes

rnorth added this to the next milestone Sep 8, 2019

bsideup modified the milestones: 1.12.3, next Oct 26, 2019

rnorth removed this from the next milestone Oct 30, 2019

rnorth self-assigned this Nov 1, 2019

rnorth added 2 commits December 7, 2019 14:13

Allow DOCKER_HOST environment variable to override hardcoded docker s…

9f971f4

…ocket paths

Merge branch 'master' into 1770_docker_host_env

d3e6aea

rnorth added 3 commits December 26, 2019 21:15

Merge branch 'master' into 1770_docker_host_env

65bdee1

Prevent client strategies from being applied if DOCKER_HOST points to…

e2b8b1a

… a different scheme

Merge branch '1770_docker_host_env' of github.com:usmanovbf/testconta…

c24b9e9

…iners-java into 1770_docker_host_env

rnorth requested a review from bsideup December 26, 2019 13:05

Merge branch 'master' into 1770_docker_host_env

89bedea

rnorth added 2 commits April 26, 2020 15:12

Merge branch 'master' into 1770_docker_host_env

3394c7c

Restore DOCKER_SOCK_PATH to maintain backwards compatibility

49f4e31

bsideup and others added 3 commits June 17, 2020 15:10

Merge branch 'master' into unshade_docker_java_api

4f65645

Merge branch 'master' into unshade_docker_java_api

561f300

Merge remote-tracking branch 'origin/master' into 1770_docker_host_env

5532e72

rnorth self-assigned this Jul 9, 2020

rnorth added 2 commits July 9, 2020 16:07

Merge remote-tracking branch 'origin/unshade_docker_java_api' into 17…

0bfa10d

…70_docker_host_env

Combine with testcontainers#2882

4c473aa

rnorth mentioned this pull request Jul 9, 2020

Add a rootless Docker strategy #2985

Merged

rnorth added 2 commits July 10, 2020 11:23

Standardise logic for determining container-mountable docker socket path

5890674

Merge remote-tracking branch 'origin/master' into 1770_docker_host_env

aa3b650

rnorth changed the title ~~Obtain DOCKER_HOST from env (#1770)~~ WIP: Obtain DOCKER_HOST from env (#1770) Jul 10, 2020

Merge remote-tracking branch 'origin/master' into 1770_docker_host_env

add84f3

rnorth changed the title ~~WIP: Obtain DOCKER_HOST from env (#1770)~~ Obtain DOCKER_HOST from env (#1770) Jul 10, 2020

rnorth changed the title ~~Obtain DOCKER_HOST from env (#1770)~~ Always obtain DOCKER_HOST from env (#1770) Jul 10, 2020

rnorth added 3 commits July 10, 2020 14:26

Harmonise socket path method with testcontainers#2985

f560757

Harmonise socket path method with testcontainers#2985

239dfcf

Revert javadoc comment

af27daa

bsideup approved these changes Jul 11, 2020

View reviewed changes

bsideup added this to the next milestone Jul 11, 2020

bsideup added the type/enhancement label Jul 11, 2020

bsideup requested changes Jul 11, 2020

View reviewed changes

rnorth added 2 commits July 11, 2020 19:15

Merge branch 'master' into 1770_docker_host_env

d0265d1

Unix client provider strategy can used a fixed path now that Environm…

fe8ad5d

…entAndSystemPropertyClientProviderStrategy is applied on non-linux systems

bsideup reviewed Jul 11, 2020

View reviewed changes

bsideup reviewed Jul 12, 2020

View reviewed changes

core/src/main/java/org/testcontainers/DockerClientFactory.java Outdated Show resolved Hide resolved

Update core/src/main/java/org/testcontainers/DockerClientFactory.java

6dc207e

Co-authored-by: Sergei Egorov <[email protected]>

rnorth closed this Jul 12, 2020

		protected static final String SOCKET_LOCATION = "unix:///var/run/docker.sock";
		protected static final String DOCKER_SOCK_PATH = "/var/run/docker.sock";

	return SystemUtils.IS_OS_LINUX \|\| SystemUtils.IS_OS_MAC_OSX;
	return SystemUtils.IS_OS_LINUX \|\| SystemUtils.IS_OS_MAC;

Uh oh!

Conversation

usmanovbf commented Aug 22, 2019

Uh oh!

bsideup left a comment

Choose a reason for hiding this comment

Uh oh!

usmanovbf commented Aug 22, 2019

Uh oh!

rnorth commented Dec 7, 2019

Uh oh!

rnorth commented Dec 7, 2019

Uh oh!

azure-pipelines bot commented Dec 7, 2019

Uh oh!

rnorth commented Dec 23, 2019

Uh oh!

bsideup commented Dec 23, 2019

Uh oh!

Alex-ala commented Jan 17, 2020

Uh oh!

rnorth commented Jan 24, 2020

Uh oh!

azure-pipelines bot commented Jan 24, 2020

Uh oh!

rnorth commented Feb 5, 2020

Uh oh!

bsideup left a comment

Choose a reason for hiding this comment

Uh oh!

bsideup Jul 11, 2020

Choose a reason for hiding this comment

Uh oh!

bsideup Jul 11, 2020

Choose a reason for hiding this comment

Uh oh!

bsideup Jul 11, 2020

Choose a reason for hiding this comment

Uh oh!

bsideup Jul 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rnorth commented Jul 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants