Emit excluded parent dirs of files re-included by dockerignore negations

go.mod

@@ -11,7 +11,6 @@ require (
 	github.com/google/uuid v1.6.0
 	github.com/mdlayher/vsock v1.2.1
 	github.com/moby/buildkit v0.29.0
-	github.com/moby/patternmatcher v0.6.1
 	github.com/opencontainers/go-digest v1.0.0
 	github.com/opencontainers/image-spec v1.1.1
 	github.com/pelletier/go-toml/v2 v2.4.1
@@ -58,6 +57,7 @@ require (
 	github.com/mdlayher/socket v0.6.0 // indirect
 	github.com/moby/docker-image-spec v1.3.1 // indirect
 	github.com/moby/locker v1.0.1 // indirect
+	github.com/moby/patternmatcher v0.6.1 // indirect
 	github.com/moby/sys/signal v0.7.1 // indirect
 	github.com/morikuni/aec v1.1.0 // indirect
 	github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10 // indirect

pkg/fssync/diffcopy.go

@@ -28,6 +28,7 @@ import (
 
 	"github.com/moby/buildkit/session/filesync"
 	"github.com/pkg/errors"
+	"github.com/tonistiigi/fsutil"
 	"github.com/tonistiigi/fsutil/types"
 	"golang.org/x/sync/errgroup"
 )
@@ -40,16 +41,39 @@ import (
 // to reduce syscall overhead for large files.
 func (f *FSSyncProxy) DiffCopy(ss filesync.FileSync_DiffCopyServer) error {
 	ctx := ss.Context()
-	fs := NewFS(ctx, f, f.contextDir, f.basePath)
+	fs, err := filteredFS(ctx, NewFS(ctx, f, f.contextDir, f.basePath))
+	if err != nil {
+		return err
+	}
 	s := &sender{
 		conn:         &syncStream{Stream: ss},
+		proxy:        f,
 		fs:           fs,
 		files:        make(map[uint32]string),
 		sendpipeline: make(chan *sendHandle, 128),
 	}
 	return s.run(ctx)
 }
 
+// filteredFS wraps the build-context FS with the exclude patterns (from
+// .dockerignore) that BuildKit sends in the request metadata, mirroring how
+// BuildKit's own filesync provider filters a local context.
+//
+// fsutil's filter implements the full pattern semantics; in particular, when
+// a negation pattern re-includes a file inside an excluded directory, the
+// excluded ancestor directories are emitted before the file. BuildKit's
+// receiver rejects the stream with "changes out of order" if a file arrives
+// without its parent directories.
+func filteredFS(ctx context.Context, inner fsutil.FS) (fsutil.FS, error) {
+	walkMeta, err := unmarshalWalkMetadata(ctx)
+	if err != nil {
+		return nil, err
+	}
+	return fsutil.NewFilterFS(inner, &fsutil.FilterOpt{
+		ExcludePatterns: walkMeta.ExcludedPatterns,
+	})
+}
+
 var bufPool = sync.Pool{
 	New: func() any {
 		buf := make([]byte, 1<<20)
@@ -70,7 +94,8 @@ type sendHandle struct {
 
 type sender struct {
 	conn            Stream
-	fs              *FS
+	proxy           *FSSyncProxy
+	fs              fsutil.FS
 	files           map[uint32]string
 	mu              sync.RWMutex
 	progressCurrent int
@@ -153,9 +178,9 @@ func (s *sender) sendFile(h *sendHandle) error {
 
 	switch h.path {
 	case filepath.Join(DockerfileStaging, "Dockerfile"):
-		r = bytes.NewReader(s.fs.proxy.dockerfile)
+		r = bytes.NewReader(s.proxy.dockerfile)
 	case filepath.Join(DockerfileStaging, "Dockerfile.dockerignore"):
-		r = bytes.NewReader(s.fs.proxy.dockerignore)
+		r = bytes.NewReader(s.proxy.dockerignore)
 	}
 
 	if r == nil {

pkg/fssync/diffcopy_test.go

@@ -17,13 +17,21 @@
 package fssync
 
 import (
+	"archive/tar"
+	"bytes"
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"errors"
 	"os"
+	"slices"
 	"sync"
 	"testing"
+	"time"
 
+	"github.com/tonistiigi/fsutil"
 	"github.com/tonistiigi/fsutil/types"
+	"google.golang.org/grpc/metadata"
 )
 
 type mockConn struct {
@@ -142,6 +150,97 @@ func TestSenderQueueInvalidIDReturnsError(t *testing.T) {
 	}
 }
 
+// makeDockerignoreReproTar builds the build-context tree from
+// https://github.com/apple/container/issues/1800:
+//
+//	foo/.gitkeep
+//	foo/bar/.gitkeep
+func makeDockerignoreReproTar() (checksum string, full []byte) {
+	var buf bytes.Buffer
+	tw := tar.NewWriter(&buf)
+
+	for _, dir := range []string{"foo", "foo/bar"} {
+		_ = tw.WriteHeader(&tar.Header{
+			Name:     dir,
+			Typeflag: tar.TypeDir,
+			Mode:     0o755,
+			ModTime:  time.Time{},
+		})
+	}
+	for _, file := range []string{"foo/.gitkeep", "foo/bar/.gitkeep"} {
+		_ = tw.WriteHeader(&tar.Header{
+			Name:     file,
+			Typeflag: tar.TypeReg,
+			Mode:     0o644,
+			Size:     0,
+			ModTime:  time.Time{},
+		})
+	}
+	_ = tw.Close()
+
+	full = buf.Bytes()
+	sum := sha256.Sum256(full)
+	return hex.EncodeToString(sum[:]), full
+}
+
+// Regression test for https://github.com/apple/container/issues/1800.
+//
+// A .dockerignore that excludes a directory's contents but re-includes some
+// of its descendants with negation patterns (the default Rails template does
+// this) must still emit the excluded ancestor directories before the
+// re-included files. BuildKit's receiver validates parent ordering and
+// rejects the stream with "changes out of order" otherwise.
+func TestDiffCopyEmitsExcludedParentDirsOfReincludedFiles(t *testing.T) {
+	prevTarFactory := testTarFactory
+	testTarFactory = makeDockerignoreReproTar
+	defer func() { testTarFactory = prevTarFactory }()
+
+	// The patterns from the issue's .dockerignore as BuildKit sends them
+	// over the DiffCopy request metadata (the dockerignore parser strips
+	// leading slashes):
+	//
+	//	/foo/*
+	//	!/foo/.gitkeep
+	//	/foo/bar/*
+	//	!/foo/bar/.gitkeep
+	ctx := metadata.NewIncomingContext(context.Background(), metadata.MD{
+		"exclude-patterns": []string{"foo/*", "!foo/.gitkeep", "foo/bar/*", "!foo/bar/.gitkeep"},
+	})
+
+	fs, err := filteredFS(ctx, NewFS(ctx, &FSSyncProxy{}, "/", t.TempDir()))
+	if err != nil {
+		t.Fatalf("filteredFS returned err=%v", err)
+	}
+
+	ms := newMockStream()
+	s := &sender{
+		conn:         &syncStream{Stream: ms},
+		fs:           fs,
+		files:        make(map[uint32]string),
+		sendpipeline: make(chan *sendHandle, 128),
+	}
+	if err := s.walk(ctx); err != nil {
+		t.Fatalf("walk returned err=%v", err)
+	}
+
+	var paths []string
+	validator := &fsutil.Validator{}
+	for _, p := range ms.sent {
+		if p.Type != types.PACKET_STAT || p.Stat == nil {
+			continue
+		}
+		paths = append(paths, p.Stat.Path)
+		if err := validator.HandleChange(fsutil.ChangeKindAdd, p.Stat.Path, &fsutil.StatInfo{Stat: p.Stat}, nil); err != nil {
+			t.Fatalf("BuildKit's receiver would reject the stream: %v (paths so far: %v)", err, paths)
+		}
+	}
+
+	want := []string{"foo", "foo/.gitkeep", "foo/bar", "foo/bar/.gitkeep"}
+	if !slices.Equal(paths, want) {
+		t.Fatalf("sent paths = %v, want %v", paths, want)
+	}
+}
+
 func TestFileCanRequestData(t *testing.T) {
 	tests := []struct {
 		mode os.FileMode

pkg/fssync/walk.go

@@ -23,7 +23,6 @@ import (
 	"strings"
 
 	"github.com/google/uuid"
-	"github.com/moby/patternmatcher"
 
 	"github.com/apple/container-builder-shim/pkg/api"
 	"github.com/apple/container-builder-shim/pkg/fileutils"
@@ -37,8 +36,8 @@ Walk requests build-context files from the macOS host and presents them to Build
 The host is asked for a tar archive containing the paths identified by
 followpaths (glob patterns BuildKit sends in the request metadata). The shim
 unpacks the tar to a content-addressed local cache and then walks the unpacked
-tree, filtering each entry through the exclude-patterns (from .dockerignore)
-before passing it to fn.
+tree, passing every entry to fn. Exclude-pattern filtering (from .dockerignore)
+is applied by the fsutil filter that DiffCopy wraps around this FS.
 
 Only TAR mode is supported. The JSON mode wire format is defined in
 RawFileInfo below but is not exercised by the current shim.
@@ -94,10 +93,6 @@ func (f *FS) Walk(ctx context.Context, target string, fn fs.WalkDirFunc) error {
 	if err != nil {
 		return err
 	}
-	excludeMatcher, err := patternmatcher.New(strings.Split(walkMeta.ExcludedPatterns, ","))
-	if err != nil {
-		return err
-	}
 
 	id := uuid.NewString()
 	demux := stream.NewDemuxWithContext(cancellableCtx, id, stream.FilterByBuildID(id), func(any) {})
@@ -134,15 +129,7 @@ func (f *FS) Walk(ctx context.Context, target string, fn fs.WalkDirFunc) error {
 	switch walkMeta.Mode {
 	case ModeTAR:
 		receiver := fileutils.NewTarReceiver(f.fsPath, demux)
-		checksum, err := receiver.Receive(ctx, f.proxy.dockerfile, f.proxy.dockerignore,
-			func(path string, d fs.DirEntry, err error) error {
-				excluded, err := excludeMatcher.MatchesOrParentMatches(path)
-				if excluded {
-					return nil
-				}
-
-				return fn(path, d, err)
-			})
+		checksum, err := receiver.Receive(ctx, f.proxy.dockerfile, f.proxy.dockerignore, fn)
 		if err != nil {
 			return err
 		}
@@ -169,7 +156,7 @@ type RawFileInfo struct {
 
 type WalkMetadata struct {
 	IncludePatterns  string
-	ExcludedPatterns string
+	ExcludedPatterns []string
 	FollowPaths      string
 	DirName          string
 	Mode             TransferMode
@@ -179,7 +166,7 @@ func unmarshalWalkMetadata(ctx context.Context) (*WalkMetadata, error) {
 	md := &WalkMetadata{}
 	if m, ok := metadata.FromIncomingContext(ctx); ok {
 		md.IncludePatterns = strings.Join(m["include-patterns"], ",")
-		md.ExcludedPatterns = strings.Join(m["exclude-patterns"], ",")
+		md.ExcludedPatterns = m["exclude-patterns"]
 		md.FollowPaths = strings.Join(m["followpaths"], ",")
 		md.DirName = strings.Join(m["dir-name"], ",")
 		modeStr := strings.Join(m["mode"], ",")

pkg/fssync/walk_test.go

@@ -71,10 +71,15 @@ func makeNestedTarHeaderAndBody() (checksum string, full []byte) {
 	return hex.EncodeToString(header[:]), full
 }
 
+// testTarFactory produces the tar content served by the fake Send below.
+// Tests that need a different build-context tree may override it and restore
+// the previous value when done.
+var testTarFactory = makeNestedTarHeaderAndBody
+
 func (p *FSSyncProxy) Send(s *api.ServerStream) error {
 	id := s.BuildId
 	d := demuxes[id]
-	checksum, full := makeNestedTarHeaderAndBody()
+	checksum, full := testTarFactory()
 	go func() {
 		_ = d.Accept(&api.ClientStream{
 			BuildId: id,