chore: revert docker 29.7.0 workaround (#1155)

Revert https://gitea.com/gitea/runner/pulls/1130. Docker 29.7.1 fixed both regressions it worked around, https://github.com/moby/moby/pull/53261 and https://github.com/moby/moby/pull/53260, so only 29.7.0 still needs it.

Verified live with a relative and an absolute `/var/run` symlink: without the workaround the copy passes on 29.4.0, 29.6.2 and 29.7.1, and fails on 29.7.0 alone.

Fixes: https://gitea.com/gitea/runner/issues/1131
Reviewed-on: https://gitea.com/gitea/runner/pulls/1155
Reviewed-by: Lunny Xiao <[email protected]>
Co-authored-by: silverwind <[email protected]>

act/container/docker_run.go

@@ -14,7 +14,6 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"path"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -939,59 +938,24 @@ func (cr *containerReference) waitForCommand(ctx context.Context, resp client.Hi
 	}
 }
 
-// mkdirInContainer creates containerPath and returns it with the symlinked components
-// replaced by the targets the daemon reports for them. Docker 29.7 rejects tar entries
-// traversing a symlink to an absolute target, like the "/var/run" of most images, with
-// "path escapes from parent", and not every daemon creates the implied parents of a
-// directory entry, so one entry per missing component is extracted at the deepest
-// existing ancestor.
-// WORKAROUND: https://github.com/moby/moby/issues/53258
-func (cr *containerReference) mkdirInContainer(ctx context.Context, containerPath string) (string, error) {
-	parts := strings.Split(strings.Trim(path.Clean(containerPath), "/"), "/")
-	existing := "/"
-	for i, part := range parts {
-		if part == "" {
-			return existing, nil
-		}
-		stat, err := cr.cli.ContainerStatPath(ctx, cr.id, client.ContainerStatPathOptions{Path: path.Join(existing, part)})
-		if err != nil {
-			// nothing below exists either, so create the remaining components
-			return path.Join(existing, path.Join(parts[i:]...)), cr.mkdirEntries(ctx, existing, parts[i:])
-		}
-		existing = path.Join(existing, part)
-		if target := stat.Stat.LinkTarget; target != "" {
-			if !path.IsAbs(target) {
-				target = path.Join(path.Dir(existing), target)
-			}
-			existing = target
-		}
-	}
-	return existing, nil
-}
-
-func (cr *containerReference) mkdirEntries(ctx context.Context, destPath string, missing []string) error {
-	buf := &bytes.Buffer{}
-	tw := tar.NewWriter(buf)
-	for i := range missing {
-		_ = tw.WriteHeader(&tar.Header{
-			Name:     path.Join(missing[:i+1]...),
-			Mode:     0o777,
-			Typeflag: tar.TypeDir,
-		})
-	}
-	tw.Close()
-	_, err := cr.cli.CopyToContainer(ctx, cr.id, client.CopyToContainerOptions{
-		DestinationPath: destPath,
-		Content:         buf,
-	})
-	return err
-}
-
 func (cr *containerReference) CopyTarStream(ctx context.Context, destPath string, tarStream io.Reader) error {
 	if cr.id == "" {
 		return cr.missingContainerError("copy to %s", destPath)
 	}
-	destPath, err := cr.mkdirInContainer(ctx, destPath)
+	// Mkdir, with a path relative to the DestinationPath ("/") below. Docker 29.5+
+	// rejects absolute tar entry names with "path escapes from parent".
+	buf := &bytes.Buffer{}
+	tw := tar.NewWriter(buf)
+	_ = tw.WriteHeader(&tar.Header{
+		Name:     strings.TrimPrefix(destPath, "/"),
+		Mode:     0o777,
+		Typeflag: tar.TypeDir,
+	})
+	tw.Close()
+	_, err := cr.cli.CopyToContainer(ctx, cr.id, client.CopyToContainerOptions{
+		DestinationPath: "/",
+		Content:         buf,
+	})
 	if err != nil {
 		return fmt.Errorf("failed to mkdir to copy content to container: %w", err)
 	}
@@ -1016,10 +980,6 @@ func (cr *containerReference) copyDir(dstPath, srcPath string, useGitIgnore bool
 			return cr.missingContainerError("copy directory to %s", dstPath)
 		}
 		logger := common.Logger(ctx)
-		dstPath, err := cr.mkdirInContainer(ctx, dstPath)
-		if err != nil {
-			return fmt.Errorf("failed to mkdir to copy directory to container: %w", err)
-		}
 		tarFile, err := os.CreateTemp("", "act")
 		if err != nil {
 			return err

act/container/docker_run_test.go

@@ -94,11 +94,6 @@ func (m *mockDockerClient) ExecInspect(ctx context.Context, execID string, opts
 	return args.Get(0).(mobyclient.ExecInspectResult), args.Error(1)
 }
 
-func (m *mockDockerClient) ContainerStatPath(ctx context.Context, containerID string, opts mobyclient.ContainerStatPathOptions) (mobyclient.ContainerStatPathResult, error) {
-	args := m.Called(ctx, containerID, opts)
-	return args.Get(0).(mobyclient.ContainerStatPathResult), args.Error(1)
-}
-
 func (m *mockDockerClient) ContainerAttach(ctx context.Context, containerID string, opts mobyclient.ContainerAttachOptions) (mobyclient.ContainerAttachResult, error) {
 	args := m.Called(ctx, containerID, opts)
 	return args.Get(0).(mobyclient.ContainerAttachResult), args.Error(1)
@@ -342,37 +337,52 @@ func TestDockerWaitFailure(t *testing.T) {
 	client.AssertExpectations(t)
 }
 
-// stubStatPath answers path resolution: the given paths exist, mapped to their target
+func TestDockerCopyTarStream(t *testing.T) {
-// when they are a symlink, everything else does not exist.
+	ctx := context.Background()
-func stubStatPath(client *mockDockerClient, existing map[string]string) {
+
-	for containerPath, target := range existing {
+	client := &mockDockerClient{}
-		client.On("ContainerStatPath", mock.Anything, "123", mobyclient.ContainerStatPathOptions{Path: containerPath}).
+	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-			Return(mobyclient.ContainerStatPathResult{Stat: container.PathStat{LinkTarget: target}}, nil).Maybe()
+		return opts.DestinationPath == "/" && opts.Content != nil
+	})).Return(mobyclient.CopyToContainerResult{}, nil)
+	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
+		return opts.DestinationPath == "/var/run/act" && opts.Content != nil
+	})).Return(mobyclient.CopyToContainerResult{}, nil)
+	cr := &containerReference{
+		id:  "123",
+		cli: client,
+		input: &NewContainerInput{
+			Image: "image",
+		},
 	}
