From 1cebf6782a9dd94d0c5cd86476bd6c06f8f832f3 Mon Sep 17 00:00:00 2001 From: Ines Qian Date: Wed, 6 Sep 2023 09:45:50 -0400 Subject: [PATCH] Restrict orig preservation to only /usr/etc directory and files that are claimed by the rpm; delete all incorrectly preserved orig files and create noorigs for them --- pkg/daemon/file_writers.go | 78 ++++++++++++++++++++++++++++++++++++-- pkg/daemon/update.go | 28 +++++++++----- pkg/daemon/update_test.go | 43 +++------------------ 3 files changed, 98 insertions(+), 51 deletions(-) diff --git a/pkg/daemon/file_writers.go b/pkg/daemon/file_writers.go index 017d61c7a0..d7954cd439 100644 --- a/pkg/daemon/file_writers.go +++ b/pkg/daemon/file_writers.go @@ -8,6 +8,7 @@ import ( "os/user" "path/filepath" "strconv" + "strings" ign3types "github.com/coreos/ignition/v2/config/v3_4/types" "github.com/google/renameio" @@ -45,12 +46,81 @@ func noOrigFileStampName(fpath string) string { return filepath.Join(noOrigParentDir(), fpath+".mcdnoorig") } +func isFileOwnedByRPMPkg(fpath string) (bool, bool, error) { + // The first bool returns false if rpm exists, and true otherwise (indicating other Linux distros or Mac). + // We would like to skip orig file creation and preservation for [first bool = true] cases. + // The second bool returns true if the given path is owned by the rpm pkg. + + // Check whether the underlying OS is Fedora/RHEL, skip the rest and return if not (e.g. Mac -> no rpm) + // If we cannot find the rpm binary, return an error. + path, err := exec.LookPath("rpm") + if err != nil { + return true, false, err + } + + // Check whether the given path is owned by an rpm pkg + cmd := exec.Command(path, "-qf", fpath) + out, err := cmd.CombinedOutput() + + if err == nil && cmd.ProcessState.ExitCode() == 0 { + return false, true, nil + } + fileNotOwnedMsg := fmt.Sprintf("file %s is not owned by any package", fpath) + if strings.Contains(string(out), fileNotOwnedMsg) { + return false, false, nil + } + return false, false, fmt.Errorf("command %q returned with unexpected error: %s: %w", cmd, string(out), err) +} + func createOrigFile(fromPath, fpath string) error { - if _, err := os.Stat(noOrigFileStampName(fpath)); err == nil { - // we already created the no orig file for this default file - return nil + orig := false + + // https://issues.redhat.com/browse/OCPBUGS-11437 + // MCO keeps the pull secret to .orig file once it replaced + // Adapt a check used in function deleteStaleData for backwards compatibility: basically if the file doesn't + // exist in /usr/etc (on FCOS/RHCOS) and no rpm is claiming it, we assume the orig file doesn't need to be created. + // Only do the check for existing files because files that wasn't present on disk before MCD took over will never + // be files that were shipped _with_ the underlying OS (e.g. a default chrony config). + if _, err := os.Stat(fpath); err == nil { + rpmNotFound, isOwned, err := isFileOwnedByRPMPkg(fpath) + if isOwned { + // File is owned by an rpm + orig = true + } else if !isOwned && (err == nil) { + // Run on Fedora/RHEL - check whether the file exist in /usr/etc (on FCOS/RHCOS) + if strings.HasPrefix(fpath, "/etc") { + if _, err := os.Stat(withUsrPath(fpath)); err != nil { + if !os.IsNotExist(err) { + return err + } + } else { + orig = true + } + } + } else if rpmNotFound { + // Run on non-Fedora/RHEL machine + klog.Infof("Running on non-Fedora/RHEL, skip orig file preservation.") + } else { + return err + } + } else if !os.IsNotExist(err) { + return err } - if _, err := os.Stat(fpath); os.IsNotExist(err) { + + if !orig { + if _, err := os.Stat(noOrigFileStampName(fpath)); err == nil { + // we already created the no orig file for this default file + return nil + } + + // check whether there is already an orig preservation for the path, and remove upon existence (wrongly preserved) + if _, err := os.Stat(origFileName(fpath)); err == nil { + if delErr := os.Remove(origFileName(fpath)); delErr != nil { + return fmt.Errorf("deleting orig file for %q: %w", origFileName(fpath), delErr) + } + klog.Infof("Removing files %q completely for incorrect preservation", origFileName(fpath)) + } + // create a noorig file that tells the MCD that the file wasn't present on disk before MCD // took over so it can just remove it when deleting stale data, as opposed as restoring a file // that was shipped _with_ the underlying OS (e.g. a default chrony config). diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index 7766b28e61..95a253e814 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -1245,6 +1245,7 @@ func (dn *Daemon) isPathInDropins(path string, systemd *ign3types.Systemd) bool //nolint:gocyclo func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) error { klog.Info("Deleting stale data") + newFileSet := make(map[string]struct{}) for _, f := range newIgnConfig.Storage.Files { newFileSet[f.Path] = struct{}{} @@ -1282,20 +1283,27 @@ func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) e // Add a check for backwards compatibility: basically if the file doesn't exist in /usr/etc (on FCOS/RHCOS) // and no rpm is claiming it, we assume that the orig file came from a wrongful backup of a MachineConfig // file instead of a file originally on disk. See https://bugzilla.redhat.com/show_bug.cgi?id=1814397 - var restore bool - if _, err := exec.Command("rpm", "-qf", f.Path).CombinedOutput(); err == nil { + restore := false + rpmNotFound, isOwned, err := isFileOwnedByRPMPkg(f.Path) + if isOwned { // File is owned by an rpm restore = true - } else if strings.HasPrefix(f.Path, "/etc") && dn.os.IsCoreOSVariant() { - if _, err := os.Stat(withUsrPath(f.Path)); err != nil { - if !os.IsNotExist(err) { - return err + } else if !isOwned && (err == nil) { + // Run on Fedora/RHEL - check whether the file exist in /usr/etc (on FCOS/RHCOS) + if strings.HasPrefix(f.Path, "/etc") { + if _, err := os.Stat(withUsrPath(f.Path)); err != nil { + if !os.IsNotExist(err) { + return err + } + } else { + restore = true } - - // If the error is ErrNotExist then we don't restore the file - } else { - restore = true } + } else if rpmNotFound { + // Run on non-Fedora/RHEL machine + klog.Infof("Running on non-Fedora/RHEL machine, skip file restoration.") + } else { + return err } if restore { diff --git a/pkg/daemon/update_test.go b/pkg/daemon/update_test.go index cd9099f4a3..458475b677 100644 --- a/pkg/daemon/update_test.go +++ b/pkg/daemon/update_test.go @@ -817,51 +817,20 @@ func TestOriginalFileBackupRestore(t *testing.T) { testDir, cleanup := setupTempDirWithEtc(t) defer cleanup() - // Write a normal file as a control to make sure normal case works + // Write a file in the /tmp dir to test whether orig files are selectively preserved controlFile := filepath.Join(testDir, "control-file") err := os.WriteFile(controlFile, []byte("control file contents"), 0755) assert.Nil(t, err) - // Back up that normal file + // Back up the tmp file err = createOrigFile(controlFile, controlFile) assert.Nil(t, err) - // Now try again and make sure it knows it's already backed up + // Now try again and make sure it knows it's already backed up if it should be err = createOrigFile(controlFile, controlFile) assert.Nil(t, err) - // Restore the normal file - err = restorePath(controlFile) - assert.Nil(t, err) - - // The normal file worked, try it with a symlink - // Write a file we can point a symlink at - err = os.WriteFile(filepath.Join(testDir, "target-file"), []byte("target file contents"), 0755) - assert.Nil(t, err) - - // Make a relative symlink - relativeSymlink := filepath.Join(testDir, "etc", "relative-symlink-to-target-file") - relativeSymlinkTarget := filepath.Join("..", "target-file") - err = os.Symlink(relativeSymlinkTarget, relativeSymlink) - assert.Nil(t, err) - - // Back up the relative symlink - err = createOrigFile(relativeSymlink, relativeSymlink) - assert.Nil(t, err) - - // Remove the symlink and write a file over it - fileOverSymlink := filepath.Join(testDir, "etc", "relative-symlink-to-target-file") - err = os.Remove(fileOverSymlink) - assert.Nil(t, err) - err = os.WriteFile(fileOverSymlink, []byte("replacement contents"), 0755) - assert.Nil(t, err) - - // Try to back it up again make sure it knows it's already backed up - err = createOrigFile(relativeSymlink, relativeSymlink) - assert.Nil(t, err) - - // Finally, make sure we can restore the relative symlink if we rollback - err = restorePath(relativeSymlink) - assert.Nil(t, err) - + // Check whether there is an orig preservation for the path - OK: there is no back up for the tmp file + _, err = os.Stat(origFileName(controlFile)) + assert.True(t, os.IsNotExist(err)) }