From fd498cbf5db65ce29af5e5b92824b26ee897d7f2 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Fri, 8 Aug 2025 10:16:17 -0400 Subject: [PATCH 1/6] imagebuildah.Executor/StageExecutor: check numeric --from= values When we look up a stage that's referred to in a COPY --from argument, treat the string as a stage number not only if it parses as one, as we checked before, but now also require that the number correspond to one of the stages that would be completed before the one into which the content will be copied. Signed-off-by: Nalin Dahyabhai --- imagebuildah/executor.go | 2 +- imagebuildah/stage_executor.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/imagebuildah/executor.go b/imagebuildah/executor.go index 63089ff7b..f479eea73 100644 --- a/imagebuildah/executor.go +++ b/imagebuildah/executor.go @@ -862,7 +862,7 @@ func (b *Executor) Build(ctx context.Context, stages imagebuilder.Stages) (image logrus.Debugf("stage %d name: %q resolves to %q", stageIndex, stageName, baseWithArg) stageName = baseWithArg // If --from= convert index to name - if index, err := strconv.Atoi(stageName); err == nil { + if index, err := strconv.Atoi(stageName); err == nil && index >= 0 && index < stageIndex { stageName = stages[index].Name } // Check if selected base is not an additional diff --git a/imagebuildah/stage_executor.go b/imagebuildah/stage_executor.go index 51918ef45..9a96448f6 100644 --- a/imagebuildah/stage_executor.go +++ b/imagebuildah/stage_executor.go @@ -467,7 +467,7 @@ func (s *StageExecutor) performCopy(excludes []string, copies ...imagebuilder.Co // exists and if stage short_name matches any // additionalContext replace stage with additional // build context. - if index, err := strconv.Atoi(from); err == nil { + if index, err := strconv.Atoi(from); err == nil && index >= 0 && index < s.index { from = s.stages[index].Name } if foundContext, ok := s.executor.additionalBuildContexts[from]; ok { @@ -1395,7 +1395,7 @@ func (s *StageExecutor) Execute(ctx context.Context, base string) (imgID string, // also account if the index is given instead // of name so convert index in --from= // to name. - if index, err := strconv.Atoi(from); err == nil { + if index, err := strconv.Atoi(from); err == nil && index >= 0 && index < s.index { from = s.stages[index].Name } // If additional buildContext contains this From 9461dd61d4eac2774ed8c23b005d363a967fbac7 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Fri, 8 Aug 2025 14:57:03 -0400 Subject: [PATCH 2/6] copier.Get(): strip user and group names from entries When generating archives, clear user and group names to keep up with recent changes to the storage library. Signed-off-by: Nalin Dahyabhai --- copier/copier.go | 1 + copier/copier_test.go | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/copier/copier.go b/copier/copier.go index 697c5f46a..8122812ad 100644 --- a/copier/copier.go +++ b/copier/copier.go @@ -1591,6 +1591,7 @@ func copierHandlerGetOne(srcfi os.FileInfo, symlinkTarget, name, contentPath str if name != "" { hdr.Name = filepath.ToSlash(name) } + hdr.Uname, hdr.Gname = "", "" if options.Rename != nil { hdr.Name = handleRename(options.Rename, hdr.Name) } diff --git a/copier/copier_test.go b/copier/copier_test.go index 8bf905784..263f8c33a 100644 --- a/copier/copier_test.go +++ b/copier/copier_test.go @@ -1555,11 +1555,16 @@ func testGetMultiple(t *testing.T) { tr := tar.NewReader(pipeReader) hdr, err := tr.Next() actualContents := []string{} - for err == nil { + for hdr != nil { actualContents = append(actualContents, filepath.FromSlash(hdr.Name)) + assert.Equal(t, "", hdr.Uname, "expected user name field to be cleared") + assert.Equal(t, "", hdr.Gname, "expected group name field to be cleared") if testCase.timestamp != nil { assert.Truef(t, testCase.timestamp.Equal(hdr.ModTime), "timestamp was supposed to be forced for %q", hdr.Name) } + if err != nil { + break + } hdr, err = tr.Next() } pipeReader.Close() From 738fa0d3c456037cd3ef41cc8b8302b435353d85 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Fri, 8 Aug 2025 14:59:17 -0400 Subject: [PATCH 3/6] copier.Get(): ensure that directory entries end in "/" Make sure that entries with Typeflag == TypeDir always end with a "/", adding it as a suffix. Signed-off-by: Nalin Dahyabhai --- copier/copier.go | 3 ++ copier/copier_linux_test.go | 2 +- copier/copier_test.go | 104 ++++++++++++++++++------------------ 3 files changed, 56 insertions(+), 53 deletions(-) diff --git a/copier/copier.go b/copier/copier.go index 8122812ad..4e8b5f2b7 100644 --- a/copier/copier.go +++ b/copier/copier.go @@ -1699,6 +1699,9 @@ func copierHandlerGetOne(srcfi os.FileInfo, symlinkTarget, name, contentPath str if options.ChmodDirs != nil { hdr.Mode = int64(*options.ChmodDirs) } + if !strings.HasSuffix(hdr.Name, "/") { + hdr.Name += "/" + } } else { if options.ChownFiles != nil { hdr.Uid, hdr.Gid = options.ChownFiles.UID, options.ChownFiles.GID diff --git a/copier/copier_linux_test.go b/copier/copier_linux_test.go index 2ebbfb379..6b0498e7e 100644 --- a/copier/copier_linux_test.go +++ b/copier/copier_linux_test.go @@ -184,7 +184,7 @@ func TestGetNoCrossDevice(t *testing.T) { tr := tar.NewReader(&buf) th, err := tr.Next() // should be the "subdir" directory require.NoError(t, err, "error reading first entry archived") - assert.Equal(t, "subdir", th.Name, `first entry in archive was not named "subdir"`) + assert.Equal(t, "subdir/", th.Name, `first entry in archive was not named "subdir/"`) th, err = tr.Next() assert.Error(t, err, "should not have gotten a second entry in archive") diff --git a/copier/copier_test.go b/copier/copier_test.go index 263f8c33a..b8589a6f9 100644 --- a/copier/copier_test.go +++ b/copier/copier_test.go @@ -912,22 +912,22 @@ func testGetMultiple(t *testing.T) { {Name: "non-archive-a", Typeflag: tar.TypeReg, Size: 1199, Mode: 0o600}, {Name: "hlink-0", Typeflag: tar.TypeLink, Linkname: "file-0", Size: 123456789, Mode: 0o600}, {Name: "something-a", Typeflag: tar.TypeReg, Size: 34, Mode: 0o600}, - {Name: "subdir-a", Typeflag: tar.TypeDir, Mode: 0o700}, + {Name: "subdir-a/", Typeflag: tar.TypeDir, Mode: 0o700}, {Name: "subdir-a/file-n", Typeflag: tar.TypeReg, Size: 108, Mode: 0o660}, {Name: "subdir-a/file-o", Typeflag: tar.TypeReg, Size: 45, Mode: 0o660}, {Name: "subdir-a/file-a", Typeflag: tar.TypeSymlink, Linkname: "../file-a", Size: 23, Mode: 0o600}, {Name: "subdir-a/file-b", Typeflag: tar.TypeSymlink, Linkname: "../../file-b", Size: 23, Mode: 0o600}, {Name: "subdir-a/file-c", Typeflag: tar.TypeReg, Size: 56, Mode: 0o600}, - {Name: "subdir-b", Typeflag: tar.TypeDir, Mode: 0o700}, + {Name: "subdir-b/", Typeflag: tar.TypeDir, Mode: 0o700}, {Name: "subdir-b/file-n", Typeflag: tar.TypeReg, Size: 216, Mode: 0o660}, {Name: "subdir-b/file-o", Typeflag: tar.TypeReg, Size: 67, Mode: 0o660}, - {Name: "subdir-c", Typeflag: tar.TypeDir, Mode: 0o700}, + {Name: "subdir-c/", Typeflag: tar.TypeDir, Mode: 0o700}, {Name: "subdir-c/file-p", Typeflag: tar.TypeReg, Size: 432, Mode: 0o666}, {Name: "subdir-c/file-q", Typeflag: tar.TypeReg, Size: 78, Mode: 0o666}, - {Name: "subdir-d", Typeflag: tar.TypeDir, Mode: 0o700}, + {Name: "subdir-d/", Typeflag: tar.TypeDir, Mode: 0o700}, {Name: "subdir-d/hlink-0", Typeflag: tar.TypeLink, Linkname: "../file-0", Size: 123456789, Mode: 0o600}, - {Name: "subdir-e", Typeflag: tar.TypeDir, Mode: 0o700}, - {Name: "subdir-e/subdir-f", Typeflag: tar.TypeDir, Mode: 0o700}, + {Name: "subdir-e/", Typeflag: tar.TypeDir, Mode: 0o700}, + {Name: "subdir-e/subdir-f/", Typeflag: tar.TypeDir, Mode: 0o700}, {Name: "subdir-e/subdir-f/hlink-b", Typeflag: tar.TypeLink, Linkname: "../../file-b", Size: 23, Mode: 0o600}, }, contents: map[string][]byte{ @@ -952,22 +952,22 @@ func testGetMultiple(t *testing.T) { "something-a", "archive-a", "non-archive-a", - "subdir-a", + "subdir-a/", "subdir-a/file-n", "subdir-a/file-o", "subdir-a/file-a", "subdir-a/file-b", "subdir-a/file-c", - "subdir-b", + "subdir-b/", "subdir-b/file-n", "subdir-b/file-o", - "subdir-c", + "subdir-c/", "subdir-c/file-p", "subdir-c/file-q", - "subdir-d", + "subdir-d/", "subdir-d/hlink-0", - "subdir-e", - "subdir-e/subdir-f", + "subdir-e/", + "subdir-e/subdir-f/", "subdir-e/subdir-f/hlink-b", }, }, @@ -995,7 +995,7 @@ func testGetMultiple(t *testing.T) { "file-q", // from subdir-c "file-q", // from link-c -> subdir-c "hlink-0", // from subdir-d - "subdir-f", // from subdir-e + "subdir-f/", // from subdir-e "subdir-f/hlink-b", // from subdir-e }, }, @@ -1019,16 +1019,16 @@ func testGetMultiple(t *testing.T) { "link-c", "hlink-0", // "subdir-a/file-c", // strings.HasPrefix("**/*-c", "subdir-a/") is false - "subdir-b", + "subdir-b/", "subdir-b/file-n", "subdir-b/file-o", - "subdir-c", + "subdir-c/", "subdir-c/file-p", "subdir-c/file-q", - "subdir-d", + "subdir-d/", "subdir-d/hlink-0", - "subdir-e", - "subdir-e/subdir-f", + "subdir-e/", + "subdir-e/subdir-f/", "subdir-e/subdir-f/hlink-b", }, }, @@ -1048,7 +1048,7 @@ func testGetMultiple(t *testing.T) { "file-q", // from link-c -> subdir-c "hlink-0", "hlink-0", - "subdir-f", + "subdir-f/", "subdir-f/hlink-b", }, }, @@ -1066,22 +1066,22 @@ func testGetMultiple(t *testing.T) { "something-a", "archive-a", "non-archive-a", - "subdir-a", + "subdir-a/", "subdir-a/file-a", "subdir-a/file-b", "subdir-a/file-c", "subdir-a/file-n", "subdir-a/file-o", - "subdir-b", + "subdir-b/", "subdir-b/file-n", "subdir-b/file-o", - "subdir-c", + "subdir-c/", "subdir-c/file-p", "subdir-c/file-q", - "subdir-d", + "subdir-d/", "subdir-d/hlink-0", - "subdir-e", - "subdir-e/subdir-f", + "subdir-e/", + "subdir-e/subdir-f/", "subdir-e/subdir-f/hlink-b", }, }, @@ -1110,7 +1110,7 @@ func testGetMultiple(t *testing.T) { "something-a", "archive-a", "non-archive-a", - "subdir-f", + "subdir-f/", "subdir-f/hlink-b", }, }, @@ -1133,7 +1133,7 @@ func testGetMultiple(t *testing.T) { items: []string{ // "subdir-a/file-c", // strings.HasPrefix("**/*-c", "subdir-a/") is false "link-c", - "subdir-c", + "subdir-c/", "subdir-c/file-p", "subdir-c/file-q", }, @@ -1241,7 +1241,7 @@ func testGetMultiple(t *testing.T) { name: "subdirectory", pattern: "subdir-e", items: []string{ - "subdir-f", + "subdir-f/", "subdir-f/hlink-b", }, }, @@ -1275,7 +1275,7 @@ func testGetMultiple(t *testing.T) { name: "subdir-without-name", pattern: "subdir-e", items: []string{ - "subdir-f", + "subdir-f/", "subdir-f/hlink-b", }, }, @@ -1284,8 +1284,8 @@ func testGetMultiple(t *testing.T) { pattern: "subdir-e", keepDirectoryNames: true, items: []string{ - "subdir-e", - "subdir-e/subdir-f", + "subdir-e/", + "subdir-e/subdir-f/", "subdir-e/subdir-f/hlink-b", }, }, @@ -1336,7 +1336,7 @@ func testGetMultiple(t *testing.T) { "archive-a", "non-archive-a", "something-a", - "subdir-b", + "subdir-b/", "subdir-b/file-n", "subdir-b/file-o", "subdir-b/file-a", @@ -1391,11 +1391,11 @@ func testGetMultiple(t *testing.T) { "something-a", "archive-a", "non-archive-a", - "subdir-a", - "subdir-b", - "subdir-c", - "subdir-d", - "subdir-e", + "subdir-a/", + "subdir-b/", + "subdir-c/", + "subdir-d/", + "subdir-e/", "subdir-a/file-n", "subdir-a/file-o", "subdir-a/file-a", @@ -1408,7 +1408,7 @@ func testGetMultiple(t *testing.T) { "subdir-c/file-q", "subdir-c/file-q", "subdir-d/hlink-0", - "subdir-e/subdir-f", + "subdir-e/subdir-f/", "subdir-e/subdir-f/hlink-b", }, }, @@ -1420,11 +1420,11 @@ func testGetMultiple(t *testing.T) { items: []string{ "file-0", "file-b", - "subdir-a", - "subdir-b", - "subdir-c", - "subdir-d", - "subdir-e", + "subdir-a/", + "subdir-b/", + "subdir-c/", + "subdir-d/", + "subdir-e/", "subdir-a/file-c", "subdir-b/file-n", "subdir-b/file-o", @@ -1434,7 +1434,7 @@ func testGetMultiple(t *testing.T) { "subdir-c/file-q", "hlink-0", "subdir-d/hlink-0", - "subdir-e/subdir-f", + "subdir-e/subdir-f/", "subdir-e/subdir-f/hlink-b", }, }, @@ -1448,7 +1448,7 @@ func testGetMultiple(t *testing.T) { "something-a", "archive-a", "non-archive-a", - "subdir-a", + "subdir-a/", "subdir-a/file-n", "subdir-a/file-o", "subdir-a/file-a", @@ -1461,7 +1461,7 @@ func testGetMultiple(t *testing.T) { pattern: "/subdir-b/*", parents: true, items: []string{ - "subdir-b", + "subdir-b/", "subdir-b/file-n", "subdir-b/file-o", }, @@ -1471,18 +1471,18 @@ func testGetMultiple(t *testing.T) { pattern: "../../subdir-b/*", parents: true, items: []string{ - "subdir-b", + "subdir-b/", "subdir-b/file-n", "subdir-b/file-o", }, }, { name: "dir-with-parents", - pattern: "subdir-e/subdir-f", + pattern: "subdir-e/subdir-f/", parents: true, items: []string{ - "subdir-e", - "subdir-e/subdir-f", + "subdir-e/", + "subdir-e/subdir-f/", "subdir-e/subdir-f/hlink-b", }, }, @@ -1491,8 +1491,8 @@ func testGetMultiple(t *testing.T) { pattern: "subdir-e/subdir-f/hlink-b", parents: true, items: []string{ - "subdir-e", - "subdir-e/subdir-f", + "subdir-e/", + "subdir-e/subdir-f/", "subdir-e/subdir-f/hlink-b", }, }, From 473656b9dd340a6f4179c6d9e40166a74edcba75 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Fri, 8 Aug 2025 14:55:32 -0400 Subject: [PATCH 4/6] copier.Stat(): return owner UID and GID if available Return owner information for items that we've stat'ed. Signed-off-by: Nalin Dahyabhai --- copier/copier.go | 31 ++++++++++++++++++++++--------- copier/copier_test.go | 1 + copier/copier_unix_test.go | 8 ++++++++ copier/copier_windows_test.go | 13 ++++++++++--- 4 files changed, 41 insertions(+), 12 deletions(-) diff --git a/copier/copier.go b/copier/copier.go index 4e8b5f2b7..bd082396a 100644 --- a/copier/copier.go +++ b/copier/copier.go @@ -201,7 +201,7 @@ func (req *request) UIDMap() []idtools.IDMap { case requestEval: return nil case requestStat: - return nil + return req.StatOptions.UIDMap case requestGet: return req.GetOptions.UIDMap case requestPut: @@ -226,7 +226,7 @@ func (req *request) GIDMap() []idtools.IDMap { case requestEval: return nil case requestStat: - return nil + return req.StatOptions.GIDMap case requestGet: return req.GetOptions.GIDMap case requestPut: @@ -284,6 +284,7 @@ type StatForItem struct { Size int64 // dereferenced value for symlinks Mode os.FileMode // dereferenced value for symlinks ModTime time.Time // dereferenced value for symlinks + UID, GID int64 // usually in the uint32 range, set to -1 if unknown IsSymlink bool IsDir bool // dereferenced value for symlinks IsRegular bool // dereferenced value for symlinks @@ -342,8 +343,9 @@ func Eval(root string, directory string, _ EvalOptions) (string, error) { // StatOptions controls parts of Stat()'s behavior. type StatOptions struct { - CheckForArchives bool // check for and populate the IsArchive bit in returned values - Excludes []string // contents to pretend don't exist, using the OS-specific path separator + UIDMap, GIDMap []idtools.IDMap // map from hostIDs to containerIDs when returning results + CheckForArchives bool // check for and populate the IsArchive bit in returned values + Excludes []string // contents to pretend don't exist, using the OS-specific path separator } // Stat globs the specified pattern in the specified directory and returns its @@ -975,7 +977,7 @@ func copierHandler(bulkReader io.Reader, bulkWriter io.Writer, req request) (*re resp := copierHandlerEval(req) return resp, nil, nil case requestStat: - resp := copierHandlerStat(req, pm) + resp := copierHandlerStat(req, pm, idMappings) return resp, nil, nil case requestGet: return copierHandlerGet(bulkWriter, req, pm, idMappings) @@ -1102,7 +1104,7 @@ func copierHandlerEval(req request) *response { return &response{Eval: evalResponse{Evaluated: filepath.Join(req.rootPrefix, resolvedTarget)}} } -func copierHandlerStat(req request, pm *fileutils.PatternMatcher) *response { +func copierHandlerStat(req request, pm *fileutils.PatternMatcher, idMappings *idtools.IDMappings) *response { errorResponse := func(fmtspec string, args ...any) *response { return &response{Error: fmt.Sprintf(fmtspec, args...), Stat: statResponse{}} } @@ -1160,6 +1162,17 @@ func copierHandlerStat(req request, pm *fileutils.PatternMatcher) *response { } result.Size = linfo.Size() result.Mode = linfo.Mode() + result.UID, result.GID = -1, -1 + if uid, gid, err := owner(linfo); err == nil { + if idMappings != nil && !idMappings.Empty() { + hostPair := idtools.IDPair{UID: uid, GID: gid} + uid, gid, err = idMappings.ToContainer(hostPair) + if err != nil { + return errorResponse("copier: stat: mapping host filesystem owners %#v to container filesystem owners: %w", hostPair, err) + } + } + result.UID, result.GID = int64(uid), int64(gid) + } result.ModTime = linfo.ModTime() result.IsDir = linfo.IsDir() result.IsRegular = result.Mode.IsRegular() @@ -1272,7 +1285,7 @@ func checkLinks(item string, req request, info os.FileInfo) (string, os.FileInfo func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMatcher, idMappings *idtools.IDMappings) (*response, func() error, error) { statRequest := req statRequest.Request = requestStat - statResponse := copierHandlerStat(req, pm) + statResponse := copierHandlerStat(req, pm, idMappings) errorResponse := func(fmtspec string, args ...any) (*response, func() error, error) { return &response{Error: fmt.Sprintf(fmtspec, args...), Stat: statResponse.Stat, Get: getResponse{}}, nil, nil } @@ -2270,7 +2283,7 @@ type EnsurePath struct { // EnsureOptions controls parts of Ensure()'s behavior. type EnsureOptions struct { - UIDMap, GIDMap []idtools.IDMap // map from hostIDs to containerIDs in the chroot + UIDMap, GIDMap []idtools.IDMap // map from containerIDs to hostIDs in the chroot Paths []EnsurePath } @@ -2437,7 +2450,7 @@ type ConditionalRemovePath struct { // ConditionalRemoveOptions controls parts of ConditionalRemove()'s behavior. type ConditionalRemoveOptions struct { - UIDMap, GIDMap []idtools.IDMap // map from hostIDs to containerIDs in the chroot + UIDMap, GIDMap []idtools.IDMap // map from containerIDs to hostIDs in the chroot Paths []ConditionalRemovePath } diff --git a/copier/copier_test.go b/copier/copier_test.go index b8589a6f9..abc2ead73 100644 --- a/copier/copier_test.go +++ b/copier/copier_test.go @@ -716,6 +716,7 @@ func testStat(t *testing.T) { if actualContent, ok := testArchive.contents[testItem.Name]; ok { testItem.Size = int64(len(actualContent)) } + checkStatInfoOwnership(t, result) require.Equal(t, testItem.Size, result.Size, "unexpected size difference for %q", name) require.True(t, result.IsRegular, "expected %q.IsRegular to be true", glob) require.False(t, result.IsDir, "expected %q.IsDir to be false", glob) diff --git a/copier/copier_unix_test.go b/copier/copier_unix_test.go index 722672d79..2da316660 100644 --- a/copier/copier_unix_test.go +++ b/copier/copier_unix_test.go @@ -5,6 +5,8 @@ package copier import ( "os" "testing" + + "github.com/stretchr/testify/require" ) const ( @@ -101,3 +103,9 @@ func TestConditionalRemoveChroot(t *testing.T) { testConditionalRemove(t) canChroot = couldChroot } + +func checkStatInfoOwnership(t *testing.T, result *StatForItem) { + t.Helper() + require.EqualValues(t, 0, result.UID, "expected the owning user to be reported") + require.EqualValues(t, 0, result.GID, "expected the owning group to be reported") +} diff --git a/copier/copier_windows_test.go b/copier/copier_windows_test.go index 8533a324a..efaf31d15 100644 --- a/copier/copier_windows_test.go +++ b/copier/copier_windows_test.go @@ -2,7 +2,14 @@ package copier -const ( - testModeMask = int64(0o600) - testIgnoreSymlinkDates = true +import ( + "testing" + + "github.com/stretchr/testify/require" ) + +func checkStatInfoOwnership(t *testing.T, result *StatForItem) { + t.Helper() + require.EqualValues(t, -1, result.UID, "expected the owning user to not be supported") + require.EqualValues(t, -1, result.GID, "expected the owning group to not be supported") +} From bf2cbe16455365aa90d66b29a37f2f7735dfef95 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Mon, 11 Aug 2025 13:53:44 -0400 Subject: [PATCH 5/6] Note that we have to build `true` first for the sake of its tests Add a note that we need a test binary built for the sake of a few conformance tests, for people who run the conformance tests directly instead of using the top-level makefile target. Signed-off-by: Nalin Dahyabhai --- tests/conformance/README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/conformance/README.md b/tests/conformance/README.md index 167b5147e..0bd09a787 100644 --- a/tests/conformance/README.md +++ b/tests/conformance/README.md @@ -27,6 +27,11 @@ docker pull registry.fedoraproject.org/fedora-minimal:42-x86_64 docker pull registry.fedoraproject.org/fedora-minimal ``` +This test program is used as input in a few of the conformance tests: +``` +make tests/conformance/testdata/mount-targets/true +``` + You can run all of the tests with go test (and under `buildah unshare` or `podman unshare` if you're not root): ``` go test -v -timeout=30m -tags "$(./btrfs_installed_tag.sh)" ./tests/conformance From 69a50588c583c33cd9e4ce7d524d88e312459f0e Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Thu, 7 Aug 2025 17:28:45 -0400 Subject: [PATCH 6/6] Rework how we decide what to filter out of layer diffs After narrowing down the list of parent directories which we might need to exclude to those which are present in the base image, filter them out of the layer diff as it is generated. Signed-off-by: Nalin Dahyabhai --- image.go | 124 ++++++++++++++++++++++++++++++++++++++++--------- tests/bud.bats | 29 ++++++++---- 2 files changed, 122 insertions(+), 31 deletions(-) diff --git a/image.go b/image.go index 998076b6e..0dbbd165e 100644 --- a/image.go +++ b/image.go @@ -107,6 +107,8 @@ type containerImageRef struct { extraImageContent map[string]string compatSetParent types.OptionalBool layerExclusions []copier.ConditionalRemovePath + layerMountTargets []copier.ConditionalRemovePath + layerPullUps []copier.EnsureParentPath unsetAnnotations []string setAnnotations []string createdAnnotation types.OptionalBool @@ -784,6 +786,55 @@ func (mb *ociManifestBuilder) manifestAndConfig() ([]byte, []byte, error) { return omanifestbytes, oconfig, nil } +// filterExclusionsByImage returns a slice of the members of "exclusions" which are present in the image with the specified ID +func (i containerImageRef) filterExclusionsByImage(exclusions []copier.EnsureParentPath, imageID string) ([]copier.EnsureParentPath, error) { + if len(exclusions) == 0 || imageID == "" { + return nil, nil + } + var paths []copier.EnsureParentPath + mountPoint, err := i.store.MountImage(imageID, nil, i.mountLabel) + if err != nil { + return nil, err + } + defer func() { + if _, err := i.store.UnmountImage(imageID, false); err != nil { + logrus.Debugf("unmounting image %q: %v", imageID, err) + } + }() + globs := make([]string, 0, len(exclusions)) + for _, exclusion := range exclusions { + globs = append(globs, exclusion.Path) + } + options := copier.StatOptions{} + stats, err := copier.Stat(mountPoint, mountPoint, options, globs) + if err != nil { + return nil, fmt.Errorf("checking for potential exclusion items in image %q: %w", imageID, err) + } + for _, stat := range stats { + for _, exclusion := range exclusions { + if stat.Glob != exclusion.Path { + continue + } + for result, stat := range stat.Results { + if result != exclusion.Path { + continue + } + if exclusion.ModTime != nil && !exclusion.ModTime.Equal(stat.ModTime) { + continue + } + if exclusion.Mode != nil && *exclusion.Mode != stat.Mode { + continue + } + if exclusion.Owner != nil && (int64(exclusion.Owner.UID) != stat.UID && int64(exclusion.Owner.GID) != stat.GID) { + continue + } + paths = append(paths, exclusion) + } + } + } + return paths, nil +} + func (i *containerImageRef) NewImageSource(_ context.Context, _ *types.SystemContext) (src types.ImageSource, err error) { // These maps will let us check if a layer ID is part of one group or another. parentLayerIDs := make(map[string]bool) @@ -948,6 +999,7 @@ func (i *containerImageRef) NewImageSource(_ context.Context, _ *types.SystemCon } var rc io.ReadCloser var errChan chan error + var layerExclusions []copier.ConditionalRemovePath if i.confidentialWorkload.Convert { // Convert the root filesystem into an encrypted disk image. rc, err = i.extractConfidentialWorkloadFS(i.confidentialWorkload) @@ -980,6 +1032,18 @@ func (i *containerImageRef) NewImageSource(_ context.Context, _ *types.SystemCon if i.emptyLayer && layerID == i.layerID { continue } + if layerID == i.layerID { + // We need to filter out any mount targets that we created. + layerExclusions = append(slices.Clone(i.layerExclusions), i.layerMountTargets...) + // And we _might_ need to filter out directories that modified + // by creating and removing mount targets, _if_ they were the + // same in the base image for this stage. + layerPullUps, err := i.filterExclusionsByImage(i.layerPullUps, i.fromImageID) + if err != nil { + return nil, fmt.Errorf("checking which exclusions are in base image %q: %w", i.fromImageID, err) + } + layerExclusions = append(layerExclusions, layerPullUps...) + } // Extract this layer, one of possibly many. rc, err = i.store.Diff("", layerID, diffOptions) if err != nil { @@ -1002,7 +1066,7 @@ func (i *containerImageRef) NewImageSource(_ context.Context, _ *types.SystemCon // At this point, there are multiple ways that can happen. diffBeingAltered := i.compression != archive.Uncompressed diffBeingAltered = diffBeingAltered || i.layerModTime != nil || i.layerLatestModTime != nil - diffBeingAltered = diffBeingAltered || len(i.layerExclusions) != 0 + diffBeingAltered = diffBeingAltered || len(layerExclusions) != 0 if diffBeingAltered { destHasher = digest.Canonical.Digester() multiWriter = io.MultiWriter(counter, destHasher.Hash()) @@ -1022,7 +1086,7 @@ func (i *containerImageRef) NewImageSource(_ context.Context, _ *types.SystemCon // Use specified timestamps in the layer, if we're doing that for history // entries. nestedWriteCloser := ioutils.NewWriteCloserWrapper(writer, writeCloser.Close) - writeCloser = makeFilteredLayerWriteCloser(nestedWriteCloser, i.layerModTime, i.layerLatestModTime, i.layerExclusions) + writeCloser = makeFilteredLayerWriteCloser(nestedWriteCloser, i.layerModTime, i.layerLatestModTime, layerExclusions) writer = writeCloser // Okay, copy from the raw diff through the filter, compressor, and counter and // digesters. @@ -1443,6 +1507,24 @@ func (b *Builder) makeContainerImageRef(options CommitOptions) (*containerImageR return nil, fmt.Errorf("getting the per-container data directory for %q: %w", b.ContainerID, err) } + gatherExclusions := func(excludesFiles []string) ([]copier.ConditionalRemovePath, error) { + var excludes []copier.ConditionalRemovePath + for _, excludesFile := range excludesFiles { + if strings.Contains(excludesFile, containerExcludesSubstring) { + continue + } + excludesData, err := os.ReadFile(excludesFile) + if err != nil { + return nil, fmt.Errorf("reading commit exclusions for %q: %w", b.ContainerID, err) + } + var theseExcludes []copier.ConditionalRemovePath + if err := json.Unmarshal(excludesData, &theseExcludes); err != nil { + return nil, fmt.Errorf("parsing commit exclusions for %q: %w", b.ContainerID, err) + } + excludes = append(excludes, theseExcludes...) + } + return excludes, nil + } mountTargetFiles, err := filepath.Glob(filepath.Join(cdir, containerExcludesDir, "*")) if err != nil { return nil, fmt.Errorf("checking for commit exclusions for %q: %w", b.ContainerID, err) @@ -1451,29 +1533,27 @@ func (b *Builder) makeContainerImageRef(options CommitOptions) (*containerImageR if err != nil { return nil, fmt.Errorf("checking for commit pulled-up items for %q: %w", b.ContainerID, err) } - excludesFiles := slices.Clone(mountTargetFiles) - if !options.ConfidentialWorkloadOptions.Convert && !options.Squash { - excludesFiles = append(excludesFiles, pulledUpFiles...) + layerMountTargets, err := gatherExclusions(mountTargetFiles) + if err != nil { + return nil, err + } + if len(layerMountTargets) > 0 { + logrus.Debugf("these items were created for use as mount targets: %#v", layerMountTargets) + } + layerPullUps, err := gatherExclusions(pulledUpFiles) + if err != nil { + return nil, err + } + if len(layerPullUps) > 0 { + logrus.Debugf("these items appear to have been pulled up: %#v", layerPullUps) } var layerExclusions []copier.ConditionalRemovePath - for _, excludesFile := range excludesFiles { - if strings.Contains(excludesFile, containerExcludesSubstring) { - continue - } - excludesData, err := os.ReadFile(excludesFile) - if err != nil { - return nil, fmt.Errorf("reading commit exclusions for %q: %w", b.ContainerID, err) - } - var excludes []copier.ConditionalRemovePath - if err := json.Unmarshal(excludesData, &excludes); err != nil { - return nil, fmt.Errorf("parsing commit exclusions for %q: %w", b.ContainerID, err) - } - layerExclusions = append(layerExclusions, excludes...) - } if options.CompatLayerOmissions == types.OptionalBoolTrue { - layerExclusions = append(layerExclusions, compatLayerExclusions...) + layerExclusions = slices.Clone(compatLayerExclusions) + } + if len(layerExclusions) > 0 { + logrus.Debugf("excluding these items from committed layer: %#v", layerExclusions) } - logrus.Debugf("excluding these items from committed layer: %#v", layerExclusions) manifestType := options.PreferredManifestType if manifestType == "" { @@ -1577,6 +1657,8 @@ func (b *Builder) makeContainerImageRef(options CommitOptions) (*containerImageR extraImageContent: maps.Clone(options.ExtraImageContent), compatSetParent: options.CompatSetParent, layerExclusions: layerExclusions, + layerMountTargets: layerMountTargets, + layerPullUps: layerPullUps, createdAnnotation: options.CreatedAnnotation, } if ref.created != nil { diff --git a/tests/bud.bats b/tests/bud.bats index f44690f82..70acde48a 100644 --- a/tests/bud.bats +++ b/tests/bud.bats @@ -8817,17 +8817,26 @@ RUN --mount=type=bind,ro,src=/Dockerfile,target=/etc/removed-file --mount=type=b _EOF local source_date_epoch=$(date +%s) - # build a copy of the image that's all squashed - run_buildah build --no-cache --squash --source-date-epoch=$source_date_epoch --rewrite-timestamp -t dir:${TEST_SCRATCH_DIR}/squashed-image ${contextdir} - # build a copy of the image that's "normal" - run_buildah build --no-cache --layers --source-date-epoch=$source_date_epoch --rewrite-timestamp -t not-squashed-image ${contextdir} - # now squash it, with no record of needing to exclude anything carrying over - run_buildah from --name not-squashed-container not-squashed-image - run_buildah commit --squash not-squashed-container dir:${TEST_SCRATCH_DIR}/squashed-later-image - # find the diffs for the two versions, which should look the same + # build a copy of the image that's all squashed from the start + run_buildah build --no-cache --squash=true --source-date-epoch=$source_date_epoch --rewrite-timestamp -t dir:${TEST_SCRATCH_DIR}/squashed-image ${contextdir} + # build a copy of the image where we only commit at the end + run_buildah build --no-cache --layers=false --source-date-epoch=$source_date_epoch --rewrite-timestamp -t not-layered-image ${contextdir} + # build a copy of the image where we commit at every step + run_buildah build --no-cache --layers=true --source-date-epoch=$source_date_epoch --rewrite-timestamp -t layered-image ${contextdir} + # now squash them, with no internal record of needing to exclude anything carrying over + run_buildah from --name not-layered-container not-layered-image + run_buildah commit --squash not-layered-container dir:${TEST_SCRATCH_DIR}/squashed-not-layered-image + run_buildah from --name layered-container layered-image + run_buildah commit --squash layered-container dir:${TEST_SCRATCH_DIR}/squashed-layered-image + # find the diffs for the three versions, which should look the same thanks to the --source-date-epoch --rewrite-timestamp combo local squashed=${TEST_SCRATCH_DIR}/squashed-image/$(dir_image_last_diff ${TEST_SCRATCH_DIR}/squashed-image) - local squashedlater=${TEST_SCRATCH_DIR}/squashed-later-image/$(dir_image_last_diff ${TEST_SCRATCH_DIR}/squashed-later-image) - cmp ${squashed} ${squashedlater} + local notlayered=${TEST_SCRATCH_DIR}/squashed-not-layered-image/$(dir_image_last_diff ${TEST_SCRATCH_DIR}/squashed-not-layered-image) + local layered=${TEST_SCRATCH_DIR}/squashed-layered-image/$(dir_image_last_diff ${TEST_SCRATCH_DIR}/squashed-layered-image) + tar tvf ${squashed} > ${TEST_SCRATCH_DIR}/squashed-image-rootfs.txt + tar tvf ${notlayered} > ${TEST_SCRATCH_DIR}/squashed-not-layered-image-rootfs.txt + tar tvf ${layered} > ${TEST_SCRATCH_DIR}/squashed-layered-image-rootfs.txt + diff -u ${TEST_SCRATCH_DIR}/squashed-layered-image-rootfs.txt ${TEST_SCRATCH_DIR}/squashed-image-rootfs.txt + diff -u ${TEST_SCRATCH_DIR}/squashed-layered-image-rootfs.txt ${TEST_SCRATCH_DIR}/squashed-not-layered-image-rootfs.txt } @test "bud COPY one file to ..../. creates the destination directory" {