What does this PR do?

Container env detection

Currently testcontainers-go detects only nested Docker environments (no check for /run.container for Podman) and from my understanding it only checks for the default gateway of the default network to use this for port mappings.

This PR refactors the current behavior to also consider Podman environments and to detect its own container ID from the /etc/hostname mount (both Docker and Podman are doing this somehow similar with a tmpfs mount from a directory that has the container ID in the mount root path.

Bind mount remapping

The library now detects if it is running in a container (with a local Docker daemon i.e. with a Docker socket, not a TCP/HTTP connection) and if a developer attempts to use a bind mount in this setting it will try to map the source path to a host path.

Considering the following example:

• CI provider clones the repo to /home/ci/work/1234/ on the host
• spawns a new container like so: docker run --rm -i -v /home/ci/work/1234:/work -v /var/run/docker.sock:/var/run/docker.sock:ro -w /work ...
• Scenario A:
  • test code attempts to bind mount /work/testdata/my-config.toml to a test container
  • tc-go detects it is running in container environment and that the path /work/ is actually part of a bind mount
  • tc-go "re-maps" the mount source /work/testdata/my-config.toml to /home/ci/work/1234/testdata/my-config.toml
• Scenario B:
  • test code attempts to bind mount /etc/test/my-config.toml to a test container
  • tc-go detects it is running in container environment and that the path /etc/test/my-config.toml might not be available if mounted to another container and returns an error

There are some corner cases that are currently not yet taken care of like:

• a user might intentionally mount a path that is not part of a bind mount
• there's a TCP-to-socket proxy in place i.e. remote daemon detection doesn't work
• ...

Codecov Report

Merging #497 (e9d63ed) into main (097f9af) will increase coverage by 0.50%. The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #497      +/-   ##
==========================================
+ Coverage   69.67%   70.17%   +0.50%     
==========================================
  Files          21       21              
  Lines        2048     2116      +68     
==========================================
+ Hits         1427     1485      +58     
- Misses        497      500       +3     
- Partials      124      131       +7     

Impacted Files	Coverage Δ
docker_mounts.go	100.00% <ø> (+4.87%)	:arrow_up:
docker.go	72.58% <64.86%> (+1.46%)	:arrow_up:
reaper.go	82.20% <66.66%> (-1.59%)	:arrow_down:
compose.go	74.04% <100.00%> (ø)
container.go	81.81% <100.00%> (+1.09%)	:arrow_up:
wait/http.go	59.52% <100.00%> (ø)
wait/log.go	76.59% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

@baez90 btw, just to let you know I started yesterday as an @AtomicJar employee maintaining the open source projects of the testcontainers family, so full-time dedication to this amazing community!!

I'll resolve the conflicts in the evening. I wasn't sure how to 'fix' the coverage issue as I don't have much experience with codecov and it would be necessary to somehow 'merge' the coverage results from the nested test run and from the non-nested one. Do you happen to have an idea how to do this? Otherwise I'll give it a try also this evening :blush:

Congratulations to the new position! :tada: I'm excited to hear that the community got such a great supporter with you! Wish you all the best with this new challenge :blush:

I'll resolve the conflicts in the evening. I wasn't sure how to 'fix' the coverage issue as I don't have much experience with codecov and it would be necessary to somehow 'merge' the coverage results from the nested test run and from the non-nested one. Do you happen to have an idea how to do this? Otherwise I'll give it a try also this evening 😊

Yeah, many times codecov complains about a line not being touched, even on dependabot PRs, which makes me think if it's adding more noise than value. So I'd delegate a final approval on the coverage to a human

Don't know how to fix the patch coverage issue but at least to overall coverage is fixed 🙂 Let me know if there's something missing otherwise I'd squash the commits to make this ready to be merged?

I'll have a look at your nits probably tomorrow, thanks for your feedback!

Sorry about "mixing" up the PR, I tried to separate the changes in multiple commits to improve the review flow e.g. all the ioutil changes are in a separate commit and so but I'll try to narrow the scope in the future, sorry again :sweat_smile:

Sorry about "mixing" up the PR, I tried to separate the changes in multiple commits to improve the review flow e.g. all the ioutil changes are in a separate commit and so but I'll try to narrow the scope in the future, sorry again 😅

No worries at all, I totally understand it and I can imagine myself adding commits on top as you did while finding "things". So not a problem :)

I mentioned because we are tying to have a better internal review process, and things like that would improve our performance 😅

I am joining this PR as a tc-java maintainer and I might miss the bigger picture, just wanted to point out something I noticed in the description of the PR:

I'm also planning to rework the mount behavior a bit to auto-replace bind mounts source paths to make them host-path relative if possible or raise an error if the mount won't work due to nested environment constraints

There are other reasons why the mount won't work, most notably a remote Docker daemon. With making them host-path relative are you specifically talking about the Docker-Socket-Mounting scenario? I believe solving this can be a bigger headache than clearly not supporting it and using copy-to-container mechanisms instead.

using copy-to-container mechanisms instead.

Thanks @kiview, I was planing to articulate exactly this into a separate issue, maybe it makes sense to add it here as a reference.

Thanks @kiview for the feedback. I honestly forgot to update the PR description but you're right, this is only about re-mapping bind mounts if the current process is running in a container and the docker daemon is not a remote daemon.

Of course this could as well left out but I'm hitting this scenario a lot in the last time and always building custom images only to work around this issue in certain circumstances means a significant loss of performance in comparison to just re-map the source if possible - especially because you've to build the image no matter if running locally or in the CI. Also I think it improves the user experience if an error is returned in case testcontainers-go detected that a re-mapping is not possible instead of just returning an error from the Docker daemon telling the user a directory doesn't exist although it very well does if the test is executed locally but not in the CI environment just because e.g. Microsoft decided to mount the working directory in another path in the container than on the host.

I don't intent to solve this issue altogether (like also taking care of volume mounts....) but in this special scenario it...doesn't seem like such a headache to be honest? :sweat_smile:

@baez90 Thanks for the explanation, I was initially missing the meaning of remap in this context. I agree that this likely helps with the DX (developer experience) if the test fails early in environments where this is not supported. I am simply a bit cautious regarding how robust the detection is (because I have seen so weird Docker environments from users in tc-java). Probably I need to look more in the tc-go implementations around this, to also learn if there are things we can apply to tc-java as well.

:smile: yeah I mean there are always things that can be improved and some corner cases that are hard to circumvent e.g. if a user has a...socket-to-TCP proxy to - for whatever reason - not use the TCP connection directly it'll be nearly impossible to detect this but for such scenarios it's still possible to fallback to copy-to-container or introduce an option to explicitly disable the mapping later. Also there are of course still scenarios that could be covered to further improve the DX but this PR is already blown up too much :smile: so probably something for later

What's the current state of this? Is it missing something?

@baez90 after moving compose to a separate module, this PR got into conflicts. Are you still interested on it? Otherwise we could close it and eventually submit a new one.

Thanks!

I'm actually wondering if it would not make sense to re-create this PR instead of trying to rebase it. Also I currently do not have the use case for it anymore. I could look into it again as I think it would be great to improve this but I cannot tell you when I'll be able to work on it as I'm currently occupied with other projects

Hey @baez90 I'm tempted to close this PR because of

I'm actually wondering if it would not make sense to re-create this PR instead of trying to rebase it. Also I currently do not have the use case for it anymore.

Just to let you know, there is a WIP PR for improving podman support here: https://github.com/testcontainers/testcontainers-go/pull/1251

Yeah, I won't be able to progress on that in the close future I'm afraid, thanks!