Skip to content

Commit

Permalink
fix: avoid double lock in DockerProvider.DaemonHost() (#2900)
Browse files Browse the repository at this point in the history
* avoid double lock in DockerProvider.DaemonHost()

* cleaner structure

* put comment back

* add regression test

* use require

* test improvements

* better error output

* try to fix rootless mode

* pass on XDG_RUNTIME_DIR

* fix: DaemonHost locking test

Fix the DaemonHost locking test by implementing a way to change the
location of the file the core library tests for.

---------

Co-authored-by: Steven Hartland <[email protected]>
  • Loading branch information
vikstrous and stevenh authored Dec 20, 2024
1 parent ea4feea commit abe0f82
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 2 deletions.
12 changes: 11 additions & 1 deletion docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -1495,7 +1495,11 @@ func (p *DockerProvider) daemonHostLocked(ctx context.Context) (string, error) {
p.hostCache = daemonURL.Hostname()
case "unix", "npipe":
if core.InAContainer() {
ip, err := p.GetGatewayIP(ctx)
defaultNetwork, err := p.ensureDefaultNetworkLocked(ctx)
if err != nil {
return "", fmt.Errorf("ensure default network: %w", err)
}
ip, err := p.getGatewayIP(ctx, defaultNetwork)
if err != nil {
ip, err = core.DefaultGatewayIP()
if err != nil {
Expand Down Expand Up @@ -1595,7 +1599,10 @@ func (p *DockerProvider) GetGatewayIP(ctx context.Context) (string, error) {
if err != nil {
return "", fmt.Errorf("ensure default network: %w", err)
}
return p.getGatewayIP(ctx, defaultNetwork)
}

func (p *DockerProvider) getGatewayIP(ctx context.Context, defaultNetwork string) (string, error) {
nw, err := p.GetNetwork(ctx, NetworkRequest{Name: defaultNetwork})
if err != nil {
return "", err
Expand All @@ -1621,7 +1628,10 @@ func (p *DockerProvider) GetGatewayIP(ctx context.Context) (string, error) {
func (p *DockerProvider) ensureDefaultNetwork(ctx context.Context) (string, error) {
p.mtx.Lock()
defer p.mtx.Unlock()
return p.ensureDefaultNetworkLocked(ctx)
}

func (p *DockerProvider) ensureDefaultNetworkLocked(ctx context.Context) (string, error) {
if p.defaultNetwork != "" {
// Already set.
return p.defaultNetwork, nil
Expand Down
38 changes: 38 additions & 0 deletions docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/testcontainers/testcontainers-go/internal/core"
"github.com/testcontainers/testcontainers-go/wait"
)

Expand All @@ -35,6 +36,7 @@ const (
nginxAlpineImage = "nginx:alpine"
nginxDefaultPort = "80/tcp"
nginxHighPort = "8080/tcp"
golangImage = "golang"
daemonMaxVersion = "1.41"
)

Expand Down Expand Up @@ -2156,3 +2158,39 @@ func TestCustomPrefixTrailingSlashIsProperlyRemovedIfPresent(t *testing.T) {
dockerContainer := c.(*DockerContainer)
require.Equal(t, fmt.Sprintf("%s%s", hubPrefixWithTrailingSlash, dockerImage), dockerContainer.Image)
}

// TODO: remove this skip check when context rework is merged alongside [core.DockerEnvFile] removal.
func Test_Provider_DaemonHost_Issue2897(t *testing.T) {
ctx := context.Background()
provider, err := NewDockerProvider()
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, provider.Close())
})

orig := core.DockerEnvFile
core.DockerEnvFile = filepath.Join(t.TempDir(), ".dockerenv")
t.Cleanup(func() {
core.DockerEnvFile = orig
})

f, err := os.Create(core.DockerEnvFile)
require.NoError(t, err)
require.NoError(t, f.Close())
t.Cleanup(func() {
require.NoError(t, os.Remove(f.Name()))
})

errCh := make(chan error, 1)
go func() {
_, err := provider.DaemonHost(ctx)
errCh <- err
}()

select {
case <-time.After(1 * time.Second):
t.Fatal("timeout waiting for DaemonHost")
case err := <-errCh:
require.NoError(t, err)
}
}
7 changes: 6 additions & 1 deletion internal/core/docker_host.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,10 +309,15 @@ func testcontainersHostFromProperties(ctx context.Context) (string, error) {
return "", ErrTestcontainersHostNotSetInProperties
}

// DockerEnvFile is the file that is created when running inside a container.
// It's a variable to allow testing.
// TODO: Remove this once context rework is done, which eliminates need for the default network creation.
var DockerEnvFile = "/.dockerenv"

// InAContainer returns true if the code is running inside a container
// See https://github.com/docker/docker/blob/a9fa38b1edf30b23cae3eade0be48b3d4b1de14b/daemon/initlayer/setup_unix.go#L25
func InAContainer() bool {
return inAContainer("/.dockerenv")
return inAContainer(DockerEnvFile)
}

func inAContainer(path string) bool {
Expand Down

0 comments on commit abe0f82

Please sign in to comment.