From da9b559fb5d462f4523a4fa813934534b693ebae Mon Sep 17 00:00:00 2001 From: silverwind Date: Fri, 7 Aug 2026 19:19:43 +0000 Subject: [PATCH] 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 Co-authored-by: silverwind --- act/container/docker_run.go | 68 ++++------------ act/container/docker_run_test.go | 134 ++++++++++++++++++------------- 2 files changed, 93 insertions(+), 109 deletions(-) diff --git a/act/container/docker_run.go b/act/container/docker_run.go index 1db35b67..20125229 100644 --- a/act/container/docker_run.go +++ b/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 diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index d6a60d47..9ff339fa 100644 --- a/act/container/docker_run_test.go +++ b/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 -// when they are a symlink, everything else does not exist. -func stubStatPath(client *mockDockerClient, existing map[string]string) { - for containerPath, target := range existing { - client.On("ContainerStatPath", mock.Anything, "123", mobyclient.ContainerStatPathOptions{Path: containerPath}). - Return(mobyclient.ContainerStatPathResult{Stat: container.PathStat{LinkTarget: target}}, nil).Maybe() +func TestDockerCopyTarStream(t *testing.T) { + ctx := context.Background() + + 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{}, 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 -// to it that never traverse the "/var/run" symlink, see moby/moby#53258. -func TestDockerCopyTarStream(t *testing.T) { +// Docker 29.5+ rejects absolute names in the mkdir tarball with +// "path escapes from parent", since it is extracted relative to "/". +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{} - stubStatPath(client, map[string]string{"/var": "", "/var/run": ""}) - client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool { - return opts.DestinationPath == "/var/run" && opts.Content != nil - })).Return(mobyclient.CopyToContainerResult{}, testCase.mkdirErr) - client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool { - return opts.DestinationPath == "/var/run/act" && opts.Content != nil - })).Return(mobyclient.CopyToContainerResult{}, testCase.copyErr).Maybe() - cr := &containerReference{ - id: "123", - cli: client, - input: &NewContainerInput{ - Image: "image", - }, - } - - require.ErrorIs(t, cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{}), merr) - - client.AssertExpectations(t) - }) + client := &mockDockerClient{} + client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool { + return opts.DestinationPath == "/" && 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) +} + +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 - // exact step that fails on a broken daemon. - err := rc.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{}) + // CopyTarStream first creates the destination directory by extracting a tar at "/", + // which makes the daemon mkdir var, then var/run (the symlink), then act — the exact + // step that fails on the broken daemon. + err := rc.CopyTarStream(ctx, "/var/run/act/actions/", &bytes.Buffer{}) require.NoError(t, err) }