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 support for user defined networks #1923

Closed

Conversation

dom54
Copy link

@dom54 dom54 commented Sep 28, 2019

for environments where the hostIp does not resolve correctly to an accessible IP address.

In our environments, we use docker in docker (sibling strategy) where a CI agent is a docker container on a user defined network. We want to the RYUK container to be part of that user defined network and then accessed via container name on port 8080 (other strategies dont seem to work in this setup, the hostIp resolves to a docker IP which is not accessible)

for those not using user defined networks, a way to override the hostIp (i.e. with the accessible DNS instead of internal docker gateway - which doesnt seem to work for us) has also been provided

System parameters added:
TESTCONTAINERS_RYUK_HOST_OVERRIDE
TESTCONTAINERS_RYUK_USER_NETWORK

See #1922

…tIp does not resolve correctly to an accessible IP address.
@bsideup
Copy link
Member

bsideup commented Oct 1, 2019

Hi @dom54,

Ryuk is just a regular container, we start other containers the very same way, and, if Ryuk does not work (e.g. the host is wrong, or you want to use the original port) then other containers will fail too.

Hence I suggest to focus on supporting the user defined networks for any container, not just Ryuk

@dom54
Copy link
Author

dom54 commented Oct 1, 2019

Hi,

That's not quite true - when creating a container you have access to the networkMode via GenericContainer container.withNetworkMode(getNetworkName()); We are using this and it works perfectly for us.

The ResourceReaper class is hard coded to go to the randomized port and the local network gateway IP address which isn't guaranteed to be accessible

@dom54
Copy link
Author

dom54 commented Oct 1, 2019

This PR simply uses the same API when the system properties are set. This is very handy for us because locally we want to use "out of the box" but on the CI server (docker in docker) we need to use local networks.

@dom54
Copy link
Author

dom54 commented Oct 2, 2019

Is the content of the PR OK ? Or another approach needed?

@bsideup
Copy link
Member

bsideup commented Oct 2, 2019

@dom54 there are a few other containers we start in Testcontainers (e.g. Socat) that need to be taken care of too.
As I previously said, I would focus on making any container work in this mode

@dom54
Copy link
Author

dom54 commented Oct 2, 2019

Not really sure how this would be done - Ryuk seems special in that you can enable / disable it via environment variables already (unlike the other containers). Unfortunately dont have more time to spend on this sorry. Happy to close if you dont want the change in

@rnorth
Copy link
Member

rnorth commented Oct 12, 2019

I think we're going to have to close this as a little too niche for us to pick up.

In the future if someone wants to contribute more generalized support for running support containers on a specific network, that would probably resolve the issue identified by @bsideup.

Thanks for taking the time to propose this, @dom54, and sorry that we couldn't proceed with it on this occasion.

@ChristianCiach
Copy link

ChristianCiach commented Aug 14, 2020

I've just wasted multiple hours trying to make Testcontainers work inside a user defined network. I wish I had found this issue earlier..

At first it failed because of Ryuk, but this could be workarounded by disabling it using an environment variable. Then is failed because Testcontainers tried to connect to its own created container by using the host IP (ContainerState.getHost()), which is not available inside a user defined network.

If you decide not to support user defined networks, that's okay, but I think this should be clearly documented. It could have saved me a lot of time.

That being said, I don't think user defined networks are "niche". The docker documentation itself stresses multiple times that the default "bridge" network should not be used.

Our use case: We test multiple branches (and pull requests) of the same project on the same Jenkins host in parallel. To avoid port-clashes, we separate each Jenkins build using a separate user defined network.

@bsideup
Copy link
Member

bsideup commented Aug 15, 2020

@ChristianCiach Testcontainers randomizes the ports and there will be no clashes if you run them on the same host. Have you considered simplifying your Jenkins setup?

@ChristianCiach
Copy link

ChristianCiach commented Aug 15, 2020

@bsideup Thanks for your reply. Unfortunately we have tons of tests that open ports for various reasons, and not all of them can easily be changed to use random ports. This has nothing to do with Testcontainers, but user defined networks are the most elegant solution for these problems. And I am not willing to change our established CI setup (that follows all the recommendations of the docker manual and general best practices in DevOps) just to make a library work.

I know you mean well, but changing our large infrastructure that is used by tons of projects just to workaround a shortcoming of Testcontainers is just not feasible.

I am sorry for my rant here yesterday. I was just in a bad mood after putting so much time into it.

@bsideup
Copy link
Member

bsideup commented Aug 15, 2020

@ChristianCiach I see! We will think what can we do to make it easy to run TC in such environments.

And no problem with the rant :) Next time, consider joining our Slack, we're always ready to help and answer questions, so that you don't have to spend a lot of time figuring out something trivial. There is always someone from the community who may help, or even had a similar situation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants