Skip to content

Use host port 0 when setting up PortBindings - #4396

Closed
rnorth wants to merge 2 commits into
mainfrom
bindport-zero
Closed

rnorth wants to merge 2 commits into
mainfrom
bindport-zero

Conversation

@rnorth

@rnorth rnorth commented Aug 26, 2021 •

Copy link
Copy Markdown
Member

Fixes #4395

See also docker/for-mac#5588

@rnorth

rnorth commented Aug 26, 2021

Copy link
Copy Markdown
Member Author

I'm running all Testcontainers tests using this branch, using the Docker wormhole pattern on a Mac.

One slightly unexpected failure is this in the gcloud module - I doubt it's anything to do with this PR, but it seems odd that it's happening and perhaps indicates a gap in our testing of docker-wormhole usage:

org.testcontainers.containers.DatastoreEmulatorContainerTest > testSimple FAILED
    java.lang.IllegalArgumentException: Project endpoint "172.17.0.1:55070/v1/projects/test-project" must include scheme.
        at com.google.datastore.v1.client.DatastoreOptions$Builder.projectEndpoint(DatastoreOptions.java:148)
        at com.google.cloud.datastore.spi.v1.HttpDatastoreRpc.<init>(HttpDatastoreRpc.java:71)
        at com.google.cloud.datastore.DatastoreOptions$DefaultDatastoreRpcFactory.create(DatastoreOptions.java:60)
        at com.google.cloud.datastore.DatastoreOptions$DefaultDatastoreRpcFactory.create(DatastoreOptions.java:54)
        at com.google.cloud.ServiceOptions.getRpc(ServiceOptions.java:561)
        at com.google.cloud.datastore.DatastoreOptions.getDatastoreRpcV1(DatastoreOptions.java:169)
        at com.google.cloud.datastore.DatastoreImpl.<init>(DatastoreImpl.java:57)
        at com.google.cloud.datastore.DatastoreOptions$DefaultDatastoreFactory.create(DatastoreOptions.java:50)
        at com.google.cloud.datastore.DatastoreOptions$DefaultDatastoreFactory.create(DatastoreOptions.java:44)
        at com.google.cloud.ServiceOptions.getService(ServiceOptions.java:541)
        at org.testcontainers.containers.DatastoreEmulatorContainerTest.testSimple(DatastoreEmulatorContainerTest.java:33)

@rnorth

rnorth commented Aug 27, 2021

Copy link
Copy Markdown
Member Author

OK, having run all the tests on a Mac, using the wormhole pattern, I'd say the result is fine. The Mac in question is an x86 machine using Docker Desktop 3.6.0.

There were a few failures, but I think as a direct result of tests/modules that don't play well with a remote docker host rather than anything else.

@kiview kiview left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Works fine on Windows and WSL as well.

@rnorth
rnorth marked this pull request as ready for review September 7, 2021 12:17
@rnorth
rnorth requested a review from bsideup as a code owner September 7, 2021 12:17
@bsideup

bsideup commented Sep 7, 2021

Copy link
Copy Markdown
Member

@rnorth Have you heard anything from the Docker team on this one? I am seriously worried thwt this may break unexpectedly...

@rnorth

rnorth commented Sep 7, 2021

Copy link
Copy Markdown
Member Author

@bsideup not yet.

@rnorth

rnorth commented Sep 20, 2021

Copy link
Copy Markdown
Member Author

I'm going to move this back to draft as we still don't have a clear steer on whether this is safer. We're still broken in some local DinD use cases, which is a bad thing, and I'm wondering if there's another way to tackle this (e.g. an opt-in configuration setting that applies this workaround).

@kiview

kiview commented Sep 20, 2021

Copy link
Copy Markdown
Member

I would say, this fix does not make it worse. Instead of increasing our own complexity, I'd opt for implementing it in a straightforward way and be prepared to revert this in the future in case Docker would break compatibility. But I would not design directly for this eventuality.

We had to deal with certain breaking changes in Docker, especially in their desktop distributions, in the past already and it will happen again in the future, but this should not paralyze us or make us overly defensive.

@rnorth

rnorth commented Oct 8, 2021

Copy link
Copy Markdown
Member Author

To round this off, from docker/for-mac#5588:

I can't guarantee anything but I think there's no imminent danger of it stopping working: I'll add a test case which checks that it continues to work and linking to this issue so can avoid breaking it if possible / give you advance warning

I think this should give us the confidence to (a) use this workaround, and (b) know that if the workaround is ever on the way to being broken we'll be informed ahead of time.

The other comments give me hope that a longer term solution can be developed, but we'll have to wait on Docker to make some changes.

@kiview

kiview commented Oct 8, 2021

Copy link
Copy Markdown
Member

This PR fixes the currently failing ImagePullPolicyTest and AuthenticatedImagePullTest on Docker Desktop for Windows with WSL backend.

@smurfy

smurfy commented Apr 22, 2022

Copy link
Copy Markdown

Any update on this? Still failing for me on mac with docker desktop 4.7.1 (77678)

Tried testcontainers 1.16.3 and 1.17.1

@kiview

kiview commented Jan 3, 2023

Copy link
Copy Markdown
Member

The correct way is now document through #6383.

@kiview kiview closed this Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't connect to Ryuk when using when running in container and using Docker-Socket-Mounting on Docker Desktop

4 participants