From 66f17a75956f3ef960a96837e3844b587422061c Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Mon, 28 Oct 2024 12:51:02 +0000 Subject: [PATCH] fix(compose): container initialisation (#2844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix compose to fully initialise the containers it returns. This ensures that running things like checks for running behave as expected. Extracts the functionality to connect to reaper into a helper method so its consistent across uses. Fix data race in daemonHost function converting it to a method to make use of encapsulation. Fix container and network requests so they use sessionID from labels if available so that user specified values are respected. Export the functionality to create a container from a ContainerList response via provider.ContainerFromType. Enforce no bare returns instead of no named returns as that was the original intention. Fixes #2667 Co-authored-by: Manuel de la Peña --- .golangci.yml | 4 +- container.go | 9 ++ docker.go | 158 ++++++++++++++++------------ generic_test.go | 6 +- modules/compose/compose.go | 11 +- modules/compose/compose_api.go | 31 ++---- modules/compose/compose_api_test.go | 6 ++ network.go | 11 ++ reaper.go | 17 ++- 9 files changed, 154 insertions(+), 99 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 26f8f8a3c2..7c421f0be6 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -7,11 +7,13 @@ linters: - gofumpt - misspell - nolintlint - - nonamedreturns + - nakedret - testifylint - thelper linters-settings: + nakedret: + max-func-lines: 0 errorlint: # Check whether fmt.Errorf uses the %w verb for formatting errors. # See the https://github.com/polyfloyd/go-errorlint for caveats. diff --git a/container.go b/container.go index d114a5988a..c0ae935f07 100644 --- a/container.go +++ b/container.go @@ -167,6 +167,15 @@ type ContainerRequest struct { LogConsumerCfg *LogConsumerConfig // define the configuration for the log producer and its log consumers to follow the logs } +// sessionID returns the session ID for the container request. +func (c *ContainerRequest) sessionID() string { + if sessionID := c.Labels[core.LabelSessionID]; sessionID != "" { + return sessionID + } + + return core.SessionID() +} + // containerOptions functional options for a container type containerOptions struct { ImageName string diff --git a/docker.go b/docker.go index f82cd55381..66ac42eda6 100644 --- a/docker.go +++ b/docker.go @@ -15,7 +15,6 @@ import ( "os" "path/filepath" "regexp" - "strings" "sync" "time" @@ -889,6 +888,32 @@ func (c *DockerContainer) GetLogProductionErrorChannel() <-chan error { return errCh } +// connectReaper connects the reaper to the container if it is needed. +func (c *DockerContainer) connectReaper(ctx context.Context) error { + if c.provider.config.RyukDisabled || isReaperImage(c.Image) { + // Reaper is disabled or we are the reaper container. + return nil + } + + reaper, err := spawner.reaper(context.WithValue(ctx, core.DockerHostContextKey, c.provider.host), core.SessionID(), c.provider) + if err != nil { + return fmt.Errorf("reaper: %w", err) + } + + if c.terminationSignal, err = reaper.Connect(); err != nil { + return fmt.Errorf("reaper connect: %w", err) + } + + return nil +} + +// cleanupTermSignal triggers the termination signal if it was created and an error occurred. +func (c *DockerContainer) cleanupTermSignal(err error) { + if c.terminationSignal != nil && err != nil { + c.terminationSignal <- true + } +} + // DockerNetwork represents a network started using Docker type DockerNetwork struct { ID string // Network ID from Docker @@ -1035,28 +1060,6 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque req.Labels = make(map[string]string) } - var termSignal chan bool - // the reaper does not need to start a reaper for itself - isReaperContainer := strings.HasSuffix(imageName, config.ReaperDefaultImage) - if !p.config.RyukDisabled && !isReaperContainer { - r, err := spawner.reaper(context.WithValue(ctx, core.DockerHostContextKey, p.host), core.SessionID(), p) - if err != nil { - return nil, fmt.Errorf("reaper: %w", err) - } - - termSignal, err := r.Connect() - if err != nil { - return nil, fmt.Errorf("reaper connect: %w", err) - } - - // Cleanup on error. - defer func() { - if err != nil { - termSignal <- true - } - }() - } - if err = req.Validate(); err != nil { return nil, err } @@ -1120,7 +1123,7 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque } } - if !isReaperContainer { + if !isReaperImage(imageName) { // Add the labels that identify this as a testcontainers container and // allow the reaper to terminate it if requested. AddGenericLabels(req.Labels) @@ -1198,26 +1201,35 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque } } - c := &DockerContainer{ - ID: resp.ID, - WaitingFor: req.WaitingFor, - Image: imageName, - imageWasBuilt: req.ShouldBuildImage(), - keepBuiltImage: req.ShouldKeepBuiltImage(), - sessionID: core.SessionID(), - exposedPorts: req.ExposedPorts, - provider: p, - terminationSignal: termSignal, - logger: p.Logger, - lifecycleHooks: req.LifecycleHooks, + // This should match the fields set in ContainerFromDockerResponse. + ctr := &DockerContainer{ + ID: resp.ID, + WaitingFor: req.WaitingFor, + Image: imageName, + imageWasBuilt: req.ShouldBuildImage(), + keepBuiltImage: req.ShouldKeepBuiltImage(), + sessionID: req.sessionID(), + exposedPorts: req.ExposedPorts, + provider: p, + logger: p.Logger, + lifecycleHooks: req.LifecycleHooks, } - err = c.createdHook(ctx) - if err != nil { - return nil, err + if err = ctr.connectReaper(ctx); err != nil { + return ctr, err // No wrap as it would stutter. } - return c, nil + // Wrapped so the returned error is passed to the cleanup function. + defer func(ctr *DockerContainer) { + ctr.cleanupTermSignal(err) + }(ctr) + + if err = ctr.createdHook(ctx); err != nil { + // Return the container to allow caller to clean up. + return ctr, fmt.Errorf("created hook: %w", err) + } + + return ctr, nil } func (p *DockerProvider) findContainerByName(ctx context.Context, name string) (*types.Container, error) { @@ -1229,7 +1241,7 @@ func (p *DockerProvider) findContainerByName(ctx context.Context, name string) ( filter := filters.NewArgs(filters.Arg("name", fmt.Sprintf("^%s$", name))) containers, err := p.client.ContainerList(ctx, container.ListOptions{Filters: filter}) if err != nil { - return nil, err + return nil, fmt.Errorf("container list: %w", err) } defer p.Close() @@ -1284,7 +1296,7 @@ func (p *DockerProvider) ReuseOrCreateContainer(ctx context.Context, req Contain } } - sessionID := core.SessionID() + sessionID := req.sessionID() var termSignal chan bool if !p.config.RyukDisabled { @@ -1425,10 +1437,13 @@ func (p *DockerProvider) Config() TestcontainersConfig { // Warning: this is based on your Docker host setting. Will fail if using an SSH tunnel // You can use the "TESTCONTAINERS_HOST_OVERRIDE" env variable to set this yourself func (p *DockerProvider) DaemonHost(ctx context.Context) (string, error) { - return daemonHost(ctx, p) + p.mtx.Lock() + defer p.mtx.Unlock() + + return p.daemonHostLocked(ctx) } -func daemonHost(ctx context.Context, p *DockerProvider) (string, error) { +func (p *DockerProvider) daemonHostLocked(ctx context.Context) (string, error) { if p.hostCache != "" { return p.hostCache, nil } @@ -1492,7 +1507,7 @@ func (p *DockerProvider) CreateNetwork(ctx context.Context, req NetworkRequest) IPAM: req.IPAM, } - sessionID := core.SessionID() + sessionID := req.sessionID() var termSignal chan bool if !p.config.RyukDisabled { @@ -1617,37 +1632,42 @@ func (p *DockerProvider) ensureDefaultNetwork(ctx context.Context) (string, erro return p.defaultNetwork, nil } -// containerFromDockerResponse builds a Docker container struct from the response of the Docker API -func containerFromDockerResponse(ctx context.Context, response types.Container) (*DockerContainer, error) { - provider, err := NewDockerProvider() - if err != nil { - return nil, err +// ContainerFromType builds a Docker container struct from the response of the Docker API +func (p *DockerProvider) ContainerFromType(ctx context.Context, response types.Container) (ctr *DockerContainer, err error) { + exposedPorts := make([]string, len(response.Ports)) + for i, port := range response.Ports { + exposedPorts[i] = fmt.Sprintf("%d/%s", port.PublicPort, port.Type) + } + + // This should match the fields set in CreateContainer. + ctr = &DockerContainer{ + ID: response.ID, + Image: response.Image, + imageWasBuilt: false, + sessionID: response.Labels[core.LabelSessionID], + isRunning: response.State == "running", + exposedPorts: exposedPorts, + provider: p, + logger: p.Logger, + lifecycleHooks: []ContainerLifecycleHooks{ + DefaultLoggingHook(p.Logger), + }, } - ctr := DockerContainer{} - - ctr.ID = response.ID - ctr.WaitingFor = nil - ctr.Image = response.Image - ctr.imageWasBuilt = false - - ctr.logger = provider.Logger - ctr.lifecycleHooks = []ContainerLifecycleHooks{ - DefaultLoggingHook(ctr.logger), + if err = ctr.connectReaper(ctx); err != nil { + return nil, err } - ctr.provider = provider - - ctr.sessionID = core.SessionID() - ctr.consumers = []LogConsumer{} - ctr.isRunning = response.State == "running" - // the termination signal should be obtained from the reaper - ctr.terminationSignal = nil + // Wrapped so the returned error is passed to the cleanup function. + defer func(ctr *DockerContainer) { + ctr.cleanupTermSignal(err) + }(ctr) // populate the raw representation of the container jsonRaw, err := ctr.inspectRawContainer(ctx) if err != nil { - return nil, fmt.Errorf("inspect raw container: %w", err) + // Return the container to allow caller to clean up. + return ctr, fmt.Errorf("inspect raw container: %w", err) } // the health status of the container, if any @@ -1655,7 +1675,7 @@ func containerFromDockerResponse(ctx context.Context, response types.Container) ctr.healthStatus = health.Status } - return &ctr, nil + return ctr, nil } // ListImages list images from the provider. If an image has multiple Tags, each tag is reported diff --git a/generic_test.go b/generic_test.go index 250a9aaab2..7c0de2a246 100644 --- a/generic_test.go +++ b/generic_test.go @@ -156,10 +156,14 @@ func TestGenericReusableContainerInSubprocess(t *testing.T) { require.NoError(t, err) require.Len(t, ctrs, 1) - nginxC, err := containerFromDockerResponse(context.Background(), ctrs[0]) + provider, err := NewDockerProvider() require.NoError(t, err) + provider.SetClient(cli) + + nginxC, err := provider.ContainerFromType(context.Background(), ctrs[0]) CleanupContainer(t, nginxC) + require.NoError(t, err) } func createReuseContainerInSubprocess(t *testing.T) string { diff --git a/modules/compose/compose.go b/modules/compose/compose.go index fa02cde077..be829f4575 100644 --- a/modules/compose/compose.go +++ b/modules/compose/compose.go @@ -153,6 +153,14 @@ func NewDockerComposeWith(opts ...ComposeStackOption) (*dockerCompose, error) { return nil, fmt.Errorf("initialize docker client: %w", err) } + provider, err := testcontainers.NewDockerProvider(testcontainers.WithLogger(composeOptions.Logger)) + if err != nil { + return nil, fmt.Errorf("new docker provider: %w", err) + } + + dockerClient := dockerCli.Client() + provider.SetClient(dockerClient) + composeAPI := &dockerCompose{ name: composeOptions.Identifier, configs: composeOptions.Paths, @@ -160,11 +168,12 @@ func NewDockerComposeWith(opts ...ComposeStackOption) (*dockerCompose, error) { logger: composeOptions.Logger, projectProfiles: composeOptions.Profiles, composeService: compose.NewComposeService(dockerCli), - dockerClient: dockerCli.Client(), + dockerClient: dockerClient, waitStrategies: make(map[string]wait.Strategy), containers: make(map[string]*testcontainers.DockerContainer), networks: make(map[string]*testcontainers.DockerNetwork), sessionID: testcontainers.SessionID(), + provider: provider, } return composeAPI, nil diff --git a/modules/compose/compose_api.go b/modules/compose/compose_api.go index c84f1af6ce..45dd72c6e0 100644 --- a/modules/compose/compose_api.go +++ b/modules/compose/compose_api.go @@ -229,6 +229,9 @@ type dockerCompose struct { // sessionID is used to identify the reaper session sessionID string + + // provider is used to docker operations. + provider *testcontainers.DockerProvider } func (d *dockerCompose) ServiceContainer(ctx context.Context, svcName string) (*testcontainers.DockerContainer, error) { @@ -325,17 +328,12 @@ func (d *dockerCompose) Up(ctx context.Context, opts ...StackUpOption) (err erro return err } - provider, err := testcontainers.NewDockerProvider(testcontainers.WithLogger(d.logger)) - if err != nil { - return fmt.Errorf("new docker provider: %w", err) - } - var termSignals []chan bool var reaper *testcontainers.Reaper - if !provider.Config().Config.RyukDisabled { + if !d.provider.Config().Config.RyukDisabled { // NewReaper is deprecated: we need to find a way to create the reaper for compose // bypassing the deprecation. - reaper, err = testcontainers.NewReaper(ctx, testcontainers.SessionID(), provider, "") + reaper, err = testcontainers.NewReaper(ctx, testcontainers.SessionID(), d.provider, "") if err != nil { return fmt.Errorf("create reaper: %w", err) } @@ -492,26 +490,11 @@ func (d *dockerCompose) lookupContainer(ctx context.Context, svcName string) (*t return nil, fmt.Errorf("no container found for service name %s", svcName) } - containerInstance := containers[0] - // TODO: Fix as this is only setting a subset of the fields - // and the container is not fully initialized, for example - // the isRunning flag is not set. - // See: https://github.com/testcontainers/testcontainers-go/issues/2667 - ctr := &testcontainers.DockerContainer{ - ID: containerInstance.ID, - Image: containerInstance.Image, - } - ctr.SetLogger(d.logger) - - dockerProvider, err := testcontainers.NewDockerProvider(testcontainers.WithLogger(d.logger)) + ctr, err := d.provider.ContainerFromType(ctx, containers[0]) if err != nil { - return nil, fmt.Errorf("new docker provider: %w", err) + return nil, fmt.Errorf("container from type: %w", err) } - dockerProvider.SetClient(d.dockerClient) - - ctr.SetProvider(dockerProvider) - d.containersLock.Lock() defer d.containersLock.Unlock() d.containers[svcName] = ctr diff --git a/modules/compose/compose_api_test.go b/modules/compose/compose_api_test.go index e13c6ca937..310057ff49 100644 --- a/modules/compose/compose_api_test.go +++ b/modules/compose/compose_api_test.go @@ -33,6 +33,12 @@ func TestDockerComposeAPI(t *testing.T) { err = compose.Up(ctx, Wait(true)) cleanup(t, compose) require.NoError(t, err, "compose.Up()") + + for _, service := range compose.Services() { + container, err := compose.ServiceContainer(context.Background(), service) + require.NoError(t, err, "compose.ServiceContainer()") + require.True(t, container.IsRunning()) + } } func TestDockerComposeAPIStrategyForInvalidService(t *testing.T) { diff --git a/network.go b/network.go index 5a145ac668..e0cc83f510 100644 --- a/network.go +++ b/network.go @@ -4,6 +4,8 @@ import ( "context" "github.com/docker/docker/api/types/network" + + "github.com/testcontainers/testcontainers-go/internal/core" ) // NetworkProvider allows the creation of networks on an arbitrary system @@ -47,3 +49,12 @@ type NetworkRequest struct { ReaperImage string // Deprecated: use WithImageName ContainerOption instead. Alternative reaper registry ReaperOptions []ContainerOption // Deprecated: the reaper is configured at the properties level, for an entire test session } + +// sessionID returns the session ID for the network request. +func (r NetworkRequest) sessionID() string { + if sessionID := r.Labels[core.LabelSessionID]; sessionID != "" { + return sessionID + } + + return core.SessionID() +} diff --git a/reaper.go b/reaper.go index 650bfad0bd..dfdcea5c5f 100644 --- a/reaper.go +++ b/reaper.go @@ -161,6 +161,13 @@ func (r *reaperSpawner) lookupContainer(ctx context.Context, sessionID string) ( } defer dockerClient.Close() + provider, err := NewDockerProvider() + if err != nil { + return nil, fmt.Errorf("new provider: %w", err) + } + + provider.SetClient(dockerClient) + opts := container.ListOptions{ All: true, Filters: filters.NewArgs( @@ -184,11 +191,10 @@ func (r *reaperSpawner) lookupContainer(ctx context.Context, sessionID string) ( } if len(resp) > 1 { - return nil, fmt.Errorf("multiple reaper containers found for session ID %s", sessionID) + return nil, fmt.Errorf("found %d reaper containers for session ID %q", len(resp), sessionID) } - container := resp[0] - r, err := containerFromDockerResponse(ctx, container) + r, err := provider.ContainerFromType(ctx, resp[0]) if err != nil { return nil, fmt.Errorf("from docker: %w", err) } @@ -566,3 +572,8 @@ func (r *Reaper) handshake(conn net.Conn) error { func (r *Reaper) Labels() map[string]string { return GenericLabels() } + +// isReaperImage returns true if the image name is the reaper image. +func isReaperImage(name string) bool { + return strings.HasSuffix(name, config.ReaperDefaultImage) +}