feat: support reading Docker host from the current Docker context #2810

mdelapenya · 2024-10-02T15:24:28Z

feat: support reading Docker context
chore: move read Docker config to the core
chore: read docker config more consistently
chore: extract to a parse function to detect remote docker hosts
docs: add Docker context to docs

What does this PR do?

This PR adds a new step in the Docekr host discovery algorithm, extracting the Docker host from the current Docker context. To achieve that, we are copying code from the docker/cli project,
as we don't want to receive all the dependencies from that project (please see docker_context.go file). In the end, it just does the following:

list all contexts from the default location: docker config path / context / meta / contextA / meta.json
get the current context from the docker config, located at docker config path / config.json
read the meta.json file for the current context, extracting the host endpoint
use that host endpoint for the Docker Host.

NOTE: The default docker config path lives in $HOME/.docker.

This PR also reduces the complexity in discovering the default bridge network, so instead of having to detect if a network named Bridge exists, it simply delegates to the container runtime to detect it under the hood.

Why is it important?

It will simplify the experience using different container runtimes, as this change means that with just setting the current context, testcontainers-go will use that container runtime.

netlify · 2024-10-02T15:24:45Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`13934da`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/6751909473144700088a1713
😎 Deploy Preview	https://deploy-preview-2810--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

stevenh

quick first pass with some question,.

internal/core/docker_config.go

docs/features/configuration.md

internal/core/docker_context.go

internal/core/docker_context_test.go

stevenh · 2024-10-02T16:50:01Z

internal/core/docker_host.go

+		return dockerHost, nil
+	}
+
+	return "", ErrDockerSocketNotSetInDockerContext


question: Is this discarding useful information from the actual error?

Indeed. Wdyt about this order: return "", errors.Join(ErrDockerSocketNotSetInDockerContext, err)?

Suggested change

return "", ErrDockerSocketNotSetInDockerContext

return "", errors.Join(ErrDockerSocketNotSetInDockerContext, err)

I think that would be confusing, I would detect a specific "not found" error from the call and only translate that if needed. If that's not needed I would just return a wrapped straight wrapped error.

Wdyt a38a345 ?

internal/core/docker_host_test.go

internal/core/testdata/.docker/config.json

No need to have podman-specific code, as it will be read from the docker context

Different container runtimes could produce "Step" or "STEP"

