diff --git a/pkg/merge/merge.go b/pkg/merge/merge.go new file mode 100644 index 00000000..9d556dd1 --- /dev/null +++ b/pkg/merge/merge.go @@ -0,0 +1,191 @@ +/* +Copyright © 2025-2026 SUSE LLC +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package merge + +import ( + "bytes" + "fmt" + "io" + iofs "io/fs" + "os" + "strings" + + "github.com/suse/elemental/v3/pkg/sys/vfs" +) + +// ChangeType represents the kind of change made to a file relative to a +// common ancestor. The zero value is the empty string, deliberately not a +// valid change, so an uninitialised ChangeType reads as obviously missing. +type ChangeType string + +const ( + ChangeTypeAdded ChangeType = "added" + ChangeTypeModified ChangeType = "modified" + ChangeTypeDeleted ChangeType = "deleted" +) + +// MaxContentCompareSize bounds how large a regular file may be before +// FileChange stops comparing its content byte-for-byte. Files above this +// size whose stat metadata matches are reported as modified rather than +// read in full; this trades a small false-positive risk for bounded I/O +// on user-supplied blobs. +const MaxContentCompareSize = 16 * 1024 * 1024 + +// Conflict represents a file the user and the OS both changed relative to +// the common ancestor. +type Conflict struct { + Path string + UserChange ChangeType + OSChange ChangeType +} + +func (c Conflict) String() string { + return fmt.Sprintf(" %s — user: %s, OS: %s", c.Path, c.UserChange, c.OSChange) +} + +// FormatConflictSummary returns a human-readable summary of detected +// conflicts; returns "" if there are none. +func FormatConflictSummary(volumePath string, conflicts []Conflict) string { + if len(conflicts) == 0 { + return "" + } + + var b strings.Builder + + _, _ = fmt.Fprintf(&b, "Merge conflicts detected for %s (%d file(s)), user version kept:\n", + volumePath, len(conflicts)) + for _, c := range conflicts { + _, _ = fmt.Fprintf(&b, "%s\n", c) + } + + return b.String() +} + +// FileChange reports how a single file changed between oldPath and newPath. +// Returns empty string if the file is unchanged (or absent from both sides). +// +// Regular files are compared by size and then content (up to +// MaxContentCompareSize; larger matched-size files are conservatively +// flagged as modified without being read). Symlinks compared by target. +// Type changes (file->dir, file->symlink, ...) reported as modified. +// Directory-only additions/deletions are ignored (contents at those paths +// are what's meaningful, and snapper reports them individually). +func FileChange(fs vfs.FS, oldPath, newPath string) (ChangeType, error) { + oldInfo, oldErr := fs.Lstat(oldPath) + if oldErr != nil && !os.IsNotExist(oldErr) { + return "", oldErr + } + newInfo, newErr := fs.Lstat(newPath) + if newErr != nil && !os.IsNotExist(newErr) { + return "", newErr + } + + oldExists := oldErr == nil + newExists := newErr == nil + + switch { + case !oldExists && !newExists: + return "", nil + case !oldExists: + if newInfo.IsDir() { + return "", nil + } + return ChangeTypeAdded, nil + case !newExists: + if oldInfo.IsDir() { + return "", nil + } + return ChangeTypeDeleted, nil + } + + differs, err := entriesDiffer(fs, oldInfo, newInfo, oldPath, newPath) + if err != nil { + return "", err + } + if differs { + return ChangeTypeModified, nil + } + return "", nil +} + +// entriesDiffer compares two entries that exist at the same relative path on +// both sides and reports whether they should be considered modified. +func entriesDiffer(fs vfs.FS, oldInfo, newInfo iofs.FileInfo, oldPath, newPath string) (bool, error) { + if oldInfo.Mode().Type() != newInfo.Mode().Type() { + return true, nil + } + if oldInfo.IsDir() { + return false, nil + } + if oldInfo.Mode()&os.ModeSymlink != 0 { + oldTarget, err := fs.Readlink(oldPath) + if err != nil { + return false, err + } + newTarget, err := fs.Readlink(newPath) + if err != nil { + return false, err + } + return oldTarget != newTarget, nil + } + if !oldInfo.Mode().IsRegular() { + return false, nil + } + if oldInfo.Size() != newInfo.Size() { + return true, nil + } + if oldInfo.Size() > MaxContentCompareSize { + // Bigger than the cap; conservatively report modified instead of + // reading the file. Worst case is a false-positive conflict warning. + return true, nil + } + return regularFilesContentDiffer(fs, oldPath, newPath) +} + +func regularFilesContentDiffer(fs vfs.FS, oldPath, newPath string) (bool, error) { + oldF, err := fs.Open(oldPath) + if err != nil { + return false, err + } + defer oldF.Close() + newF, err := fs.Open(newPath) + if err != nil { + return false, err + } + defer newF.Close() + + const bufSize = 32 * 1024 + bufA := make([]byte, bufSize) + bufB := make([]byte, bufSize) + for { + nA, errA := io.ReadFull(oldF, bufA) + nB, errB := io.ReadFull(newF, bufB) + if nA != nB || !bytes.Equal(bufA[:nA], bufB[:nB]) { + return true, nil + } + if errA == io.EOF || errA == io.ErrUnexpectedEOF { + return false, nil + } + if errA != nil { + return false, errA + } + if errB != nil { + return false, errB + } + } +} diff --git a/pkg/merge/merge_suite_test.go b/pkg/merge/merge_suite_test.go new file mode 100644 index 00000000..5e0d00c9 --- /dev/null +++ b/pkg/merge/merge_suite_test.go @@ -0,0 +1,30 @@ +/* +Copyright © 2025-2026 SUSE LLC +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package merge_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestMergeSuite(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Merge test suite") +} diff --git a/pkg/merge/merge_test.go b/pkg/merge/merge_test.go new file mode 100644 index 00000000..85356803 --- /dev/null +++ b/pkg/merge/merge_test.go @@ -0,0 +1,171 @@ +/* +Copyright © 2025-2026 SUSE LLC +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package merge_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/suse/elemental/v3/pkg/merge" + sysmock "github.com/suse/elemental/v3/pkg/sys/mock" + "github.com/suse/elemental/v3/pkg/sys/vfs" +) + +var _ = Describe("FileChange", Label("merge"), func() { + var ( + tfs vfs.FS + cleanup func() + ) + + BeforeEach(func() { + var err error + tfs, cleanup, err = sysmock.TestFS(nil) + Expect(err).NotTo(HaveOccurred()) + Expect(vfs.MkdirAll(tfs, "/old", vfs.DirPerm)).To(Succeed()) + Expect(vfs.MkdirAll(tfs, "/new", vfs.DirPerm)).To(Succeed()) + }) + + AfterEach(func() { + cleanup() + }) + + It("returns empty change when both paths are absent", func() { + change, err := merge.FileChange(tfs, "/old/nope", "/new/nope") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(BeEquivalentTo("")) + }) + + It("returns empty change when both files are identical", func() { + Expect(tfs.WriteFile("/old/a", []byte("hello"), vfs.FilePerm)).To(Succeed()) + Expect(tfs.WriteFile("/new/a", []byte("hello"), vfs.FilePerm)).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/a", "/new/a") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(BeEquivalentTo("")) + }) + + It("reports Added when only the new path exists", func() { + Expect(tfs.WriteFile("/new/added", []byte("v"), vfs.FilePerm)).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/added", "/new/added") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(Equal(merge.ChangeTypeAdded)) + }) + + It("reports Deleted when only the old path exists", func() { + Expect(tfs.WriteFile("/old/gone", []byte("v"), vfs.FilePerm)).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/gone", "/new/gone") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(Equal(merge.ChangeTypeDeleted)) + }) + + It("ignores directory-only additions", func() { + Expect(vfs.MkdirAll(tfs, "/new/newdir", vfs.DirPerm)).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/newdir", "/new/newdir") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(BeEquivalentTo("")) + }) + + It("ignores directory-only deletions", func() { + Expect(vfs.MkdirAll(tfs, "/old/gonedir", vfs.DirPerm)).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/gonedir", "/new/gonedir") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(BeEquivalentTo("")) + }) + + It("reports Modified for different-sized regular files", func() { + Expect(tfs.WriteFile("/old/grew", []byte("aa"), vfs.FilePerm)).To(Succeed()) + Expect(tfs.WriteFile("/new/grew", []byte("aaaa"), vfs.FilePerm)).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/grew", "/new/grew") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(Equal(merge.ChangeTypeModified)) + }) + + It("detects edits that don't change file size", func() { + // Same size (3 bytes each) means the size short-circuit says "equal"; + // only the byte-for-byte compare catches this. + Expect(tfs.WriteFile("/old/cfg", []byte("abc"), vfs.FilePerm)).To(Succeed()) + Expect(tfs.WriteFile("/new/cfg", []byte("xyz"), vfs.FilePerm)).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/cfg", "/new/cfg") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(Equal(merge.ChangeTypeModified)) + }) + + It("detects symlink target changes", func() { + Expect(tfs.Symlink("/old/target-a", "/old/link")).To(Succeed()) + Expect(tfs.Symlink("/new/target-b", "/new/link")).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/link", "/new/link") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(Equal(merge.ChangeTypeModified)) + }) + + It("returns empty change when symlink targets match", func() { + Expect(tfs.Symlink("/somewhere", "/old/link")).To(Succeed()) + Expect(tfs.Symlink("/somewhere", "/new/link")).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/link", "/new/link") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(BeEquivalentTo("")) + }) + + It("reports type changes (dir -> file) as Modified", func() { + Expect(vfs.MkdirAll(tfs, "/old/x", vfs.DirPerm)).To(Succeed()) + Expect(tfs.WriteFile("/new/x", []byte("now a file"), vfs.FilePerm)).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/x", "/new/x") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(Equal(merge.ChangeTypeModified)) + }) + + It("flags same-size files above the cap as Modified without reading them", func() { + // Identical bytes on both sides. Without the cap the content compare + // would report "equal" (no change); reporting Modified confirms the + // cap branch fired instead. + big := make([]byte, merge.MaxContentCompareSize+1) + Expect(tfs.WriteFile("/old/huge", big, vfs.FilePerm)).To(Succeed()) + Expect(tfs.WriteFile("/new/huge", big, vfs.FilePerm)).To(Succeed()) + + change, err := merge.FileChange(tfs, "/old/huge", "/new/huge") + Expect(err).NotTo(HaveOccurred()) + Expect(change).To(Equal(merge.ChangeTypeModified)) + }) +}) + +var _ = Describe("FormatConflictSummary", Label("merge"), func() { + It("returns empty string when no conflicts", func() { + Expect(merge.FormatConflictSummary("/etc", nil)).To(Equal("")) + Expect(merge.FormatConflictSummary("/etc", []merge.Conflict{})).To(Equal("")) + }) + + It("formats a summary with conflicts", func() { + conflicts := []merge.Conflict{ + {Path: "/etc/foo", UserChange: merge.ChangeTypeModified, OSChange: merge.ChangeTypeModified}, + {Path: "/etc/bar", UserChange: merge.ChangeTypeDeleted, OSChange: merge.ChangeTypeModified}, + } + summary := merge.FormatConflictSummary("/etc", conflicts) + Expect(summary).To(ContainSubstring("Merge conflicts detected for /etc (2 file(s))")) + Expect(summary).To(ContainSubstring("/etc/foo — user: modified, OS: modified")) + Expect(summary).To(ContainSubstring("/etc/bar — user: deleted, OS: modified")) + }) +}) diff --git a/pkg/transaction/snapper_upgrade_helper.go b/pkg/transaction/snapper_upgrade_helper.go index 4b040883..31668076 100644 --- a/pkg/transaction/snapper_upgrade_helper.go +++ b/pkg/transaction/snapper_upgrade_helper.go @@ -23,6 +23,7 @@ import ( "os" "path/filepath" "regexp" + "slices" "strconv" "strings" @@ -30,6 +31,7 @@ import ( "github.com/suse/elemental/v3/pkg/chroot" "github.com/suse/elemental/v3/pkg/deployment" "github.com/suse/elemental/v3/pkg/fstab" + "github.com/suse/elemental/v3/pkg/merge" "github.com/suse/elemental/v3/pkg/rsync" "github.com/suse/elemental/v3/pkg/snapper" "github.com/suse/elemental/v3/pkg/sys/vfs" @@ -212,8 +214,8 @@ func (sc snapperContext) configureRWVolumes(trans *Transaction) error { } // merge runs a 3 way merge for snapshotted RW volumes. -// Current implementation solves potential conflicts by always keeping -// custom changes over changes coming from the OS image. +// User changes are always applied over OS changes. When both sides modified the +// same file, user version wins and the conflict is reported to the user. func (sc snapperContext) merge(trans *Transaction) (err error) { var status, tmpDir string @@ -240,10 +242,14 @@ func (sc snapperContext) merge(trans *Transaction) (err error) { return err } - err = sc.applyCustomChanges(status, rwVol.Path, m) + conflicts, err := sc.applyCustomChanges(status, rwVol.Path, m) if err != nil { return err } + if len(conflicts) > 0 { + conflictSummary := merge.FormatConflictSummary(rwVol.Path, conflicts) + sc.s.Logger().Warn("%s", conflictSummary) + } } return nil } @@ -274,13 +280,34 @@ func (sc snapperContext) customChangesStatus(volPath string, merge *Merge, outpu return nil } -// applyCustomChanges reads the given status file and applies reported changes in to the target destination. -// This method is the responsible of applying customizations to the new volume -func (sc snapperContext) applyCustomChanges(status, rwVolPath string, merge *Merge) (err error) { +// userChangeTypeFromSnapper maps snapper's status prefix character to a +// ChangeType. Returns "" for prefixes that don't represent a merge-relevant +// user change (which the caller should skip). +func userChangeTypeFromSnapper(c string) merge.ChangeType { + switch c { + case "+": + return merge.ChangeTypeAdded + case "-": + return merge.ChangeTypeDeleted + case "c", "t", ".": + return merge.ChangeTypeModified + } + return "" +} + +// applyCustomChanges reads the given snapper status file, applies each user +// change to m.New, and reports files the new image also touches (conflicts). +// +// For each user-changed file it calls merge.FileChange(m.Old, m.New) -- at +// this point m.New still holds the pristine new-image content, so that diff +// IS the OS defaults delta for that path. Deletes are buffered and applied +// after the scan so their side-effects can't disturb later per-file checks. +// Non-deletes are appended to the rsync files-from list as before. +func (sc snapperContext) applyCustomChanges(status, rwVolPath string, m *Merge) (conflicts []merge.Conflict, err error) { sc.s.Logger().Debug("rw volume path: %s", rwVolPath) statusF, err := sc.s.FS().OpenFile(status, os.O_RDONLY, vfs.FilePerm) if err != nil { - return err + return nil, err } defer func() { e := statusF.Close() @@ -292,41 +319,61 @@ func (sc snapperContext) applyCustomChanges(status, rwVolPath string, merge *Mer syncFiles := filepath.Join(filepath.Dir(status), fmt.Sprintf("sync_%s", snapper.ConfigName(rwVolPath))) syncF, err := sc.s.FS().OpenFile(syncFiles, os.O_CREATE|os.O_WRONLY, vfs.FilePerm) if err != nil { - return fmt.Errorf("failed opening modified files list: %w", err) + return nil, fmt.Errorf("failed opening modified files list: %w", err) } r := regexp.MustCompile(`(([-+ct.])[p.][u.][g.][x.][a.])\s+(.*)`) + var pendingDeletes []string scanner := bufio.NewScanner(statusF) for scanner.Scan() { line := scanner.Text() match := r.FindStringSubmatch(line) + if len(match) == 0 { + continue + } + if strings.HasPrefix(match[1], "....") { + // Ignore extended attributes changes because the stock snapshot used + // for comparison was taken before SELinux relabelling, hence this is + // likely to list almost every single file. + continue + } - switch { - case len(match) == 0: + rel := strings.TrimPrefix(match[3], rwVolPath) + userChange := userChangeTypeFromSnapper(match[2]) + if userChange == "" { continue - case strings.HasPrefix(match[1], "...."): - // Ignore extended attributes changes because the stock snapshot used for - // comparison was taken before SELINUX relabelling, hence this is likely to - // list almost every single file. + } + + // Per-file OS delta check. m.New is still pristine here: rsync runs + // after this loop and deletes are buffered, so no earlier iteration + // can have mutated the path we're about to inspect. + osChange, ferr := merge.FileChange(sc.s.FS(), filepath.Join(m.Old, rel), filepath.Join(m.New, rel)) + if ferr != nil { + sc.s.Logger().Warn("Could not compute OS change for %s: %v", rel, ferr) + } else if osChange != "" { + conflicts = append(conflicts, merge.Conflict{ + Path: rel, UserChange: userChange, OSChange: osChange, + }) + } + + if userChange == merge.ChangeTypeDeleted { + pendingDeletes = append(pendingDeletes, rel) continue - case match[2] == "-": - err = sc.s.FS().RemoveAll(filepath.Join(merge.New, strings.TrimPrefix(match[3], rwVolPath))) - if err != nil { - _ = syncF.Close() - return err - } - default: - _, err = fmt.Fprintln(syncF, strings.TrimPrefix(match[3], rwVolPath)) // #nosec G705 - if err != nil { - _ = syncF.Close() - return err - } + } + if _, err = fmt.Fprintln(syncF, rel); err != nil { // #nosec G705 + _ = syncF.Close() + return nil, err } } - err = syncF.Close() - if err != nil { - return fmt.Errorf("failed closing modified files list: %w", err) + if err = syncF.Close(); err != nil { + return nil, fmt.Errorf("failed closing modified files list: %w", err) + } + + for _, rel := range pendingDeletes { + if err = sc.s.FS().RemoveAll(filepath.Join(m.New, rel)); err != nil { + return nil, err + } } // Ensure rsync gets the raw path in testing environments @@ -336,12 +383,14 @@ func (sc snapperContext) applyCustomChanges(status, rwVolPath string, merge *Mer syncFlags := append(rsync.DefaultFlags(), "--files-from", syncFiles) sync := rsync.NewRsync(sc.s, rsync.WithContext(sc.ctx), rsync.WithFlags(syncFlags...)) - err = sync.SyncData(merge.Modified, merge.New, snapper.SnapshotsPath) - if err != nil { - return err + if err = sync.SyncData(m.Modified, m.New, snapper.SnapshotsPath); err != nil { + return nil, err } - return nil + slices.SortFunc(conflicts, func(a, b merge.Conflict) int { + return strings.Compare(a.Path, b.Path) + }) + return conflicts, nil } // snapshotIDFromPath determines the snapshot ID form the snapshot root path diff --git a/pkg/transaction/snapper_upgrade_helper_test.go b/pkg/transaction/snapper_upgrade_helper_test.go index d2f301bf..553d2497 100644 --- a/pkg/transaction/snapper_upgrade_helper_test.go +++ b/pkg/transaction/snapper_upgrade_helper_test.go @@ -195,7 +195,21 @@ var _ = Describe("SnapperUpgradeHelper", Label("transaction"), func() { Expect(tfs.WriteFile(filepath.Join(etcMerge.Modified, "unmodifiedFile"), []byte("non modified file"), vfs.FilePerm)).To(Succeed()) Expect(vfs.MkdirAll(tfs, trans.Merges["/home"].Modified, vfs.DirPerm)).To(Succeed()) - // New defaults + // Old stock (the pre-upgrade /etc baseline). Populate everything + // the user's snapper status claims to have touched so per-file + // merge.FileChange lookups see realistic old-vs-new content: + // - modifiedFile: user edited a file that also exists in old + // - deletedFile: user removed a file that existed in old + // - relabelledFile: xattr-only diff (filtered out anyway) + // createdFile intentionally absent -- the user added it fresh. + Expect(vfs.MkdirAll(tfs, etcMerge.Old, vfs.DirPerm)).To(Succeed()) + Expect(tfs.WriteFile(filepath.Join(etcMerge.Old, "modifiedFile"), []byte("old defaults modified file"), vfs.FilePerm)).To(Succeed()) + Expect(tfs.WriteFile(filepath.Join(etcMerge.Old, "deletedFile"), []byte("old deletedFile content"), vfs.FilePerm)).To(Succeed()) + Expect(tfs.WriteFile(filepath.Join(etcMerge.Old, "relabelledFile"), []byte("relabelled file content"), vfs.FilePerm)).To(Succeed()) + + Expect(vfs.MkdirAll(tfs, trans.Merges["/home"].Old, vfs.DirPerm)).To(Succeed()) + + // New defaults already unpacked into the new snapshot tree. Expect(vfs.MkdirAll(tfs, newEtc, vfs.DirPerm)).To(Succeed()) Expect(tfs.WriteFile(filepath.Join(newEtc, "deletedFile"), []byte("to be deleted file"), vfs.FilePerm)).To(Succeed()) Expect(tfs.WriteFile(filepath.Join(newEtc, "modifiedFile"), []byte("new defaults modified file"), vfs.FilePerm)).To(Succeed())