-
Notifications
You must be signed in to change notification settings - Fork 22
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
env_docker: support docker-in-docker #63
Conversation
If we are running e2e tests inside Docker then it's not so straightforward to access host's network. Let's use the special `host.docker.internal` for that.
I was facing the same issue some time ago, but back then the special hostname I see the documentation says this workaround is available on all platforms (https://docs.docker.com/desktop/networking/#use-cases-and-workarounds-for-all-platforms), but I wonder if this applies only to Docker Desktop? |
env_docker.go
Outdated
// To access the host's network, let's use host.docker.internal. | ||
// See: https://docs.docker.com/desktop/networking/#i-want-to-connect-from-a-container-to-a-service-on-the-host. | ||
if _, err := os.Stat("/.dockerenv"); err == nil { | ||
addr = "host.docker.internal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we refer to the constant from the same file that already holds this address and avoid repeating it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM mod the above comments!
I think for most platforms/setup localhost addrs work for Endpoint()
, but likely needs an exception for docker-in-docker!
Mhm, what do you think if we would also double check whether |
Mostly I just wonder if this will work on every platform / setup, as it's not clear to me from the current documentation. I only know in the past, the |
@matej-g OS check won't work as Docker-in-Docker will happily return "Linux", but as reported by @GiedriusS, still uses the |
I'm still missing a straightforward answer if this works on all environments out of the box or not (not sure if what @GiedriusS wrote should be taken as confirmation or assumption - I cannot find documented usage of that hostname anywhere outside of Docker Desktop). Either way, feel free to merge this, worst case scenario we can fix it afterwards and it should not brake existing uses outside of DinD. |
@matej-g it doesn't work in all environments. It depends on the tools you are using/talking about:
Many tools that want to be fully compatible with Docker are using Podman, for example, uses a different hostname: |
Sorry for the confusion. It seems like something on our Jenkins hosts either adds
At least on my machine. Hence, maybe let's try to resolve it with |
I think trying to resolve it is a good idea. It'll be the most stable and failproof way of detecting whether the environment supports it, @GiedriusS. |
Added check + re-used the same constant. Thanks for the reviews 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition, thanks you 🙇
* env_docker: support docker-in-docker If we are running e2e tests inside Docker then it's not so straightforward to access host's network. Let's use the special `host.docker.internal` for that. * env_docker: try resolve before using docker gateway
If we are running e2e tests inside Docker then it's not so straightforward to access host's network. Let's use the special
host.docker.internal
for that.