* main: chore: use require.(No)Error(t,err) instead of t.Fatal(err) (testcontainers#2851) fix: simplify fully-qualified image names (testcontainers#2846)

* main: (34 commits) fix: only upload to sonar on ubuntu-latest (testcontainers#2891) fix: build artifact name properly (testcontainers#2890) fix: do not run sonar upload when ryuk is disabled (testcontainers#2889) fix: update GH actions for uploading/downloading artifacts (testcontainers#2888) feat(ci): Enable master moby with rootless (testcontainers#2880) fix(redpanda): temporary file use chore(deps): bump actions/download-artifact from 3.0.2 to 4.1.8 (testcontainers#2676) chore(deps): bump actions/upload-artifact from 3.1.3 to 4.4.3 (testcontainers#2885) fix!: port forwarding clean up and make private (testcontainers#2881) chore: resolve AWS deprecations for localstack (testcontainers#2879) docs: fix new lifecycle hooks section (testcontainers#2875) fix: host access port instability (testcontainers#2867) feat: add build to life cycle hooks (testcontainers#2653) fix: typo in containerd integration (testcontainers#2873) chore(deps): bump mkdocs-include-markdown-plugin from 6.0.4 to 6.2.2 (testcontainers#2806) chore: use testify instead of t.Error (testcontainers#2871) ci: enable perfsprint linter (testcontainers#2872) chore(deps): bump mkdocs-markdownextradata-plugin from 0.2.5 to 0.2.6 (testcontainers#2807) chore: use testcontainers.RequireContainerExec (testcontainers#2870) chore(deps): bump golangci/golangci-lint-action from 6.1.0 to 6.1.1 (testcontainers#2868) ...

stevenh · 2024-11-25T11:43:54Z

docker.go

-	}
-
-	nw, err := p.GetNetwork(ctx, NetworkRequest{Name: defaultNetwork})
+	nw, err := p.GetNetwork(ctx, NetworkRequest{Name: Bridge})


bug: I believe this will fail on podman as the default network there is not bridge but podman.

Nope, the podman engine will retrieve the default bridge network, that's why it's fine to remove all the logic we had for calculating the default network.

➜ testcontainers-go (context-support) ✔ podman network ls NETWORK ID NAME DRIVER 2f259bab93aa podman bridge ➜ testcontainers-go (context-support) ✔ docker network ls NETWORK ID NAME DRIVER SCOPE 2f259bab93aa bridge bridge local ➜ testcontainers-go (context-support) ✔

But this is filtering for Name = "bridge" not type Driver = "bridge" so essentially its running:

docker network ls -f name=bridge

Does that work on podman?

yes, I verified this PR against podman, rancher desktop, colima, orbstack, docker, and testcontainers cloud

docker_test.go

stevenh · 2024-11-25T11:50:23Z

docker_test.go

@@ -1701,7 +1639,7 @@ func TestContainerWithNoUserID(t *testing.T) {
 func TestGetGatewayIP(t *testing.T) {
 	// When using docker compose with DinD mode, and using host port or http wait strategy
 	// It's need to invoke GetGatewayIP for get the host
-	provider, err := providerType.GetProvider(WithLogger(TestLogger(t)))
+	provider, err := ProviderDocker.GetProvider(WithLogger(TestLogger(t)))


question: is the intent to just have one, if so should we rename, also could we avoid exporting this to avoid future breaking changes when we do the client update discussed?

I didn't want to rename here in this PR, but if we see it's related, I can deprecate the existing DockerProvider type and use RuntimeProvider instead.

stevenh · 2024-11-25T12:13:43Z

internal/core/docker_context.go

+	var result []string
+	for _, fi := range fis {
+		if fi.IsDir() {
+			if isContextDir(filepath.Join(root, fi.Name())) {


question: Is this a duplicate check as if we start with root and fi is a directory it should not be possible that this could ever fail?

internal/core/docker_context.go

stevenh · 2024-11-25T12:18:23Z

internal/core/docker_context.go

@@ -0,0 +1,328 @@
+package core


question: can we check this is all required, as it looks like some types don't seem to be referenced.

The only exposed function in this file is GetDockerHostFromCurrentContext, which starts the call to the types and functions here.

internal/core/docker_host.go

* main: docs: better contribution guidelines (testcontainers#2893) fix(influxdb): Respect custom waitStrategy (testcontainers#2845)

internal/config/config.go

internal/core/docker_host.go

* main: fix: container binds syntax (testcontainers#2899) refactor(cockroachdb): to use request driven options (testcontainers#2883) chore(deps): bump actions/setup-go from 5.0.0 to 5.1.0 (testcontainers#2904) chore(deps): bump ossf/scorecard-action from 2.3.1 to 2.4.0 (testcontainers#2903) chore(deps): bump test-summary/action from 2.3 to 2.4 (testcontainers#2902) feat(wait): strategy walk (testcontainers#2895) feat(wait): tls strategy (testcontainers#2896)

mdelapenya added 5 commits October 2, 2024 16:30

feat: support reading Docker context

6c1b3d8

chore: move read Docker config to the core

415c896

chore: read docker config more consistently

1e620f1

chore: extract to a parse function to detect remote docker hosts

ea44c92

docs: add Docker context to docs

549b1cb

mdelapenya added the feature New functionality or new behaviors on the existing one label Oct 2, 2024

mdelapenya self-assigned this Oct 2, 2024

mdelapenya requested a review from stevenh October 2, 2024 15:24

fix: lint

fbefbdf

stevenh requested changes Oct 2, 2024

View reviewed changes

mdelapenya added 19 commits October 3, 2024 13:12

chore: simplify container provider initialisation

1ca3f63

No need to have podman-specific code, as it will be read from the docker context

feat: support configuring the bridge network name

7e1e4b5

chore: rename function

2decbe3

fix: remove wrong copy&paste

c596e37

chore: simplify

ece6ea5

chore: simplify error

a775c4b

chore: simple naming in test

5ac2263

fix: do not lose the original error

932fce2

fix: line endings

9c048f7

fix: case insensitive regex

77cdda6

Different container runtimes could produce "Step" or "STEP"

chore: deprecate reaper_network and always use the bridge one

7eab89f

chore: support replacing the bridge network in the endpoint modifier

1e58328

fix: lint

8abfeba

chore: assume the container runtime uses the default network by default

203d762

chore: rename variable to avoid shading package

464bc9b

fix: if the properties file does not exist, use default config

37a8142

fix: typo

07003ff

fix: use default config in tests

adf0ba1

docs: simplify

b31da78

Merge branch 'main' into context-support

5511d17

* main: chore: use require.(No)Error(t,err) instead of t.Fatal(err) (testcontainers#2851) fix: simplify fully-qualified image names (testcontainers#2846)

mdelapenya mentioned this pull request Oct 28, 2024

fix!: data races #2843

Merged

mdelapenya added 2 commits November 19, 2024 16:25

chore: proper deprecation path

bc2f04b

stevenh requested changes Nov 25, 2024

View reviewed changes

mdelapenya added 13 commits November 25, 2024 14:52

Merge branch 'main' into context-support

dc571d3

* main: docs: better contribution guidelines (testcontainers#2893) fix(influxdb): Respect custom waitStrategy (testcontainers#2845)

chore: comment out empty field used for documentation

ad79b43

chore: use require

4d4769f

docs: update podman commands

56d6774

chore: readability in var comments

1273e02

chore: unique parsing error messages

e7dd83c

docs: readability as function comments

584f6da

chore: idiomatic var declaration

3b59086

chore: extract file name to constant

1c93fe8

chore: simplify default config

92d306f

chore: make errors private

499a05d

chore: simplify function

cdb874d

fix: remove unused func

1933f42

stevenh requested changes Nov 26, 2024

View reviewed changes

internal/config/config.go Outdated Show resolved Hide resolved

internal/config/config.go Show resolved Hide resolved

internal/config/config.go Outdated Show resolved Hide resolved

internal/core/docker_host.go Outdated Show resolved Hide resolved

mdelapenya added 8 commits December 4, 2024 17:44

chore: bubble up config errors when when config is incorrect

f4783a7

chore: use better test names

28f0802

chore: deprecate ReadConfig in favour of NewConfig

f8d24c7

chore: line separator

786d79c

chore: do not export var

4bd9934

chore: bubble up error on environment configuration

8fcd654

chore: rename test cases

13934da

mdelapenya requested a review from stevenh December 5, 2024 11:56

mdelapenya marked this pull request as ready for review December 5, 2024 15:45

mdelapenya requested a review from a team as a code owner December 5, 2024 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support reading Docker host from the current Docker context #2810

feat: support reading Docker host from the current Docker context #2810

mdelapenya commented Oct 2, 2024 •

edited

Loading

netlify bot commented Oct 2, 2024 •

edited

Loading

stevenh left a comment

stevenh Oct 2, 2024

mdelapenya Oct 3, 2024

stevenh Oct 3, 2024

mdelapenya Oct 14, 2024

stevenh Nov 25, 2024

mdelapenya Nov 25, 2024

stevenh Nov 25, 2024 •

edited

Loading

mdelapenya Dec 4, 2024

stevenh Nov 25, 2024

mdelapenya Dec 4, 2024 •

edited

Loading

stevenh Nov 25, 2024

stevenh Nov 25, 2024

mdelapenya Dec 5, 2024

	return "", ErrDockerSocketNotSetInDockerContext
	return "", errors.Join(ErrDockerSocketNotSetInDockerContext, err)

feat: support reading Docker host from the current Docker context #2810

Are you sure you want to change the base?

feat: support reading Docker host from the current Docker context #2810

Conversation

mdelapenya commented Oct 2, 2024 • edited Loading

What does this PR do?

Why is it important?

netlify bot commented Oct 2, 2024 • edited Loading

✅ Deploy Preview for testcontainers-go ready!

stevenh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenh Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdelapenya Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdelapenya commented Oct 2, 2024 •

edited

Loading

netlify bot commented Oct 2, 2024 •

edited

Loading

stevenh Nov 25, 2024 •

edited

Loading

mdelapenya Dec 4, 2024 •

edited

Loading