diff --git a/go.mod b/go.mod index 7c244a18..2599c000 100644 --- a/go.mod +++ b/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 diff --git a/pkg/fssync/diffcopy.go b/pkg/fssync/diffcopy.go index 9944b029..ebb8b284 100644 --- a/pkg/fssync/diffcopy.go +++ b/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,9 +41,13 @@ 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), @@ -50,6 +55,25 @@ func (f *FSSyncProxy) DiffCopy(ss filesync.FileSync_DiffCopyServer) error { 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 { diff --git a/pkg/fssync/diffcopy_test.go b/pkg/fssync/diffcopy_test.go index 21e003b6..da741e64 100644 --- a/pkg/fssync/diffcopy_test.go +++ b/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 diff --git a/pkg/fssync/walk.go b/pkg/fssync/walk.go index 52290e63..8de5d44b 100644 --- a/pkg/fssync/walk.go +++ b/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"], ",") diff --git a/pkg/fssync/walk_test.go b/pkg/fssync/walk_test.go index 8d658830..0c6dfd1f 100644 --- a/pkg/fssync/walk_test.go +++ b/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,