Improve experience in nested container environments #497

prskr · 2022-08-13T12:33:43Z

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 · 2022-08-19T15:47:39Z

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%)`	⬆️
docker.go	`72.58% <64.86%> (+1.46%)`	⬆️
reaper.go	`82.20% <66.66%> (-1.59%)`	⬇️
compose.go	`74.04% <100.00%> (ø)`
container.go	`81.81% <100.00%> (+1.09%)`	⬆️
wait/http.go	`59.52% <100.00%> (ø)`
wait/log.go	`76.59% <100.00%> (ø)`

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

mdelapenya

I left a few minor comments before testing the changes locally.

Could you please resolve the go.mod conflicts too?

docker.go

go.mod

wait/testdata/go.mod

mdelapenya · 2022-08-23T12:24:45Z

@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!!

prskr · 2022-08-23T12:30:29Z

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 😊

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

mdelapenya · 2022-08-23T12:37:26Z

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

prskr · 2022-08-24T15:10:27Z

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?

mdelapenya

I added a few ultra-minor comments, but other than that LGTM

I noticed there are changes that could have been sent in a separate PR (replacement of ioutil/io, go version in module files, indents, filepath for the win, usage of constants in tests, better assertions, better usage of t.Testing functions -helpers,environment vars-, increased timeouts) but I'm pretty sure they we re discovered and needed while working in the nested checks.

For further PRs I'd appreciate if we could send those small chunks separately, so that the review of the original feature (nested container environments) could be done faster 🙏

Thanks again for your contribution to make tc-go great!

mdelapenya · 2022-08-30T06:46:20Z

Makefile

@@ -11,6 +11,22 @@ test-unit:
 		--packages="./..." \
 		-- -coverprofile=cover.txt

+.PHONY: test-nested-container


Nit

Suggested change

.PHONY: test-nested-container

.PHONY: test-unit-nested

mdelapenya · 2022-08-30T06:46:38Z

Makefile

@@ -1,6 +1,6 @@

 .PHONY: test-all
-test-all: tools test-unit test-e2e
+test-all: tools test-unit test-e2e test-nested-container


Nit

Suggested change

test-all: tools test-unit test-e2e test-nested-container

test-all: tools test-unit test-e2e test-unit-nested

mdelapenya · 2022-08-30T06:51:06Z

container.go

@@ -264,3 +265,12 @@ func (c *ContainerRequest) validateMounts() error {
 	}
 	return nil
 }
+
+func RunningInContainer() bool {
+	return fileExists("/.dockerenv") || fileExists("/run/.containerenv")


I guess these checks are coupled to traditional LXC containers (.dockerenv) or Podman ones (.containerenv), right?

Do you foresee any other container type, or even any other source to check if running in a container?

On the same hand, this method is used only for testing. Do we still want to expose it as public API?

If I remember correctly it is not only used in testing but also to determine if the container environment information should be fetched.

I was wondering as well if it makes sense to expose this but I thought it could be useful and it's not a huge 'burden' to maintain 😄

Regarding other indicators: I'm not sure how Docker is handling the same with Windows containers so there might be another check in the future to cover these as well (although supporting Windows containers will be fun anyway if this is something that should be done in the future) but apart from that I hope /run/.containerenv is not only used by Podman but by all other container providers apart from Docker

Let's be careful about .dockerenv. To my last knowledge, this is an undocumented implementation detail of Docker and not necessarily a stable indicator. There is some discussion around this on SO, which sadly does not provide a better alternative: https://superuser.com/questions/1021834/what-are-dockerenv-and-dockerinit

I am ware, although Podman explicitly adopted this with their /run/.containerenv flag.
Surely this is not the greatest solution but a fairly easy one and for instance also used in tc-dotnet.
Furthermore the SO link you attached mentions a moby repository where they added the exact same behavior so it seems this flag is considered stable enough by the Docker guys 😄

An alternative approach could be to not use these flags at all but to check if /etc/hostname is mounted (by checking the /proc/self/mountinfo) and if so try to get the container ID from the mount root.

Both solutions are not perfect, I'm open for suggestions

container_test.go

prskr · 2022-08-30T08:09:43Z

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 😅

mdelapenya · 2022-08-30T08:11:59Z

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 😅

kiview · 2022-08-30T11:01:19Z

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.

mdelapenya · 2022-08-30T11:12:35Z

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.

prskr · 2022-08-30T12:48:21Z

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? 😅

kiview · 2022-08-31T10:09:01Z

@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.

prskr · 2022-08-31T11:02:13Z

😄 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 😄 so probably something for later

prskr · 2022-09-06T16:08:06Z

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

mdelapenya · 2023-03-21T07:43:56Z

@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!

prskr · 2023-03-21T10:41:50Z

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

mdelapenya · 2023-06-14T05:53:36Z

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: #1251

prskr · 2023-06-14T08:06:16Z

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

prskr force-pushed the 496-improve-experience-in-nested-container-envs branch 6 times, most recently from d7ec773 to 10a32cf Compare August 19, 2022 14:01

prskr marked this pull request as ready for review August 19, 2022 17:38

mdelapenya reviewed Aug 23, 2022

View reviewed changes

docker.go Outdated Show resolved Hide resolved

docker.go Show resolved Hide resolved

docker.go Show resolved Hide resolved

go.mod Show resolved Hide resolved

wait/testdata/go.mod Outdated Show resolved Hide resolved

prskr force-pushed the 496-improve-experience-in-nested-container-envs branch 2 times, most recently from ea47fe7 to 45436fa Compare August 23, 2022 20:31

mdelapenya self-assigned this Aug 24, 2022

mdelapenya previously approved these changes Aug 30, 2022

View reviewed changes

prskr added 6 commits August 31, 2022 16:25

wip: nested container gateway discovery

b338249

refactor(docker): rename confusing hostCache

cd26262

refactor: remove obsolete ioutil references

c6721f3

chore(ci): ensure Docker test network in CI

82c6acd

refactor(deps): get rid of gotest.tools/v3 and use standard library

172d13d

refactor: improve image build output handling

acf82a2

prskr added 4 commits August 31, 2022 16:25

fix: don't collect coverage in nested tests

db2ea08

docs(docker): document initContainerEnvInformation

2c35997

refactor: apply nits

6981d35

ci: merge coverage results from nested test execution

c7f8515

prskr force-pushed the 496-improve-experience-in-nested-container-envs branch from db8b292 to c7f8515 Compare August 31, 2022 14:25

prskr requested a review from a team as a code owner August 31, 2022 14:25

fix(make): fix renamed Makefile targets

e9d63ed

prskr changed the title ~~WIP: Improve experience in nested container environments~~ Improve experience in nested container environments Aug 31, 2022

mdelapenya added the podman Issues regarding podman. label Sep 16, 2022

mdelapenya dismissed their stale review via e9d63ed October 27, 2022 05:37

mdelapenya closed this Jun 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve experience in nested container environments #497

Improve experience in nested container environments #497

prskr commented Aug 13, 2022 •

edited

codecov bot commented Aug 19, 2022 •

edited

mdelapenya left a comment

mdelapenya commented Aug 23, 2022

prskr commented Aug 23, 2022

mdelapenya commented Aug 23, 2022

prskr commented Aug 24, 2022

mdelapenya left a comment

mdelapenya Aug 30, 2022

mdelapenya Aug 30, 2022

mdelapenya Aug 30, 2022

mdelapenya Aug 30, 2022

prskr Aug 30, 2022

kiview Aug 30, 2022 •

edited

prskr Aug 31, 2022

prskr commented Aug 30, 2022

mdelapenya commented Aug 30, 2022

kiview commented Aug 30, 2022 •

edited

mdelapenya commented Aug 30, 2022

prskr commented Aug 30, 2022

kiview commented Aug 31, 2022

prskr commented Aug 31, 2022

prskr commented Sep 6, 2022

mdelapenya commented Mar 21, 2023

prskr commented Mar 21, 2023

mdelapenya commented Jun 14, 2023

prskr commented Jun 14, 2023

	test-all: tools test-unit test-e2e test-nested-container
	test-all: tools test-unit test-e2e test-unit-nested

Improve experience in nested container environments #497

Improve experience in nested container environments #497

Conversation

prskr commented Aug 13, 2022 • edited

What does this PR do?

Container env detection

Bind mount remapping

codecov bot commented Aug 19, 2022 • edited

Codecov Report

mdelapenya left a comment

Choose a reason for hiding this comment

mdelapenya commented Aug 23, 2022

prskr commented Aug 23, 2022

mdelapenya commented Aug 23, 2022

prskr commented Aug 24, 2022

mdelapenya left a comment

Choose a reason for hiding this comment

mdelapenya Aug 30, 2022

Choose a reason for hiding this comment

mdelapenya Aug 30, 2022

Choose a reason for hiding this comment

mdelapenya Aug 30, 2022

Choose a reason for hiding this comment

mdelapenya Aug 30, 2022

Choose a reason for hiding this comment

prskr Aug 30, 2022

Choose a reason for hiding this comment

kiview Aug 30, 2022 • edited

Choose a reason for hiding this comment

prskr Aug 31, 2022

Choose a reason for hiding this comment

prskr commented Aug 30, 2022

mdelapenya commented Aug 30, 2022

kiview commented Aug 30, 2022 • edited

mdelapenya commented Aug 30, 2022

prskr commented Aug 30, 2022

kiview commented Aug 31, 2022

prskr commented Aug 31, 2022

prskr commented Sep 6, 2022

mdelapenya commented Mar 21, 2023

prskr commented Mar 21, 2023

mdelapenya commented Jun 14, 2023

prskr commented Jun 14, 2023

prskr commented Aug 13, 2022 •

edited

codecov bot commented Aug 19, 2022 •

edited

kiview Aug 30, 2022 •

edited

kiview commented Aug 30, 2022 •

edited