-	client.On("ContainerStatPath", mock.Anything, "123", mock.Anything).
+
-		Return(mobyclient.ContainerStatPathResult{}, cerrdefs.ErrNotFound).Maybe()
+	_ = cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{})
+
+	client.AssertExpectations(t)
 }
 
-// The mkdir tarball is extracted at the deepest existing ancestor, with entries relative
+// Docker 29.5+ rejects absolute names in the mkdir tarball with
-// to it that never traverse the "/var/run" symlink, see moby/moby#53258.
+// "path escapes from parent", since it is extracted relative to "/".
-func TestDockerCopyTarStream(t *testing.T) {
+func TestDockerCopyTarStreamMkdirEntryIsRelative(t *testing.T) {
 	ctx := context.Background()
 
 	var mkdirNames []string
 	client := &mockDockerClient{}
-	stubStatPath(client, map[string]string{"/var": "", "/var/run": "/run", "/run": ""})
 	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-		if opts.DestinationPath != "/run" || opts.Content == nil {
+		if opts.DestinationPath != "/" || opts.Content == nil {
 			return false
 		}
 		tr := tar.NewReader(opts.Content)
-		for hdr, err := tr.Next(); err == nil; hdr, err = tr.Next() {
+		for {
+			hdr, err := tr.Next()
+			if err != nil {
+				break
+			}
 			mkdirNames = append(mkdirNames, hdr.Name)
 		}
 		return true
 	})).Return(mobyclient.CopyToContainerResult{}, nil)
 	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-		return opts.DestinationPath == "/run/act" && opts.Content != nil
+		return opts.DestinationPath == "/var/run/act" && opts.Content != nil
 	})).Return(mobyclient.CopyToContainerResult{}, nil)
 	cr := &containerReference{
 		id:  "123",
@@ -383,45 +393,58 @@ func TestDockerCopyTarStream(t *testing.T) {
 	}
 
 	require.NoError(t, cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{}))
