cmd/atlas/docker: Revert let Docker Engine allocate a free port (#3500)

This reverts commit 25065bc.

Author: giautm

cmd/atlas/internal/docker/docker.go

@@ -11,10 +11,12 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"net"
 	"net/url"
 	"os"
 	"os/exec"
 	"path"
+	"strconv"
 	"strings"
 	"time"
 
@@ -50,9 +52,11 @@ type (
 	}
 	// A Container is an instance of a created container.
 	Container struct {
-		Config   // Config used to create this container
-		ID       string
-		HostPort string // Host port to connect to
+		Config // Config used to create this container
+		// ID of the container.
+		ID string
+		// Port on the host this containers service is bound to.
+		Port string
 	}
 	// ConnOptions allows configuring the underlying connection pool.
 	ConnOptions struct {
@@ -384,54 +388,40 @@ func Conn(s *ConnOptions) ConfigOption {
 
 // Run pulls and starts a new docker container from the Config.
 func (c *Config) Run(ctx context.Context) (*Container, error) {
-	if c == nil || c.Image == "" || c.Port == "" || c.Out == nil {
-		return nil, fmt.Errorf("docker: invalid configuration %#v", c)
-	}
-	args := []string{"run", "--rm", "--detach", "--publish", c.Port}
-	for _, e := range c.Env {
-		args = append(args, "--env", e)
-	}
-	args = append(args, c.Image)
-	id, err := c.docker(ctx, args...)
-	if err != nil {
-		return nil, fmt.Errorf("docker: run container: %w", err)
+	// Make sure the configuration is not missing critical values.
+	if err := c.validate(); err != nil {
+		return nil, err
 	}
-	hostPort, err := c.docker(ctx, "port", id, c.Port)
+	// Get a free host TCP port the container can bind its exposed service port on.
+	p, err := freePort()
 	if err != nil {
-		return nil, fmt.Errorf("docker: get container port: %w", err)
+		return nil, fmt.Errorf("getting open port: %w", err)
 	}
-	return &Container{
-		Config:   *c,
-		ID:       id,
-		HostPort: strings.SplitN(hostPort, "\n", 2)[0],
-	}, nil
-}
-
-// StopAndRemove stops the container with the given id.
-func (c *Config) StopAndRemove(ctx context.Context, id string) error {
-	_, err := c.docker(ctx, "kill", id)
-	if err != nil {
-		return fmt.Errorf("docker: kill container: %w", err)
+	// Run the container.
+	args := []string{"docker", "run", "--rm", "--detach"}
+	for _, e := range c.Env {
+		args = append(args, "-e", e)
 	}
-	return nil
-}
-
-func (c *Config) docker(ctx context.Context, args ...string) (string, error) {
+	args = append(args, "-p", fmt.Sprintf("%s:%s", p, c.Port), c.Image)
+	cmd := exec.CommandContext(ctx, args[0], args[1:]...) //nolint:gosec
 	stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{}
-	cmd := exec.CommandContext(ctx, "docker", args...) //nolint:gosec
 	cmd.Stdout, cmd.Stderr = io.MultiWriter(c.Out, stdout), stderr
 	if err := cmd.Run(); err != nil {
 		if stderr.Len() > 0 {
 			err = errors.New(strings.TrimSpace(stderr.String()))
 		}
-		return "", err
+		return nil, err
 	}
-	return strings.TrimSpace(stdout.String()), nil
+	return &Container{
+		Config: *c,
+		ID:     strings.TrimSpace(stdout.String()),
+		Port:   p,
+	}, nil
 }
 
 // Close stops and removes this container.
 func (c *Container) Close() error {
-	return c.Config.StopAndRemove(context.Background(), c.ID)
+	return exec.Command("docker", "kill", c.ID).Run() //nolint:gosec
 }
 
 // Wait waits for this container to be ready.
@@ -442,16 +432,20 @@ func (c *Container) Wait(ctx context.Context, timeout time.Duration) error {
 	if timeout > time.Minute {
 		timeout = time.Minute
 	}
-	u, err := c.PingURL()
+	var (
+		done   = time.After(timeout)
+		u, err = c.URL()
+	)
 	if err != nil {
 		return err
 	}
-	for done := time.After(timeout); ; {
+	pingURL := c.PingURL(*u)
+	for {
 		select {
 		case <-time.After(100 * time.Millisecond):
 			var client *sqlclient.Client
 			// Ping against the root connection.
-			client, err = sqlclient.Open(ctx, u)
+			client, err = sqlclient.Open(ctx, pingURL)
 			if err != nil {
 				continue
 			}
@@ -479,39 +473,22 @@ func (c *Container) Wait(ctx context.Context, timeout time.Duration) error {
 	}
 }
 
-// PingURL returns a URL to ping the Container.
-func (c *Container) PingURL() (string, error) {
-	u, err := c.URL()
-	if err != nil {
-		return "", err
-	}
-	switch c.driver {
-	case DriverSQLServer:
-		q := u.Query()
-		q.Del("database")
-		u.RawQuery = q.Encode()
-		return u.String(), nil
-	default:
-		u.Path = "/"
-		return u.String(), nil
-	}
-}
-
 // URL returns a URL to connect to the Container.
 func (c *Container) URL() (*url.URL, error) {
-	u := &url.URL{
-		Scheme: c.driver,
-		User:   c.User,
-		Host:   c.HostPort,
-	}
+	host := "localhost"
 	// Check if the DOCKER_HOST env var is set.
 	// If it is, use the host from the URL.
 	if h := os.Getenv("DOCKER_HOST"); h != "" {
-		dh, err := url.Parse(h)
+		u, err := url.Parse(h)
 		if err != nil {
 			return nil, err
 		}
-		u.Host = fmt.Sprintf("%s:%s", dh.Hostname(), u.Port())
+		host = u.Hostname()
+	}
+	u := &url.URL{
+		Scheme: c.driver,
+		User:   c.User,
+		Host:   fmt.Sprintf("%s:%s", host, c.Port),
 	}
 	switch c.driver {
 	case DriverSQLServer:
@@ -533,6 +510,43 @@ func (c *Container) URL() (*url.URL, error) {
 	return u, nil
 }
 
+// PingURL returns a URL to ping the Container.
+func (c *Container) PingURL(u url.URL) string {
+	switch c.driver {
+	case DriverSQLServer:
+		q := u.Query()
+		q.Del("database")
+		u.RawQuery = q.Encode()
+		return u.String()
+	default:
+		u.Path = "/"
+		return u.String()
+	}
+}
+
+// validate that no empty values are given.
+func (c *Config) validate() error {
+	if c == nil || c.Image == "" || c.Port == "" || c.Out == nil {
+		return fmt.Errorf("invalid configuration %#v", c)
+	}
+	return nil
+}
+
+func freePort() (string, error) {
+	a, err := net.ResolveTCPAddr("tcp", ":0")
+	if err != nil {
+		return "", err
+	}
+	l, err := net.ListenTCP("tcp", a)
+	if err != nil {
+		return "", err
+	}
+	if err := l.Close(); err != nil {
+		return "", err
+	}
+	return strconv.Itoa(l.Addr().(*net.TCPAddr).Port), nil
+}
+
 func init() {
 	sqlclient.Register(
 		"docker",

cmd/atlas/internal/docker/docker_test.go

@@ -6,7 +6,6 @@ package docker
 
 import (
 	"context"
-	"fmt"
 	"io"
 	"net/url"
 	"testing"
@@ -19,7 +18,7 @@ func TestDockerConfig(t *testing.T) {
 
 	// invalid config
 	_, err := (&Config{}).Run(ctx)
-	require.ErrorContains(t, err, fmt.Sprintf("invalid configuration %#v", &Config{}))
+	require.Error(t, err)
 
 	// MySQL
 	cfg, err := MySQL("latest", Out(io.Discard))
@@ -469,13 +468,11 @@ func TestContainerURL(t *testing.T) {
 			driver: "postgres",
 			User:   url.UserPassword("postgres", "pass"),
 		},
-		HostPort: "0.0.0.0:5432",
+		Port: "5432",
 	}
-	// Without DOCKER_HOST
-	t.Setenv("DOCKER_HOST", "")
 	u, err := c.URL()
 	require.NoError(t, err)
-	require.Equal(t, "postgres://postgres:pass@0.0.0.0:5432/?sslmode=disable", u.String())
+	require.Equal(t, "postgres://postgres:pass@localhost:5432/?sslmode=disable", u.String())
 
 	// With DOCKER_HOST
 	t.Setenv("DOCKER_HOST", "tcp://host.docker.internal:2375")