From 59cf0640e777e92a9b91debd923ac7644949fe84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20de=20la=20Pe=C3=B1a?= Date: Tue, 2 Jul 2024 14:21:16 +0200 Subject: [PATCH] fix: never cache JSON representation of a container (#2606) * chore: wait for the exposed ports before wait strategies * chore: do not cache raw representation of a container while inspecting it * fix: lint * fix: make test deterministic Not using fixed ports is good * fix: consider exposed port has no protocol * fix: lint --- docker.go | 32 ++++++++++----------------- docker_auth_test.go | 39 +++++++++++++++++++++----------- docker_test.go | 5 ----- lifecycle.go | 48 ++++++++++++++++++++++++++++++++++++++++ port_forwarding.go | 2 +- testdata/auth.Dockerfile | 4 +++- 6 files changed, 90 insertions(+), 40 deletions(-) diff --git a/docker.go b/docker.go index dbcddf4745..9e83c2e346 100644 --- a/docker.go +++ b/docker.go @@ -57,9 +57,10 @@ var createContainerFailDueToNameConflictRegex = regexp.MustCompile("Conflict. Th // DockerContainer represents a container started using Docker type DockerContainer struct { // Container ID from Docker - ID string - WaitingFor wait.Strategy - Image string + ID string + WaitingFor wait.Strategy + Image string + exposedPorts []string // a reference to the container's requested exposed ports. It allows checking they are ready before any wait strategy isRunning bool imageWasBuilt bool @@ -69,7 +70,6 @@ type DockerContainer struct { sessionID string terminationSignal chan bool consumers []LogConsumer - raw *types.ContainerJSON logProductionError chan error // TODO: Remove locking and wait group once the deprecated StartLogProducer and @@ -165,12 +165,8 @@ func (c *DockerContainer) Host(ctx context.Context) (string, error) { return host, nil } -// Inspect gets the raw container info, caching the result for subsequent calls +// Inspect gets the raw container info func (c *DockerContainer) Inspect(ctx context.Context) (*types.ContainerJSON, error) { - if c.raw != nil { - return c.raw, nil - } - jsonRaw, err := c.inspectRawContainer(ctx) if err != nil { return nil, err @@ -277,7 +273,6 @@ func (c *DockerContainer) Stop(ctx context.Context, timeout *time.Duration) erro defer c.provider.Close() c.isRunning = false - c.raw = nil // invalidate the cache, as the container representation will change after stopping err = c.stoppedHook(ctx) if err != nil { @@ -316,7 +311,7 @@ func (c *DockerContainer) Terminate(ctx context.Context) error { c.sessionID = "" c.isRunning = false - c.raw = nil // invalidate the cache here too + return errors.Join(errs...) } @@ -328,8 +323,7 @@ func (c *DockerContainer) inspectRawContainer(ctx context.Context) (*types.Conta return nil, err } - c.raw = &inspect - return c.raw, nil + return &inspect, nil } // Logs will fetch both STDOUT and STDERR from the current container. Returns a @@ -407,14 +401,10 @@ func (c *DockerContainer) Name(ctx context.Context) (string, error) { return inspect.Name, nil } -// State returns container's running state. This method does not use the cache -// and always fetches the latest state from the Docker daemon. +// State returns container's running state. func (c *DockerContainer) State(ctx context.Context) (*types.ContainerState, error) { inspect, err := c.inspectRawContainer(ctx) if err != nil { - if c.raw != nil { - return c.raw.State, err - } return nil, err } return inspect.State, nil @@ -1152,6 +1142,7 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque imageWasBuilt: req.ShouldBuildImage(), keepBuiltImage: req.ShouldKeepBuiltImage(), sessionID: core.SessionID(), + exposedPorts: req.ExposedPorts, provider: p, terminationSignal: termSignal, logger: p.Logger, @@ -1253,6 +1244,7 @@ func (p *DockerProvider) ReuseOrCreateContainer(ctx context.Context, req Contain WaitingFor: req.WaitingFor, Image: c.Image, sessionID: sessionID, + exposedPorts: req.ExposedPorts, provider: p, terminationSignal: termSignal, logger: p.Logger, @@ -1576,13 +1568,13 @@ func containerFromDockerResponse(ctx context.Context, response types.Container) ctr.terminationSignal = nil // populate the raw representation of the container - _, err = ctr.inspectRawContainer(ctx) + jsonRaw, err := ctr.inspectRawContainer(ctx) if err != nil { return nil, err } // the health status of the container, if any - if health := ctr.raw.State.Health; health != nil { + if health := jsonRaw.State.Health; health != nil { ctr.healthStatus = health.Status } diff --git a/docker_auth_test.go b/docker_auth_test.go index b411568171..1f8df17c09 100644 --- a/docker_auth_test.go +++ b/docker_auth_test.go @@ -223,23 +223,26 @@ func removeImageFromLocalCache(t *testing.T, img string) { } func TestBuildContainerFromDockerfileWithDockerAuthConfig(t *testing.T) { + mappedPort := prepareLocalRegistryWithAuth(t) + // using the same credentials as in the Docker Registry base64 := "dGVzdHVzZXI6dGVzdHBhc3N3b3Jk" // testuser:testpassword t.Setenv("DOCKER_AUTH_CONFIG", `{ "auths": { - "localhost:5001": { "username": "testuser", "password": "testpassword", "auth": "`+base64+`" } + "localhost:`+mappedPort+`": { "username": "testuser", "password": "testpassword", "auth": "`+base64+`" } }, "credsStore": "desktop" }`) - prepareLocalRegistryWithAuth(t) - ctx := context.Background() req := ContainerRequest{ FromDockerfile: FromDockerfile{ Context: "./testdata", Dockerfile: "auth.Dockerfile", + BuildArgs: map[string]*string{ + "REGISTRY_PORT": &mappedPort, + }, }, AlwaysPullImage: true, // make sure the authentication takes place ExposedPorts: []string{"6379/tcp"}, @@ -252,23 +255,26 @@ func TestBuildContainerFromDockerfileWithDockerAuthConfig(t *testing.T) { } func TestBuildContainerFromDockerfileShouldFailWithWrongDockerAuthConfig(t *testing.T) { + mappedPort := prepareLocalRegistryWithAuth(t) + // using different credentials than in the Docker Registry base64 := "Zm9vOmJhcg==" // foo:bar t.Setenv("DOCKER_AUTH_CONFIG", `{ "auths": { - "localhost:5001": { "username": "foo", "password": "bar", "auth": "`+base64+`" } + "localhost:`+mappedPort+`": { "username": "foo", "password": "bar", "auth": "`+base64+`" } }, "credsStore": "desktop" }`) - prepareLocalRegistryWithAuth(t) - ctx := context.Background() req := ContainerRequest{ FromDockerfile: FromDockerfile{ Context: "./testdata", Dockerfile: "auth.Dockerfile", + BuildArgs: map[string]*string{ + "REGISTRY_PORT": &mappedPort, + }, }, AlwaysPullImage: true, // make sure the authentication takes place ExposedPorts: []string{"6379/tcp"}, @@ -281,20 +287,20 @@ func TestBuildContainerFromDockerfileShouldFailWithWrongDockerAuthConfig(t *test } func TestCreateContainerFromPrivateRegistry(t *testing.T) { + mappedPort := prepareLocalRegistryWithAuth(t) + // using the same credentials as in the Docker Registry base64 := "dGVzdHVzZXI6dGVzdHBhc3N3b3Jk" // testuser:testpassword t.Setenv("DOCKER_AUTH_CONFIG", `{ "auths": { - "localhost:5001": { "username": "testuser", "password": "testpassword", "auth": "`+base64+`" } + "localhost:`+mappedPort+`": { "username": "testuser", "password": "testpassword", "auth": "`+base64+`" } }, "credsStore": "desktop" }`) - prepareLocalRegistryWithAuth(t) - ctx := context.Background() req := ContainerRequest{ - Image: "localhost:5001/redis:5.0-alpine", + Image: "localhost:" + mappedPort + "/redis:5.0-alpine", AlwaysPullImage: true, // make sure the authentication takes place ExposedPorts: []string{"6379/tcp"}, WaitingFor: wait.ForLog("Ready to accept connections"), @@ -308,14 +314,14 @@ func TestCreateContainerFromPrivateRegistry(t *testing.T) { terminateContainerOnEnd(t, ctx, redisContainer) } -func prepareLocalRegistryWithAuth(t *testing.T) { +func prepareLocalRegistryWithAuth(t *testing.T) string { ctx := context.Background() wd, err := os.Getwd() require.NoError(t, err) // copyDirectoryToContainer { req := ContainerRequest{ Image: "registry:2", - ExposedPorts: []string{"5001:5000/tcp"}, + ExposedPorts: []string{"5000/tcp"}, Env: map[string]string{ "REGISTRY_AUTH": "htpasswd", "REGISTRY_AUTH_HTPASSWD_REALM": "Registry", @@ -345,8 +351,13 @@ func prepareLocalRegistryWithAuth(t *testing.T) { registryC, err := GenericContainer(ctx, genContainerReq) require.NoError(t, err) + mappedPort, err := registryC.MappedPort(ctx, "5000/tcp") + require.NoError(t, err) + + mp := mappedPort.Port() + t.Cleanup(func() { - removeImageFromLocalCache(t, "localhost:5001/redis:5.0-alpine") + removeImageFromLocalCache(t, "localhost:"+mp+"/redis:5.0-alpine") }) t.Cleanup(func() { require.NoError(t, registryC.Terminate(context.Background())) @@ -354,6 +365,8 @@ func prepareLocalRegistryWithAuth(t *testing.T) { _, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) + + return mp } func prepareRedisImage(ctx context.Context, req ContainerRequest, t *testing.T) (Container, error) { diff --git a/docker_test.go b/docker_test.go index b6fc25a23a..38ab7fec2a 100644 --- a/docker_test.go +++ b/docker_test.go @@ -1338,11 +1338,6 @@ func TestContainerInspect_RawInspectIsCleanedOnStop(t *testing.T) { assert.NotEmpty(t, inspect.ID) require.NoError(t, ctr.Stop(context.Background(), nil)) - - // type assertion to ensure that the container is a DockerContainer - dc := ctr.(*DockerContainer) - - assert.Nil(t, dc.raw) } func readHostname(tb testing.TB, containerId string) string { diff --git a/lifecycle.go b/lifecycle.go index f46318ce7c..a98329a0b4 100644 --- a/lifecycle.go +++ b/lifecycle.go @@ -6,7 +6,9 @@ import ( "fmt" "io" "strings" + "time" + "github.com/cenkalti/backoff/v4" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" "github.com/docker/go-connections/nat" @@ -200,6 +202,52 @@ var defaultLogConsumersHook = func(cfg *LogConsumerConfig) ContainerLifecycleHoo var defaultReadinessHook = func() ContainerLifecycleHooks { return ContainerLifecycleHooks{ PostStarts: []ContainerHook{ + func(ctx context.Context, c Container) error { + // wait until all the exposed ports are mapped: + // it will be ready when all the exposed ports are mapped, + // checking every 50ms, up to 5s, and failing if all the + // exposed ports are not mapped in that time. + dockerContainer := c.(*DockerContainer) + + b := backoff.NewExponentialBackOff() + + b.InitialInterval = 50 * time.Millisecond + b.MaxElapsedTime = 1 * time.Second + b.MaxInterval = 5 * time.Second + + err := backoff.Retry(func() error { + jsonRaw, err := dockerContainer.inspectRawContainer(ctx) + if err != nil { + return err + } + + exposedAndMappedPorts := jsonRaw.NetworkSettings.Ports + + for _, exposedPort := range dockerContainer.exposedPorts { + portMap := nat.Port(exposedPort) + // having entries in exposedAndMappedPorts, where the key is the exposed port, + // and the value is the mapped port, means that the port has been already mapped. + if _, ok := exposedAndMappedPorts[portMap]; !ok { + // check if the port is mapped with the protocol (default is TCP) + if !strings.Contains(exposedPort, "/") { + portMap = nat.Port(fmt.Sprintf("%s/tcp", exposedPort)) + if _, ok := exposedAndMappedPorts[portMap]; !ok { + return fmt.Errorf("port %s is not mapped yet", exposedPort) + } + } else { + return fmt.Errorf("port %s is not mapped yet", exposedPort) + } + } + } + + return nil + }, b) + if err != nil { + return fmt.Errorf("all exposed ports, %s, were not mapped in 5s: %w", dockerContainer.exposedPorts, err) + } + + return nil + }, // wait for the container to be ready func(ctx context.Context, c Container) error { dockerContainer := c.(*DockerContainer) diff --git a/port_forwarding.go b/port_forwarding.go index f1b3962247..89aac10dd7 100644 --- a/port_forwarding.go +++ b/port_forwarding.go @@ -24,7 +24,7 @@ const ( // using the SSHD container as a bridge. HostInternal string = "host.testcontainers.internal" user string = "root" - sshPort = "22" + sshPort = "22/tcp" ) // sshPassword is a random password generated for the SSHD container. diff --git a/testdata/auth.Dockerfile b/testdata/auth.Dockerfile index 1f57898cba..367e228a67 100644 --- a/testdata/auth.Dockerfile +++ b/testdata/auth.Dockerfile @@ -1 +1,3 @@ -FROM localhost:5001/redis:5.0-alpine +ARG REGISTRY_PORT=5001 + +FROM localhost:${REGISTRY_PORT}/redis:5.0-alpine