-	assert.Equal(t, []string{"act"}, mkdirNames)
+	assert.Equal(t, []string{"var/run/act"}, mkdirNames)
 
 	client.AssertExpectations(t)
 }
 
-func TestDockerCopyTarStreamErrors(t *testing.T) {
+func TestDockerCopyTarStreamErrorInCopyFiles(t *testing.T) {
+	ctx := context.Background()
+
 	merr := errors.New("Failure")
-	for _, testCase := range []struct {
-		name     string
-		mkdirErr error
-		copyErr  error
-	}{
-		{"mkdir", merr, nil},
-		{"copy content", nil, merr},
-	} {
-		t.Run(testCase.name, func(t *testing.T) {
-			ctx := context.Background()
 
-			client := &mockDockerClient{}
+	client := &mockDockerClient{}
-			stubStatPath(client, map[string]string{"/var": "", "/var/run": ""})
+	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-			client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
+		return opts.DestinationPath == "/" && opts.Content != nil
-				return opts.DestinationPath == "/var/run" && opts.Content != nil
+	})).Return(mobyclient.CopyToContainerResult{}, merr)
-			})).Return(mobyclient.CopyToContainerResult{}, testCase.mkdirErr)
+	cr := &containerReference{
-			client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
+		id:  "123",
-				return opts.DestinationPath == "/var/run/act" && opts.Content != nil
+		cli: client,
-			})).Return(mobyclient.CopyToContainerResult{}, testCase.copyErr).Maybe()
+		input: &NewContainerInput{
-			cr := &containerReference{
+			Image: "image",
-				id:  "123",
+		},
-				cli: client,
-				input: &NewContainerInput{
-					Image: "image",
-				},
-			}
-
-			require.ErrorIs(t, cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{}), merr)
-
-			client.AssertExpectations(t)
-		})
 	}
+
+	err := cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{})
+	assert.ErrorIs(t, err, merr) //nolint:testifylint // pre-existing issue from nektos/act
+
+	client.AssertExpectations(t)
+}
+
+func TestDockerCopyTarStreamErrorInMkdir(t *testing.T) {
+	ctx := context.Background()
+
+	merr := errors.New("Failure")
+
+	client := &mockDockerClient{}
+	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
+		return opts.DestinationPath == "/" && opts.Content != nil
+	})).Return(mobyclient.CopyToContainerResult{}, nil)
+	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
+		return opts.DestinationPath == "/var/run/act" && opts.Content != nil
+	})).Return(mobyclient.CopyToContainerResult{}, merr)
+	cr := &containerReference{
+		id:  "123",
+		cli: client,
+		input: &NewContainerInput{
+			Image: "image",
+		},
+	}
+
+	err := cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{})
+	assert.ErrorIs(t, err, merr) //nolint:testifylint // pre-existing issue from nektos/act
+
+	client.AssertExpectations(t)
 }
 
 // A remove that raced the daemon's AutoRemove teardown is not a failure and must not
@@ -612,9 +635,10 @@ func TestDockerCopyToSymlinkPath(t *testing.T) {
 		_ = rc.Close()(ctx)
 	})
 
-	// CopyTarStream resolves the var/run symlink and creates act below its target, the
+	// CopyTarStream first creates the destination directory by extracting a tar at "/",
-	// exact step that fails on a broken daemon.
+	// which makes the daemon mkdir var, then var/run (the symlink), then act — the exact
-	err := rc.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{})
+	// step that fails on the broken daemon.
+	err := rc.CopyTarStream(ctx, "/var/run/act/actions/", &bytes.Buffer{})
 	require.NoError(t, err)
 }