diff --git a/pkg/daemon/config_drift_monitor.go b/pkg/daemon/config_drift_monitor.go index 271775e9bd..a86e51c7c5 100644 --- a/pkg/daemon/config_drift_monitor.go +++ b/pkg/daemon/config_drift_monitor.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" ign2types "github.com/coreos/ignition/config/v2_2/types" @@ -21,16 +22,47 @@ type configDriftErr struct { error } +func (c *configDriftErr) Unwrap() error { + return c.error +} + // Error type for file config drifts type fileConfigDriftErr struct { error } +func (f *fileConfigDriftErr) Unwrap() error { + return f.error +} + // Error type for systemd unit config drifts type unitConfigDriftErr struct { error } +func (u *unitConfigDriftErr) Unwrap() error { + return u.error +} + +// Error type for SSH key config drifts +type sshConfigDriftErr struct { + error +} + +func (s *sshConfigDriftErr) Unwrap() error { + return s.error +} + +// Error type for unexpected SSH key config drift +type unexpectedSSHFileErr struct { + filename string + error +} + +func (u *unexpectedSSHFileErr) Unwrap() error { + return u.error +} + type ConfigDriftMonitor interface { Start(ConfigDriftMonitorOpts) error Done() <-chan struct{} @@ -43,9 +75,7 @@ type ConfigDriftMonitorOpts struct { OnDrift func(error) // The currently applied MachineConfig. MachineConfig *mcfgv1.MachineConfig - // The Systemd dropin path location. - // Defaults to /etc/systemd/system - SystemdPath string + Paths // Channel to report unknown errors ErrChan chan<- error } @@ -153,10 +183,6 @@ func newConfigDriftWatcher(opts ConfigDriftMonitorOpts) (*configDriftWatcher, er return nil, fmt.Errorf("no machine config provided") } - if opts.SystemdPath == "" { - opts.SystemdPath = pathSystemd - } - c := &configDriftWatcher{ ConfigDriftMonitorOpts: opts, stopCh: make(chan struct{}), @@ -190,11 +216,16 @@ func (c *configDriftWatcher) initialize() error { // This is useful when, for example, someone places a file in /etc using a // MachineConfig (e.g., /etc/foo), but an unknown (to the MachineConfig) file // in /etc (e.g., /etc/bar) is written to. - c.filePaths, err = getFilePathsFromMachineConfig(c.MachineConfig, c.SystemdPath) + c.filePaths, err = getFilePathsFromMachineConfig(c.MachineConfig, c.Paths) if err != nil { return fmt.Errorf("could not get file paths from machine config: %w", err) } + // Add all of the SSH key path fragments to our watch list. + for _, path := range c.Paths.ExpectedSSHPathFragments() { + c.filePaths.Insert(path) + } + // fsnotify (presently) uses inotify instead of fanotify on Linux. // See: https://github.com/fsnotify/fsnotify/issues/114 // @@ -210,7 +241,7 @@ func (c *configDriftWatcher) initialize() error { for _, path := range dirPaths { glog.V(4).Infof("Watching dir: \"%s\"", path) if err := c.watcher.Add(path); err != nil { - return fmt.Errorf("could not add fsnotify watcher to dir \"%s\": %w", path, err) + return fmt.Errorf("could not add fsnotify watcher to dir %q: %w", path, err) } } @@ -274,14 +305,27 @@ func (c *configDriftWatcher) handleFileEvent(event fsnotify.Event) error { return fmt.Errorf("unknown config drift error: %w", err) } +func (c *configDriftWatcher) shouldValidateOnDiskState(event fsnotify.Event) bool { + // If the filename is not in the MachineConfig or it doesn't contain + // /home/core/.ssh, we ignore it. + // + // We do a fuzzy match of /home/core/.ssh because we want to cover any events + // in /home/core/.ssh and/or within /home/core/.ssh/authorized_keys.d. We + // can't watch for those events directly using fsnotify because: + // + // 1. fsnotify cannot recurse into a subdirectories. + // 2. fsnotify cannot alert on files that do not exist. + return c.filePaths.Has(event.Name) || strings.Contains(event.Name, c.Paths.SSHKeyRoot()) +} + // Validates on disk state for potential config drift. func (c *configDriftWatcher) checkMachineConfigForEvent(event fsnotify.Event) error { // Ignore events for files not found in the MachineConfig. - if !c.filePaths.Has(event.Name) { + if !c.shouldValidateOnDiskState(event) { return nil } - if err := validateOnDiskState(c.MachineConfig, c.SystemdPath); err != nil { + if err := validateOnDiskState(c.MachineConfig, c.Paths); err != nil { return &configDriftErr{err} } @@ -289,7 +333,7 @@ func (c *configDriftWatcher) checkMachineConfigForEvent(event fsnotify.Event) er } // Finds the paths for all files in a given MachineConfig. -func getFilePathsFromMachineConfig(mc *mcfgv1.MachineConfig, systemdPath string) (sets.String, error) { +func getFilePathsFromMachineConfig(mc *mcfgv1.MachineConfig, paths Paths) (sets.String, error) { ignConfig, err := ctrlcommon.IgnParseWrapper(mc.Spec.Config.Raw) if err != nil { return sets.String{}, fmt.Errorf("could not get dirs from ignition config: %w", err) @@ -297,16 +341,18 @@ func getFilePathsFromMachineConfig(mc *mcfgv1.MachineConfig, systemdPath string) switch typedConfig := ignConfig.(type) { case ign3types.Config: - return getFilePathsFromIgn3Config(ignConfig.(ign3types.Config), systemdPath), nil + return getFilePathsFromIgn3Config(ignConfig.(ign3types.Config), paths), nil case ign2types.Config: - return getFilePathsFromIgn2Config(ignConfig.(ign2types.Config), systemdPath), nil + return getFilePathsFromIgn2Config(ignConfig.(ign2types.Config), paths), nil default: return sets.String{}, fmt.Errorf("unexpected type for ignition config: %v", typedConfig) } } // Extracts all unique directories from a given Ignition 3 config. -func getFilePathsFromIgn3Config(ignConfig ign3types.Config, systemdPath string) sets.String { +// +//nolint:dupl // This code must be duplicated because of different types. +func getFilePathsFromIgn3Config(ignConfig ign3types.Config, paths Paths) sets.String { files := sets.NewString() // Get all the file paths from the ignition config @@ -318,13 +364,13 @@ func getFilePathsFromIgn3Config(ignConfig ign3types.Config, systemdPath string) // Get all the file paths for systemd dropins from the ignition config for _, unit := range ignConfig.Systemd.Units { - unitPath := getIgn3SystemdUnitPath(systemdPath, unit) + unitPath := paths.SystemdUnitPath(unit.Name) if _, err := os.Stat(unitPath); err == nil && !os.IsNotExist(err) { files.Insert(unitPath) } for _, dropin := range unit.Dropins { - dropinPath := getIgn3SystemdDropinPath(systemdPath, unit, dropin) + dropinPath := paths.SystemdDropinPath(unit.Name, dropin.Name) if _, err := os.Stat(dropinPath); err == nil && !os.IsNotExist(err) { files.Insert(dropinPath) } @@ -335,7 +381,9 @@ func getFilePathsFromIgn3Config(ignConfig ign3types.Config, systemdPath string) } // Extracts all unique directories from a given Ignition 2 config. -func getFilePathsFromIgn2Config(ignConfig ign2types.Config, systemdPath string) sets.String { +// +//nolint:dupl // This code must be duplicated because of different types. +func getFilePathsFromIgn2Config(ignConfig ign2types.Config, paths Paths) sets.String { files := sets.NewString() // Get all the file paths from the ignition config @@ -347,13 +395,13 @@ func getFilePathsFromIgn2Config(ignConfig ign2types.Config, systemdPath string) // Get all the file paths for systemd dropins from the ignition config for _, unit := range ignConfig.Systemd.Units { - unitPath := getIgn2SystemdUnitPath(systemdPath, unit) + unitPath := paths.SystemdUnitPath(unit.Name) if _, err := os.Stat(unitPath); err == nil && !os.IsNotExist(err) { files.Insert(unitPath) } for _, dropin := range unit.Dropins { - dropinPath := getIgn2SystemdDropinPath(systemdPath, unit, dropin) + dropinPath := paths.SystemdDropinPath(unit.Name, dropin.Name) if _, err := os.Stat(dropinPath); err == nil && !os.IsNotExist(err) { files.Insert(dropinPath) } diff --git a/pkg/daemon/config_drift_monitor_test.go b/pkg/daemon/config_drift_monitor_test.go index 228174b970..945e0203e2 100644 --- a/pkg/daemon/config_drift_monitor_test.go +++ b/pkg/daemon/config_drift_monitor_test.go @@ -5,12 +5,14 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" "testing" "time" ign3types "github.com/coreos/ignition/v2/config/v3_2/types" mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" + "github.com/openshift/machine-config-operator/pkg/daemon/constants" "github.com/openshift/machine-config-operator/test/helpers" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -23,6 +25,8 @@ func TestConfigDriftMonitor(t *testing.T) { // their types. fileErr := &configDriftErr{&fileConfigDriftErr{fmt.Errorf("file error")}} unitErr := &configDriftErr{&unitConfigDriftErr{fmt.Errorf("unit error")}} + sshErr := &configDriftErr{&sshConfigDriftErr{fmt.Errorf("ssh error")}} + unexpectedSSHErr := &configDriftErr{&sshConfigDriftErr{&unexpectedSSHFileErr{error: fmt.Errorf("unexpected ssh error")}}} // Filesystem Mutators // These are closures to avoid namespace collisions and pollution since @@ -197,35 +201,106 @@ func TestConfigDriftMonitor(t *testing.T) { expectedErr: unitErr, mutateDropin: chmodFile, }, + // SSH authorized keys + // These target the SSH authorized keys which can be located in + // /home/core/.ssh/authorized_keys (for RHCOS 8) or + // /home/core/.ssh/authorized_keys.d/ignition (for RHCOS 9 / FCOS / SCOS) + // (but not both!) + { + name: "ign SSH content drift", + expectedErr: sshErr, + mutateSSH: changeFileContent, + }, + { + name: "ign SSH touch", + mutateSSH: touchFile, + }, + { + name: "ign SSH rename", + expectedErr: sshErr, + mutateSSH: renameFile, + }, + { + name: "ign SSH delete", + expectedErr: sshErr, + mutateSSH: os.Remove, + }, + { + name: "ign SSH overwrite", + expectedErr: sshErr, + mutateSSH: overwriteFile, + }, + { + name: "ign SSH chmod", + expectedErr: sshErr, + mutateSSH: chmodFile, + }, + // This targets the SSH key directory for mode changes. + { + name: "ign SSH unexpected dir mode", + expectedErr: sshErr, + mutateSSH: func(path string) error { + return chmodFile(filepath.Dir(path)) + }, + }, + // For this test, we write to the unexpected SSH key path on disk which + // should cause a drift. + { + name: "ign SSH unexpected key path", + expectedErr: unexpectedSSHErr, + mutateSSHUnexpected: func(path string) error { + // For SSH, we want to monitor /home/core/.ssh and anything beneath it. + // Assuming /home/core/.ssh/authorized_keys.d/ignition is present, we + // also want to monitor /home/core/.ssh/authorized_keys. We do this so + // we can degrade if SSH keys are written to the old location. + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return err + } + + return os.WriteFile(path, []byte("ssh-rsa 1...\nssh-rsa 2..."), 0o600) + }, + }, } - // Create a mutex for our test cases The mutex is needed because we now - // overwrite the origParentDirPath and noOrigParentDirPath global variables - // so that our filesystem mutations are confined to a tempdir created by the - // test case. However, since this is a global value, we need to be sure that - // only one testcase can use it at a time. Other than that, the test suite - // does a good job of keeping each individual test case isolated in its own - // tempdir. - testMutex := &sync.Mutex{} - - for _, testCase := range testCases { - // Wire up the mutex to each test case before executing so they don't stomp - // on each other. - testCase.testMutex = testMutex + for _, testCase := range append(testCases, getRHCOS9TestCases(testCases)...) { testCase := testCase t.Run(testCase.name, func(t *testing.T) { t.Parallel() - tmpDir := t.TempDir() + testCase.tmpDir = t.TempDir() + os := mockOS{} + if testCase.os == os { + os = rhcos8() + } else { + os = testCase.os + } - testCase.tmpDir = tmpDir - testCase.systemdPath = filepath.Join(tmpDir, pathSystemd) + testCase.Paths = GetPathsWithPrefix(os, testCase.tmpDir) testCase.run(t) }) } } +// Generates RHCOS 9 test cases from a subset of our test cases. This is +// because the setup for those testcases is identical. However, the expected +// SSH key paths are different. These are inferred by the Paths +// constructor. +func getRHCOS9TestCases(testCases []configDriftMonitorTestCase) []configDriftMonitorTestCase { + rhcos9TestCases := []configDriftMonitorTestCase{} + + for i := range testCases { + testCase := testCases[i] + if strings.Contains(testCase.name, "SSH") { + testCase.name = testCase.name + " rhcos9" + testCase.os = rhcos9() + rhcos9TestCases = append(rhcos9TestCases, testCase) + } + } + + return rhcos9TestCases +} + // Holds a testcase and its associated helper funcs type configDriftMonitorTestCase struct { // Name of the test case @@ -234,8 +309,8 @@ type configDriftMonitorTestCase struct { expectedErr error // The tmpdir for the test case (assigned at runtime) tmpDir string - // The systemdroot for the test case (assigned at runtime) - systemdPath string + // The validation paths for the test case (assigned at runtime) + Paths // Only one of these may be used per testcase: // The mutation to apply to the Ignition file mutateFile func(string) error @@ -245,8 +320,12 @@ type configDriftMonitorTestCase struct { mutateUnit func(string) error // The mutation to apply to the systemd dropin file mutateDropin func(string) error - // Mutex to ensure that parallel tests do not stomp on one another - testMutex *sync.Mutex + // The mutation to apply to the SSH authorized key file + mutateSSH func(string) error + // The mutation to apply to the SSH authorized key file in the unexpected location + mutateSSHUnexpected func(string) error + // The OS to target + os mockOS } // Runs the test case @@ -302,7 +381,6 @@ func (tc configDriftMonitorTestCase) run(t *testing.T) { // Configure the config drift monitor opts := ConfigDriftMonitorOpts{ ErrChan: errChan, - SystemdPath: tc.systemdPath, MachineConfig: mc, OnDrift: func(err error) { go func() { @@ -311,6 +389,7 @@ func (tc configDriftMonitorTestCase) run(t *testing.T) { onDriftCalled = true tc.onDriftFunc(t, err) }, + Paths: tc.Paths, } // Start the config drift monitor @@ -390,6 +469,17 @@ func (tc configDriftMonitorTestCase) getIgnConfig(t *testing.T) ign3types.Config setDefaultUIDandGID(compressedFile), }, }, + Passwd: ign3types.Passwd{ + Users: []ign3types.PasswdUser{ + { + Name: constants.CoreUserName, + SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{ + "ssh-rsa key1", + "ssh-rsa key2", + }, + }, + }, + }, Systemd: ign3types.Systemd{ Units: []ign3types.Unit{ { @@ -428,15 +518,23 @@ func (tc configDriftMonitorTestCase) mutate(ignConfig ign3types.Config) error { } if tc.mutateDropin != nil { - dropinPath := getIgn3SystemdDropinPath(tc.systemdPath, ignConfig.Systemd.Units[0], ignConfig.Systemd.Units[0].Dropins[0]) + dropinPath := tc.SystemdDropinPath(ignConfig.Systemd.Units[0].Name, ignConfig.Systemd.Units[0].Dropins[0].Name) return tc.mutateDropin(dropinPath) } if tc.mutateUnit != nil { - unitPath := getIgn3SystemdUnitPath(tc.systemdPath, ignConfig.Systemd.Units[0]) + unitPath := tc.SystemdUnitPath(ignConfig.Systemd.Units[0].Name) return tc.mutateUnit(unitPath) } + if tc.mutateSSH != nil { + return tc.mutateSSH(tc.ExpectedSSHKeyPath()) + } + + if tc.mutateSSHUnexpected != nil { + return tc.mutateSSHUnexpected(tc.UnexpectedSSHKeyPath()) + } + return fmt.Errorf("no file mutator provided") } @@ -461,46 +559,32 @@ func (tc configDriftMonitorTestCase) getFixtures(t *testing.T) (ign3types.Config mc := helpers.CreateMachineConfigFromIgnition(ignConfig) mc.Name = "config-drift-monitor" + string(uuid.NewUUID()) + require.NoError(t, validateOnDiskState(mc, tc.Paths)) + return ignConfig, mc } -// This needs to be a pointer receiver so we can lock / unlock the mutex. func (tc *configDriftMonitorTestCase) writeIgnitionConfig(t *testing.T, ignConfig ign3types.Config) error { t.Helper() - // This is the only place where this mutex is used throughout this test - // suite. We need a mutex because the origParentDirPath and - // noOrigParentDirPath variables are global and our individual test cases - // execute in parallel. - tc.testMutex.Lock() - defer tc.testMutex.Unlock() - - // For the purposes of our test, we want all of our filesystem mutations to - // be contained within our test temp dir. With this in mind, we temporarily - // override these globals with our temp dir. - globals := map[string]*string{ - "usrPath": &usrPath, - "origParentDirPath": &origParentDirPath, - "noOrigParentDirPath": &noOrigParentDirPath, - } - - for name := range globals { - cleanup := helpers.OverrideGlobalPathVar(t, name, globals[name]) - defer cleanup() - } + fw := newFileWriters(tc.Paths, tc.os) // Write files the same way the MCD does. // NOTE: We manually handle the errors here because using require.Nil or // require.NoError will skip the deferred functions, which is undesirable. - if err := writeFiles(ignConfig.Storage.Files); err != nil { + if err := fw.WriteFiles(ignConfig.Storage.Files); err != nil { return fmt.Errorf("could not write ignition config files: %w", err) } // Write systemd units the same way the MCD does. - if err := writeUnits(ignConfig.Systemd.Units, tc.systemdPath, true); err != nil { + if err := fw.WriteUnits(ignConfig.Systemd.Units); err != nil { return fmt.Errorf("could not write systemd units: %w", err) } + if err := writeFileAtomically(tc.ExpectedSSHKeyPath(), []byte(concatSSHKeys(ignConfig.Passwd.Users)), 0o700, 0o600, -1, -1); err != nil { + return err + } + return nil } @@ -526,4 +610,17 @@ func (tc configDriftMonitorTestCase) onDriftFunc(t *testing.T, err error) { if errors.As(tc.expectedErr, &uErr) { assert.ErrorAs(t, err, &uErr) } + + // If the testcase asks for an sshConfigDriftErr, be sure we got one. + var sErr *sshConfigDriftErr + if errors.As(tc.expectedErr, &sErr) { + assert.ErrorAs(t, err, &sErr) + } + + // If the testcase askss for an unexpectedConfigDriftErr, be sure we got one. + var usErr *unexpectedSSHFileErr + if errors.As(tc.expectedErr, &usErr) { + assert.ErrorAs(t, err, &usErr) + assert.NotEmpty(t, usErr.filename) + } } diff --git a/pkg/daemon/constants/constants.go b/pkg/daemon/constants/constants.go index e51f77cffa..0256cbca2d 100644 --- a/pkg/daemon/constants/constants.go +++ b/pkg/daemon/constants/constants.go @@ -84,4 +84,15 @@ const ( // changes to registries.conf will cause a crio reload and require extra logic about whether to drain ContainerRegistryConfPath = "/etc/containers/registries.conf" + + CoreUserSSH = "/home/" + CoreUserName + "/.ssh" + + // SSH keys in RHCOS 8 will be written to /home/core/.ssh/authorized_keys + RHCOS8SSHKeyPath = CoreUserSSH + "/authorized_keys" + + // SSH keys in RHCOS 9 / FCOS / SCOS will be written to /home/core/.ssh/authorized_keys.d/ignition + RHCOS9SSHKeyPath = CoreUserSSH + "/authorized_keys.d/ignition" + + // Core user SSH config + CoreUserSSHConfig = CoreUserSSH + "/config" ) diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 88e7d83340..14d85bc39a 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -42,6 +42,17 @@ import ( mcfglistersv1 "github.com/openshift/machine-config-operator/pkg/generated/listers/machineconfiguration.openshift.io/v1" ) +type osRelease interface { + IsEL9() bool + IsEL() bool + IsFCOS() bool + IsSCOS() bool + IsCoreOSVariant() bool + IsLikeTraditionalRHEL7() bool +} + +var _ osRelease = osrelease.OperatingSystem{} + // Daemon is the dispatch point for the functions of the agent on the // machine. it keeps track of connections and the current state of the update // process. @@ -50,7 +61,7 @@ type Daemon struct { name string // os the operating system the MCD is running on - os osrelease.OperatingSystem + os osRelease // mock is set if we're running as non-root, probably under unit tests mock bool @@ -125,6 +136,12 @@ type Daemon struct { // Used for Hypershift hypershiftConfigMap string + + // Used for pathing + paths Paths + + // Holds the fileWriters funcs + fileWriters fileWriters } // CoreOSDaemon protects the methods that should only be called on CoreOS variants @@ -269,6 +286,8 @@ func New( // report OS & version (if RHCOS or FCOS) to prometheus hostOS.WithLabelValues(hostos.ToPrometheusLabel(), osVersion).Set(1) + paths := GetPaths(hostos) + return &Daemon{ mock: mock, booting: true, @@ -281,6 +300,8 @@ func New( currentConfigPath: currentConfigPath, loggerSupportsJournal: loggerSupportsJournal, configDriftMonitor: NewConfigDriftMonitor(), + paths: paths, + fileWriters: newFileWriters(paths, hostos), }, nil } @@ -596,6 +617,9 @@ func (dn *Daemon) runPreflightConfigDriftCheck() error { start := time.Now() if err := dn.validateOnDiskState(currentOnDisk); err != nil { + if err := dn.shouldDiskStateDegrade(err); err == nil { + return nil + } dn.nodeWriter.Eventf(corev1.EventTypeWarning, "PreflightConfigDriftCheckFailed", err.Error()) glog.Errorf("Preflight config drift check failed: %v", err) return &configDriftErr{err} @@ -1087,6 +1111,11 @@ func (dn *Daemon) applySSHAccessedAnnotation() error { // Called whenever the on-disk config has drifted from the current machineconfig. func (dn *Daemon) onConfigDrift(err error) { + // We only want to warn, but not degrade on a subset of Config Drift. + if err := dn.shouldDiskStateDegrade(err); err == nil { + return + } + dn.nodeWriter.Eventf(corev1.EventTypeWarning, "ConfigDriftDetected", err.Error()) glog.Error(err) if err := dn.updateErrorState(err); err != nil { @@ -1112,7 +1141,7 @@ func (dn *Daemon) startConfigDriftMonitor() { opts := ConfigDriftMonitorOpts{ OnDrift: dn.onConfigDrift, - SystemdPath: pathSystemd, + Paths: dn.paths, ErrChan: dn.exitCh, MachineConfig: currentConfig, } @@ -1936,6 +1965,20 @@ func (dn *CoreOSDaemon) validateKernelArguments(currentConfig *mcfgv1.MachineCon return nil } +func (dn *Daemon) shouldDiskStateDegrade(err error) error { + // If we've found an unexpected SSH key, we should warn, but not degrade. + var uErr *unexpectedSSHFileErr + if errors.As(err, &uErr) { + deleteCmd := fmt.Sprintf("$ oc debug node/%s -- rm %s", dn.node.Name, filepath.Join("/host", uErr.filename)) + warnMsg := fmt.Sprintf("Unexpected SSH file found: %s. Run: %q.", uErr.filename, deleteCmd) + glog.Warning(warnMsg) + dn.nodeWriter.Eventf(corev1.EventTypeWarning, "UnexpectedSSHFileFound", warnMsg) + return nil + } + + return err +} + // Implementation of validateOnDiskState which checks a few conditions func (dn *Daemon) validateOnDiskStateImpl(currentConfig *mcfgv1.MachineConfig) error { // Be sure we're booted into the OS we expect @@ -1951,7 +1994,7 @@ func (dn *Daemon) validateOnDiskStateImpl(currentConfig *mcfgv1.MachineConfig) e } } - return validateOnDiskState(currentConfig, pathSystemd) + return dn.shouldDiskStateDegrade(validateOnDiskState(currentConfig, dn.paths)) } // validateOnDiskState compares the on-disk state against what a configuration diff --git a/pkg/daemon/file_writers.go b/pkg/daemon/file_writers.go index 8c425dc196..a20e805f38 100644 --- a/pkg/daemon/file_writers.go +++ b/pkg/daemon/file_writers.go @@ -16,37 +16,20 @@ import ( ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" ) -var ( - origParentDirPath = filepath.Join("/etc", "machine-config-daemon", "orig") - noOrigParentDirPath = filepath.Join("/etc", "machine-config-daemon", "noorig") - usrPath = "/usr" -) - -func origParentDir() string { - return origParentDirPath -} - -func noOrigParentDir() string { - return noOrigParentDirPath -} - -func origFileName(fpath string) string { - return filepath.Join(origParentDir(), fpath+".mcdorig") +type fileWriters struct { + paths Paths + os osRelease } -// We use this to create a file that indicates that no original file existed on disk -// when we write a file via a MachineConfig. Otherwise the MCD does not differentiate -// between "a file existed due to a previous machineconfig" vs "a file existed on disk -// before the MCD took over". Also see deleteStaleData() above. -// -// The "stamp" part of the name indicates it is not an actual backup file, just an -// empty file to indicate lack of previous existence. -func noOrigFileStampName(fpath string) string { - return filepath.Join(noOrigParentDir(), fpath+".mcdnoorig") +func newFileWriters(paths Paths, os osRelease) fileWriters { + return fileWriters{ + paths: paths, + os: os, + } } -func createOrigFile(fromPath, fpath string) error { - if _, err := os.Stat(noOrigFileStampName(fpath)); err == nil { +func (fw *fileWriters) CreateOrigFile(fromPath, fpath string) error { + if _, err := os.Stat(fw.paths.NoOrigFileStampName(fpath)); err == nil { // we already created the no orig file for this default file return nil } @@ -54,71 +37,36 @@ func createOrigFile(fromPath, fpath string) error { // 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). - if makeErr := os.MkdirAll(filepath.Dir(noOrigFileStampName(fpath)), 0o755); makeErr != nil { + if makeErr := os.MkdirAll(filepath.Dir(fw.paths.NoOrigFileStampName(fpath)), 0o755); makeErr != nil { return fmt.Errorf("creating no orig parent dir: %w", makeErr) } - return writeFileAtomicallyWithDefaults(noOrigFileStampName(fpath), nil) + return writeFileAtomicallyWithDefaults(fw.paths.NoOrigFileStampName(fpath), nil) } // https://bugzilla.redhat.com/show_bug.cgi?id=1970959 // orig file might exist, but be a relative/dangling symlink - if symlinkTarget, err := os.Readlink(origFileName(fpath)); err == nil { + if symlinkTarget, err := os.Readlink(fw.paths.OrigFileName(fpath)); err == nil { if symlinkTarget != "" { return nil } } - if _, err := os.Stat(origFileName(fpath)); err == nil { + if _, err := os.Stat(fw.paths.OrigFileName(fpath)); err == nil { // the orig file is already there and we avoid creating a new one to preserve the real default return nil } - if err := os.MkdirAll(filepath.Dir(origFileName(fpath)), 0o755); err != nil { + if err := os.MkdirAll(filepath.Dir(fw.paths.OrigFileName(fpath)), 0o755); err != nil { return fmt.Errorf("creating orig parent dir: %w", err) } - if out, err := exec.Command("cp", "-a", "--reflink=auto", fromPath, origFileName(fpath)).CombinedOutput(); err != nil { + if out, err := exec.Command("cp", "-a", "--reflink=auto", fromPath, fw.paths.OrigFileName(fpath)).CombinedOutput(); err != nil { return fmt.Errorf("creating orig file for %q: %s: %w", fpath, string(out), err) } return nil } -func writeFileAtomicallyWithDefaults(fpath string, b []byte) error { - return writeFileAtomically(fpath, b, defaultDirectoryPermissions, defaultFilePermissions, -1, -1) -} - -// writeFileAtomically uses the renameio package to provide atomic file writing, we can't use renameio.WriteFile -// directly since we need to 1) Chown 2) go through a buffer since files provided can be big -func writeFileAtomically(fpath string, b []byte, dirMode, fileMode os.FileMode, uid, gid int) error { - dir := filepath.Dir(fpath) - if err := os.MkdirAll(dir, dirMode); err != nil { - return fmt.Errorf("failed to create directory %q: %w", dir, err) - } - t, err := renameio.TempFile(dir, fpath) - if err != nil { - return err - } - defer t.Cleanup() - // Set permissions before writing data, in case the data is sensitive. - if err := t.Chmod(fileMode); err != nil { - return err - } - w := bufio.NewWriter(t) - if _, err := w.Write(b); err != nil { - return err - } - if err := w.Flush(); err != nil { - return err - } - if uid != -1 && gid != -1 { - if err := t.Chown(uid, gid); err != nil { - return err - } - } - return t.CloseAtomicallyReplace() -} - // write dropins to disk -func writeDropins(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error { +func (fw *fileWriters) WriteDropins(u ign3types.Unit) error { for i := range u.Dropins { - dpath := filepath.Join(systemdRoot, u.Name+".d", u.Dropins[i].Name) + dpath := fw.paths.SystemdDropinPath(u.Name, u.Dropins[i].Name) if u.Dropins[i].Contents == nil || *u.Dropins[i].Contents == "" { glog.Infof("Dropin for %s has no content, skipping write", u.Dropins[i].Name) if _, err := os.Stat(dpath); err != nil { @@ -135,9 +83,8 @@ func writeDropins(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) er } glog.Infof("Writing systemd unit dropin %q", u.Dropins[i].Name) - if _, err := os.Stat(withUsrPath(dpath)); err == nil && - isCoreOSVariant { - if err := createOrigFile(withUsrPath(dpath), dpath); err != nil { + if _, err := os.Stat(fw.paths.WithUsrPath(dpath)); err == nil && fw.os.IsCoreOSVariant() { + if err := fw.CreateOrigFile(fw.paths.WithUsrPath(dpath), dpath); err != nil { return err } } @@ -153,7 +100,7 @@ func writeDropins(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) er // writeFiles writes the given files to disk. // it doesn't fetch remote files and expects a flattened config file. -func writeFiles(files []ign3types.File) error { +func (fw *fileWriters) WriteFiles(files []ign3types.File) error { for _, file := range files { glog.Infof("Writing file %q", file.Path) @@ -178,7 +125,7 @@ func writeFiles(files []ign3types.File) error { if err != nil { return fmt.Errorf("failed to retrieve file ownership for file %q: %w", file.Path, err) } - if err := createOrigFile(file.Path, file.Path); err != nil { + if err := fw.CreateOrigFile(file.Path, file.Path); err != nil { return err } if err := writeFileAtomically(file.Path, decodedContents, defaultDirectoryPermissions, mode, uid, gid); err != nil { @@ -189,13 +136,13 @@ func writeFiles(files []ign3types.File) error { } // writeUnit writes a systemd unit and its dropins to disk -func writeUnit(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error { - if err := writeDropins(u, systemdRoot, isCoreOSVariant); err != nil { +func (fw *fileWriters) WriteUnit(u ign3types.Unit) error { + if err := fw.WriteDropins(u); err != nil { return err } // write (or cleanup) path in /etc/systemd/system - fpath := filepath.Join(systemdRoot, u.Name) + fpath := fw.paths.SystemdUnitPath(u.Name) if u.Mask != nil && *u.Mask { // if the unit is masked, symlink fpath to /dev/null and return early. @@ -216,9 +163,8 @@ func writeUnit(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error if u.Contents != nil && *u.Contents != "" { glog.Infof("Writing systemd unit %q", u.Name) - if _, err := os.Stat(withUsrPath(fpath)); err == nil && - isCoreOSVariant { - if err := createOrigFile(withUsrPath(fpath), fpath); err != nil { + if _, err := os.Stat(fw.paths.WithUsrPath(fpath)); err == nil && fw.os.IsCoreOSVariant() { + if err := fw.CreateOrigFile(fw.paths.WithUsrPath(fpath), fpath); err != nil { return err } } @@ -242,10 +188,45 @@ func writeUnit(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error return nil } +func writeFileAtomicallyWithDefaults(fpath string, b []byte) error { + return writeFileAtomically(fpath, b, defaultDirectoryPermissions, defaultFilePermissions, -1, -1) +} + +// writeFileAtomically uses the renameio package to provide atomic file writing, we can't use renameio.WriteFile +// directly since we need to 1) Chown 2) go through a buffer since files provided can be big +func writeFileAtomically(fpath string, b []byte, dirMode, fileMode os.FileMode, uid, gid int) error { + dir := filepath.Dir(fpath) + if err := os.MkdirAll(dir, dirMode); err != nil { + return fmt.Errorf("failed to create directory %q: %w", dir, err) + } + t, err := renameio.TempFile(dir, fpath) + if err != nil { + return err + } + defer t.Cleanup() + // Set permissions before writing data, in case the data is sensitive. + if err := t.Chmod(fileMode); err != nil { + return err + } + w := bufio.NewWriter(t) + if _, err := w.Write(b); err != nil { + return err + } + if err := w.Flush(); err != nil { + return err + } + if uid != -1 && gid != -1 { + if err := t.Chown(uid, gid); err != nil { + return err + } + } + return t.CloseAtomicallyReplace() +} + // writeUnits writes systemd units and their dropins to disk -func writeUnits(units []ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error { +func (fw *fileWriters) WriteUnits(units []ign3types.Unit) error { for _, u := range units { - if err := writeUnit(u, systemdRoot, isCoreOSVariant); err != nil { + if err := fw.WriteUnit(u); err != nil { return err } } @@ -295,8 +276,3 @@ func getFileOwnership(file ign3types.File) (int, int, error) { } return uid, gid, nil } - -// Appends the usrPath variable (/usr) to a given path -func withUsrPath(path string) string { - return filepath.Join(usrPath, path) -} diff --git a/pkg/daemon/on_disk_validation.go b/pkg/daemon/on_disk_validation.go index 1a302dc274..58f6ebbf23 100644 --- a/pkg/daemon/on_disk_validation.go +++ b/pkg/daemon/on_disk_validation.go @@ -2,9 +2,12 @@ package daemon import ( "bytes" + "errors" "fmt" + "io/fs" "os" "path/filepath" + "strings" ign2types "github.com/coreos/ignition/config/v2_2/types" ign3types "github.com/coreos/ignition/v2/config/v3_2/types" @@ -12,10 +15,12 @@ import ( "github.com/google/go-cmp/cmp" mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" + "github.com/openshift/machine-config-operator/pkg/daemon/constants" + "k8s.io/apimachinery/pkg/util/sets" ) // Validates that the on-disk state matches a given MachineConfig. -func validateOnDiskState(currentConfig *mcfgv1.MachineConfig, systemdPath string) error { +func validateOnDiskState(currentConfig *mcfgv1.MachineConfig, paths Paths) error { // And the rest of the disk state // We want to verify the disk state in the spec version that it was created with, // to remove possibilities of behaviour changes due to translation @@ -29,26 +34,140 @@ func validateOnDiskState(currentConfig *mcfgv1.MachineConfig, systemdPath string if err := checkV3Files(ignconfigi.(ign3types.Config).Storage.Files); err != nil { return &fileConfigDriftErr{err} } - if err := checkV3Units(ignconfigi.(ign3types.Config).Systemd.Units, systemdPath); err != nil { + if err := checkV3Units(ignconfigi.(ign3types.Config).Systemd.Units, paths); err != nil { return &unitConfigDriftErr{err} } + + if err := checkV3SSHKeys(ignconfigi.(ign3types.Config).Passwd.Users, paths); err != nil { + return &sshConfigDriftErr{err} + } + return nil case ign2types.Config: if err := checkV2Files(ignconfigi.(ign2types.Config).Storage.Files); err != nil { return &fileConfigDriftErr{err} } - if err := checkV2Units(ignconfigi.(ign2types.Config).Systemd.Units, systemdPath); err != nil { + if err := checkV2Units(ignconfigi.(ign2types.Config).Systemd.Units, paths); err != nil { return &unitConfigDriftErr{err} } + // TODO: Do we need to check for Ign V2 SSH keys too? return nil default: return fmt.Errorf("unexpected type for ignition config: %v", typedConfig) } } +func stripNewlines(lines string) string { + out := []string{} + + for _, line := range strings.Split(lines, "\n") { + if line == "" { + continue + } + + out = append(out, strings.TrimSpace(line)) + } + + return strings.Join(out, "\n") +} + +// Checks the expected key path for the following conditions: +// 1. The dir mode differs from what we expect. (TODO: Should we also verify ownership?) +// 2. The file contents and mode differ from what is expected, while ignoring +// newlines at the end of the file. Ignition seems ot add a trailing newline +// ("\n") onto the end of the file, which breaks this check. +func checkExpectedSSHKeyPath(users []ign3types.PasswdUser, paths Paths) error { + authKeyPath := paths.ExpectedSSHKeyPath() + + if err := checkDirMode(filepath.Dir(authKeyPath), os.FileMode(0o700)); err != nil { + return err + } + + if err := checkFileMode(authKeyPath, os.FileMode(0o600)); err != nil { + return err + } + + contents, err := os.ReadFile(authKeyPath) + if err != nil { + return err + } + + // Trim newlines from the end of the expected file contents as well as the + // actual file contents to avoid a false positive caused by Ignition adding + // newlines when it applies the initial config. + contents = []byte(stripNewlines(string(contents))) + expectedContent := []byte(concatSSHKeys(users)) + + if !bytes.Equal(contents, expectedContent) { + glog.Errorf("content mismatch for file %q (-want +got):\n%s", authKeyPath, cmp.Diff(expectedContent, contents)) + return fmt.Errorf("content mismatch for file: %q", authKeyPath) + } + + return nil +} + +func errIfFileIsFound(path string) error { + if _, err := os.Lstat(path); err == nil { + return &unexpectedSSHFileErr{ + filename: path, + error: fmt.Errorf("expected not to find SSH keys in %s", path), + } + } else if !errors.Is(err, fs.ErrNotExist) { + return err + } + + return nil +} + +// Checks the old (unexpected) SSH key path for the presence of unexpected SSH +// keys or other files. +func checkUnexpectedSSHKeyPath(paths Paths) error { + expectedSSHKeyPath := paths.ExpectedSSHKeyPath() + + // First, check if we have any unexpected path fragments. + for _, path := range paths.UnexpectedSSHPathFragments() { + if err := errIfFileIsFound(path); err != nil { + return err + } + } + + // If we haven't found any unexpected key paths and we're on RHCOS 8, we're all done. + if strings.HasSuffix(expectedSSHKeyPath, constants.RHCOS8SSHKeyPath) { + return nil + } + + // Next, we walk the directory path of /home/core/.ssh/authorized_keys.d and + // scan for any unknonwn path fragments. + keyDir := filepath.Dir(expectedSSHKeyPath) + ignored := sets.NewString(keyDir, expectedSSHKeyPath) + return filepath.WalkDir(keyDir, func(path string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + + // Ignore /home/core/.ssh/authoried_keys.d/ignition and + // /home/core/.ssh/authorized_keys.d + if ignored.Has(path) { + return nil + } + + return errIfFileIsFound(path) + }) +} + +// Checks if SSH keys match the expected state and check that unexpected SSH +// key path fragments are not present. +func checkV3SSHKeys(users []ign3types.PasswdUser, paths Paths) error { + if err := checkExpectedSSHKeyPath(users, paths); err != nil { + return err + } + + return checkUnexpectedSSHKeyPath(paths) +} + // Checks that the ondisk state for a systemd dropin matches the expected state. -func checkV3Dropin(systemdPath string, unit ign3types.Unit, dropin ign3types.Dropin) error { - path := getIgn3SystemdDropinPath(systemdPath, unit, dropin) +func checkV3Dropin(paths Paths, unit ign3types.Unit, dropin ign3types.Dropin) error { + path := paths.SystemdDropinPath(unit.Name, dropin.Name) var content string if dropin.Contents == nil { @@ -72,9 +191,9 @@ func checkV3Dropin(systemdPath string, unit ign3types.Unit, dropin ign3types.Dro // checkV3Units validates the contents of an individual unit in the // target config and returns nil if they match. -func checkV3Unit(unit ign3types.Unit, systemdPath string) error { +func checkV3Unit(unit ign3types.Unit, paths Paths) error { for _, dropin := range unit.Dropins { - if err := checkV3Dropin(systemdPath, unit, dropin); err != nil { + if err := checkV3Dropin(paths, unit, dropin); err != nil { return err } } @@ -84,7 +203,7 @@ func checkV3Unit(unit ign3types.Unit, systemdPath string) error { return nil } - path := getIgn3SystemdUnitPath(systemdPath, unit) + path := paths.SystemdUnitPath(unit.Name) if unit.Mask != nil && *unit.Mask { link, err := filepath.EvalSymlinks(path) if err != nil { @@ -109,9 +228,9 @@ func checkV3Unit(unit ign3types.Unit, systemdPath string) error { // checkV3Units validates the contents of all the units in the // target config and returns nil if they match. -func checkV3Units(units []ign3types.Unit, systemdPath string) error { +func checkV3Units(units []ign3types.Unit, paths Paths) error { for _, unit := range units { - if err := checkV3Unit(unit, systemdPath); err != nil { + if err := checkV3Unit(unit, paths); err != nil { return err } } @@ -121,9 +240,9 @@ func checkV3Units(units []ign3types.Unit, systemdPath string) error { // checkV2Units validates the contents of a given unit in the // target config and returns nil if they match. -func checkV2Unit(unit ign2types.Unit, systemdPath string) error { +func checkV2Unit(unit ign2types.Unit, paths Paths) error { for _, dropin := range unit.Dropins { - path := getIgn2SystemdDropinPath(systemdPath, unit, dropin) + path := paths.SystemdDropinPath(unit.Name, dropin.Name) if err := checkFileContentsAndMode(path, []byte(dropin.Contents), defaultFilePermissions); err != nil { return err } @@ -134,7 +253,7 @@ func checkV2Unit(unit ign2types.Unit, systemdPath string) error { return nil } - path := getIgn2SystemdUnitPath(systemdPath, unit) + path := paths.SystemdUnitPath(unit.Name) if unit.Mask { link, err := filepath.EvalSymlinks(path) if err != nil { @@ -159,9 +278,9 @@ func checkV2Unit(unit ign2types.Unit, systemdPath string) error { // checkV2Units validates the contents of all the units in the // target config and returns nil if they match. -func checkV2Units(units []ign2types.Unit, systemdPath string) error { +func checkV2Units(units []ign2types.Unit, paths Paths) error { for _, unit := range units { - if err := checkV2Unit(unit, systemdPath); err != nil { + if err := checkV2Unit(unit, paths); err != nil { return err } } @@ -220,11 +339,8 @@ func checkV2Files(files []ign2types.File) error { return nil } -// checkFileContentsAndMode reads the file from the filepath and compares its -// contents and mode with the expectedContent and mode parameters. It logs an -// error in case of an error or mismatch and returns the status of the -// evaluation. -func checkFileContentsAndMode(filePath string, expectedContent []byte, mode os.FileMode) error { +// Checks the mode of a given file. +func checkFileMode(filePath string, mode os.FileMode) error { fi, err := os.Lstat(filePath) if err != nil { return fmt.Errorf("could not stat file %q: %w", filePath, err) @@ -232,42 +348,52 @@ func checkFileContentsAndMode(filePath string, expectedContent []byte, mode os.F if fi.Mode() != mode { return fmt.Errorf("mode mismatch for file: %q; expected: %[2]v/%[2]d/%#[2]o; received: %[3]v/%[3]d/%#[3]o", filePath, mode, fi.Mode()) } + + return nil +} + +// checkFileContentsAndMode reads the file from the filepath and compares its +// contents and mode with the expectedContent and mode parameters. It logs an +// error in case of an error or mismatch and returns the status of the +// evaluation. +func checkFileContentsAndMode(filePath string, expectedContent []byte, mode os.FileMode) error { + if err := checkFileMode(filePath, mode); err != nil { + return err + } + contents, err := os.ReadFile(filePath) if err != nil { return fmt.Errorf("could not read file %q: %w", filePath, err) } + if !bytes.Equal(contents, expectedContent) { glog.Errorf("content mismatch for file %q (-want +got):\n%s", filePath, cmp.Diff(expectedContent, contents)) - return fmt.Errorf("content mismatch for file %q", filePath) + return fmt.Errorf("content mismatch for file: %q", filePath) } + return nil } -// Gets the absolute path for a systemd unit and dropin, given a root path. -func getIgn2SystemdDropinPath(systemdPath string, unit ign2types.Unit, dropin ign2types.SystemdDropin) string { - return filepath.Join(getSystemdPath(systemdPath), unit.Name+".d", dropin.Name) -} +// checkDirMode checks a given directory path and compares its mode to what is +// expected. It logs an error in the event of a mismatch. +func checkDirMode(dirPath string, expectedMode os.FileMode) error { + fi, err := os.Lstat(dirPath) -// Gets the absolute path for a systemd unit and dropin, given a root path. -func getIgn3SystemdDropinPath(systemdPath string, unit ign3types.Unit, dropin ign3types.Dropin) string { - return filepath.Join(getSystemdPath(systemdPath), unit.Name+".d", dropin.Name) -} + if err != nil { + return fmt.Errorf("could not stat dir: %q: %w", dirPath, err) + } -// Computes the absolute path for a given system unit file. -func getIgn2SystemdUnitPath(systemdPath string, unit ign2types.Unit) string { - return filepath.Join(getSystemdPath(systemdPath), unit.Name) -} + if !fi.IsDir() { + return fmt.Errorf("expected %q to be a dir", dirPath) + } -// Computes the absolute path for a given system unit file. -func getIgn3SystemdUnitPath(systemdPath string, unit ign3types.Unit) string { - return filepath.Join(getSystemdPath(systemdPath), unit.Name) -} + // We've already checked for the presence of the Dir mode bit above, so we + // subtract it from the actual mode since it will cause a false negative otherwise. + actualMode := fi.Mode() - os.ModeDir -// Gets the systemd path. Defaults to pathSystemd, if empty. -func getSystemdPath(systemdPath string) string { - if systemdPath == "" { - systemdPath = pathSystemd + if actualMode != expectedMode { + return fmt.Errorf("mode mismatch for dir: %q; expected: %[2]v/%[2]d/%#[2]o; received: %[3]v/%[3]d/%#[3]o", dirPath, expectedMode, actualMode) } - return systemdPath + return nil } diff --git a/pkg/daemon/paths.go b/pkg/daemon/paths.go new file mode 100644 index 0000000000..e4a1865631 --- /dev/null +++ b/pkg/daemon/paths.go @@ -0,0 +1,173 @@ +package daemon + +import ( + "path/filepath" + "strings" + + "github.com/openshift/machine-config-operator/pkg/daemon/constants" +) + +// Used to hold global paths so we can override them for testing. +// TODO: Wire this up with the origDirPath and noOrigDirPath so we can avoid mutating global variables. +type Paths struct { + // The Systemd dropin path location. + // Defaults to /etc/systemd/system + systemdPath string + expectedSSHKeyPath string + unexpectedSSHKeyPath string + origParentDirPath string + noOrigParentDirPath string + usrPath string + + // The prefix to append to all paths for testing. + pathPrefix string +} + +func GetPaths(os osRelease) Paths { + p := Paths{ + systemdPath: pathSystemd, + origParentDirPath: filepath.Join("/etc", "machine-config-daemon", "orig"), + noOrigParentDirPath: filepath.Join("/etc", "machine-config-daemon", "noorig"), + usrPath: "/usr", + pathPrefix: "", + } + + if os.IsEL9() || os.IsSCOS() || os.IsFCOS() { + p.expectedSSHKeyPath = constants.RHCOS9SSHKeyPath + p.unexpectedSSHKeyPath = constants.RHCOS8SSHKeyPath + } else { + p.expectedSSHKeyPath = constants.RHCOS8SSHKeyPath + p.unexpectedSSHKeyPath = constants.RHCOS9SSHKeyPath + } + + return p +} + +func GetPathsWithPrefix(os osRelease, pathPrefix string) Paths { + p := GetPaths(os) + p.pathPrefix = pathPrefix + return p +} + +// Gets the root SSH key path dir for a given SSH path; e.g., /home/core/.ssh +func (p *Paths) SSHKeyRoot() string { + return p.appendPrefix(constants.CoreUserSSH) +} + +func (p *Paths) ExpectedSSHKeyPath() string { + return p.appendPrefix(p.expectedSSHKeyPath) +} + +func (p *Paths) UnexpectedSSHKeyPath() string { + return p.appendPrefix(p.unexpectedSSHKeyPath) +} + +// Returns all of the path fragments for the expected SSH path. If expected SSH key +// path is /home/core/.ssh/authorized_keys.d/ignition, it will return +// /home/core/.ssh/authorized_keys.d/ignition, +// /home/core/.ssh/authorized_keys.d, /home/core/.ssh +func (p *Paths) ExpectedSSHPathFragments() []string { + out := []string{constants.CoreUserSSH} + + if p.ExpectedSSHKeyPath() == p.appendPrefix(constants.RHCOS8SSHKeyPath) { + out = append(out, p.rhcos8KeyPathFragments()...) + } else { + out = append(out, p.rhcos9KeyPathFragments()...) + } + + return p.appendToAll(out) +} + +// Returns all of the path fragments for the unexpected SSH path. If the +// unexpected SSH key path is /home/core/.ssh/authorized_keys, it +// will return /home/core/.ssh/authorized_keys, /home/core/.ssh +func (p *Paths) UnexpectedSSHPathFragments() []string { + var out []string + + if p.UnexpectedSSHKeyPath() == p.appendPrefix(constants.RHCOS9SSHKeyPath) { + out = p.rhcos9KeyPathFragments() + } else { + out = p.rhcos8KeyPathFragments() + } + + return p.appendToAll(out) +} + +func (p *Paths) rhcos8KeyPathFragments() []string { + // /home/core/.ssh/authorized_keys + return []string{constants.RHCOS8SSHKeyPath} +} + +func (p *Paths) rhcos9KeyPathFragments() []string { + return []string{ + // /home/core/.ssh/authorized_keys.d/ignition + constants.RHCOS9SSHKeyPath, + // /home/core/.ssh/authorized_keys.d + filepath.Dir(constants.RHCOS9SSHKeyPath), + } +} + +// Gets the systemd root path. +func (p *Paths) SystemdPath() string { + return p.appendPrefix(p.systemdPath) +} + +// Gets the absolute path for a systemd unit, given the unit name. +func (p *Paths) SystemdUnitPath(unitName string) string { + return filepath.Join(p.SystemdPath(), unitName) +} + +// Gets the absolute path for a systemd unit and dropin, given the unit and dropin names. +func (p *Paths) SystemdDropinPath(unitName, dropinName string) string { + // Idempotently adds the ".d" suffix onto the unit name. + if !strings.HasSuffix(unitName, ".d") { + unitName += ".d" + } + + return filepath.Join(p.SystemdPath(), unitName, dropinName) +} + +func (p *Paths) OrigFileName(fpath string) string { + return filepath.Join(p.OrigParentDir(), fpath+".mcdorig") +} + +// We use this to create a file that indicates that no original file existed on disk +// when we write a file via a MachineConfig. Otherwise the MCD does not differentiate +// between "a file existed due to a previous machineconfig" vs "a file existed on disk +// before the MCD took over". Also see deleteStaleData() above. +// +// The "stamp" part of the name indicates it is not an actual backup file, just an +// empty file to indicate lack of previous existence. +func (p *Paths) NoOrigFileStampName(fpath string) string { + return filepath.Join(p.NoOrigParentDir(), fpath+".mcdnoorig") +} + +func (p *Paths) OrigParentDir() string { + return p.appendPrefix(p.origParentDirPath) +} + +func (p *Paths) NoOrigParentDir() string { + return p.appendPrefix(p.noOrigParentDirPath) +} + +func (p *Paths) WithUsrPath(fpath string) string { + return filepath.Join(p.appendPrefix(p.usrPath), fpath) +} + +// Idempotently appends the path prefix onto a given path. +func (p *Paths) appendPrefix(path string) string { + if p.pathPrefix == "" || strings.HasPrefix(path, p.pathPrefix) { + return path + } + + return filepath.Join(p.pathPrefix, path) +} + +// Idempotently appends the path prefix to all items in a given string slice. +func (p *Paths) appendToAll(paths []string) []string { + for i := range paths { + paths[i] = p.appendPrefix(paths[i]) + } + + return paths +} diff --git a/pkg/daemon/paths_test.go b/pkg/daemon/paths_test.go new file mode 100644 index 0000000000..78c70bc9f2 --- /dev/null +++ b/pkg/daemon/paths_test.go @@ -0,0 +1,125 @@ +package daemon + +import ( + "path/filepath" + "testing" + + "github.com/openshift/machine-config-operator/pkg/daemon/constants" + "github.com/stretchr/testify/assert" +) + +func TestPaths(t *testing.T) { + systemdUnitName := "test-unit" + systemdDropinName := "test-dropin" + + rhcos8PathFragments := []string{ + constants.RHCOS8SSHKeyPath, + } + + rhcos9PathFragments := []string{ + constants.RHCOS9SSHKeyPath, + "/home/core/.ssh/authorized_keys.d", + } + + commonExpectedFragments := []string{ + constants.CoreUserSSH, + } + + testCases := []struct { + name string + os osRelease + expectedSSHRoot string + expectedSSHFragments []string + unexpectedSSHFragments []string + expectedSystemdRootPath string + expectedSystemdUnitPath string + expectedSystemdDropinPath string + expectedOrigFileName string + expectedNoOrigFileStampName string + expectedOrigParentDir string + expectedNoOrigParentDir string + expectedWithUsrPath string + }{ + { + name: "RHCOS 8", + os: rhcos8(), + expectedSSHRoot: constants.CoreUserSSH, + expectedSSHFragments: append(commonExpectedFragments, rhcos8PathFragments...), + unexpectedSSHFragments: rhcos9PathFragments, + expectedSystemdRootPath: pathSystemd, + expectedSystemdUnitPath: filepath.Join(pathSystemd, systemdUnitName), + expectedSystemdDropinPath: filepath.Join(pathSystemd, systemdUnitName+".d", systemdDropinName), + expectedOrigFileName: "/etc/machine-config-daemon/orig/something.mcdorig", + expectedNoOrigFileStampName: "/etc/machine-config-daemon/noorig/something.mcdnoorig", + expectedOrigParentDir: "/etc/machine-config-daemon/orig", + expectedNoOrigParentDir: "/etc/machine-config-daemon/noorig", + expectedWithUsrPath: "/usr/something", + }, + { + name: "RHCOS 9", + os: rhcos9(), + expectedSSHRoot: constants.CoreUserSSH, + expectedSSHFragments: append(commonExpectedFragments, rhcos9PathFragments...), + unexpectedSSHFragments: rhcos8PathFragments, + expectedSystemdRootPath: pathSystemd, + expectedSystemdUnitPath: filepath.Join(pathSystemd, systemdUnitName), + expectedSystemdDropinPath: filepath.Join(pathSystemd, systemdUnitName+".d", systemdDropinName), + expectedOrigFileName: "/etc/machine-config-daemon/orig/something.mcdorig", + expectedNoOrigFileStampName: "/etc/machine-config-daemon/noorig/something.mcdnoorig", + expectedOrigParentDir: "/etc/machine-config-daemon/orig", + expectedNoOrigParentDir: "/etc/machine-config-daemon/noorig", + expectedWithUsrPath: "/usr/something", + }, + } + + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name+" Defaults", func(t *testing.T) { + paths := GetPaths(testCase.os) + assert.Equal(t, testCase.expectedSSHRoot, paths.SSHKeyRoot()) + assert.Equal(t, testCase.expectedSSHFragments, paths.ExpectedSSHPathFragments()) + assert.Equal(t, testCase.unexpectedSSHFragments, paths.UnexpectedSSHPathFragments()) + assert.Equal(t, testCase.expectedSystemdRootPath, paths.SystemdPath()) + assert.Equal(t, testCase.expectedSystemdUnitPath, paths.SystemdUnitPath(systemdUnitName)) + assert.Equal(t, testCase.expectedSystemdDropinPath, paths.SystemdDropinPath(systemdUnitName, systemdDropinName)) + assert.Equal(t, testCase.expectedSystemdDropinPath, paths.SystemdDropinPath(systemdUnitName+".d", systemdDropinName)) + assert.Equal(t, testCase.expectedOrigFileName, paths.OrigFileName("something")) + assert.Equal(t, testCase.expectedNoOrigFileStampName, paths.NoOrigFileStampName("something")) + assert.Equal(t, testCase.expectedOrigParentDir, paths.OrigParentDir()) + assert.Equal(t, testCase.expectedNoOrigParentDir, paths.NoOrigParentDir()) + assert.Equal(t, testCase.expectedWithUsrPath, paths.WithUsrPath("something")) + }) + + t.Run(testCase.name+" With Prefix", func(t *testing.T) { + prefix := "/this/is/a/temp/dir" + + apply := func(in string) string { + return filepath.Join(prefix, in) + } + + applyToAll := func(in []string) []string { + out := []string{} + + for _, item := range in { + out = append(out, apply(item)) + } + + return out + } + + paths := GetPathsWithPrefix(testCase.os, prefix) + assert.Equal(t, apply(testCase.expectedSSHRoot), paths.SSHKeyRoot()) + assert.Equal(t, applyToAll(testCase.expectedSSHFragments), paths.ExpectedSSHPathFragments()) + assert.Equal(t, applyToAll(testCase.unexpectedSSHFragments), paths.UnexpectedSSHPathFragments()) + assert.Equal(t, apply(testCase.expectedSystemdRootPath), paths.SystemdPath()) + assert.Equal(t, apply(testCase.expectedSystemdUnitPath), paths.SystemdUnitPath(systemdUnitName)) + assert.Equal(t, apply(testCase.expectedSystemdDropinPath), paths.SystemdDropinPath(systemdUnitName, systemdDropinName)) + assert.Equal(t, apply(testCase.expectedSystemdDropinPath), paths.SystemdDropinPath(systemdUnitName+".d", systemdDropinName)) + assert.Equal(t, apply(testCase.expectedOrigFileName), paths.OrigFileName("something")) + assert.Equal(t, apply(testCase.expectedNoOrigFileStampName), paths.NoOrigFileStampName("something")) + assert.Equal(t, apply(testCase.expectedOrigParentDir), paths.OrigParentDir()) + assert.Equal(t, apply(testCase.expectedNoOrigParentDir), paths.NoOrigParentDir()) + assert.Equal(t, apply(testCase.expectedWithUsrPath), paths.WithUsrPath("something")) + }) + } +} diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index 68dc36febb..245404317c 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "os/exec" "os/user" @@ -36,8 +37,6 @@ const ( defaultDirectoryPermissions os.FileMode = 0o755 // defaultFilePermissions houses the default mode to use when no file permissions are provided defaultFilePermissions os.FileMode = 0o644 - // SSH Keys for user "core" will only be written at /home/core/.ssh - coreUserSSHPath = "/home/core/.ssh/" // fipsFile is the file to check if FIPS is enabled fipsFile = "/proc/sys/crypto/fips_enabled" extensionsRepo = "/etc/yum.repos.d/coreos-extensions.repo" @@ -58,6 +57,46 @@ const ( GPGNoRebootPath = "/etc/machine-config-daemon/no-reboot/containers-gpg.pub" ) +// Used to hold options for writing SSH keys to disk and allow overrides for testing. +type sshKeyOpts struct { + // The system user to use. Defaults to "core". + systemuser string + // The Ignition users to write. + ign3Users []ign3types.PasswdUser + // The SSH key validation paths which contain the expected paths to write the + // SSH keys and any unexpected SSH keys. + paths Paths + // The user ID of the system user (defaults to the "core" uid) + uid int + // The group ID for the system user (defaults to the "core" gid) + gid int +} + +// Populates sshKeyOpts with its defaults. +func (s *sshKeyOpts) populate() error { + if s.systemuser == "" { + s.systemuser = constants.CoreUserName + } + + if s.uid == 0 { + uid, err := lookupUID(s.systemuser) + if err != nil { + return err + } + s.uid = uid + } + + if s.gid == 0 { + gid, err := lookupGID(s.systemuser) + if err != nil { + return err + } + s.gid = gid + } + + return nil +} + func getNodeRef(node *corev1.Node) *corev1.ObjectReference { return &corev1.ObjectReference{ Kind: "Node", @@ -546,13 +585,19 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig) (retErr err } }() - if err := dn.updateSSHKeys(newIgnConfig.Passwd.Users); err != nil { + sshOpts := sshKeyOpts{ + ign3Users: newIgnConfig.Passwd.Users, + paths: dn.paths, + } + + if err := dn.updateSSHKeys(sshOpts); err != nil { return err } defer func() { if retErr != nil { - if err := dn.updateSSHKeys(oldIgnConfig.Passwd.Users); err != nil { + sshOpts.ign3Users = oldIgnConfig.Passwd.Users + if err := dn.updateSSHKeys(sshOpts); err != nil { errs := kubeErrs.NewAggregate([]error{err, retErr}) retErr = fmt.Errorf("error rolling back SSH keys updates: %w", errs) return @@ -635,13 +680,19 @@ func (dn *Daemon) updateHypershift(oldConfig, newConfig *mcfgv1.MachineConfig, d } }() - if err := dn.updateSSHKeys(newIgnConfig.Passwd.Users); err != nil { + sshOpts := sshKeyOpts{ + ign3Users: newIgnConfig.Passwd.Users, + paths: dn.paths, + } + + if err := dn.updateSSHKeys(sshOpts); err != nil { return err } defer func() { if retErr != nil { - if err := dn.updateSSHKeys(oldIgnConfig.Passwd.Users); err != nil { + sshOpts.ign3Users = oldIgnConfig.Passwd.Users + if err := dn.updateSSHKeys(sshOpts); err != nil { errs := kubeErrs.NewAggregate([]error{err, retErr}) retErr = fmt.Errorf("error rolling back SSH keys updates: %w", errs) return @@ -1212,12 +1263,12 @@ func (dn *Daemon) updateFiles(oldIgnConfig, newIgnConfig ign3types.Config) error return nil } -func restorePath(path string) error { - if out, err := exec.Command("cp", "-a", "--reflink=auto", origFileName(path), path).CombinedOutput(); err != nil { - return fmt.Errorf("restoring %q from orig file %q: %s: %w", path, origFileName(path), string(out), err) +func restorePath(path string, paths Paths) error { + if out, err := exec.Command("cp", "-a", "--reflink=auto", paths.OrigFileName(path), path).CombinedOutput(); err != nil { + return fmt.Errorf("restoring %q from orig file %q: %s: %w", path, paths.OrigFileName(path), string(out), err) } - if err := os.Remove(origFileName(path)); err != nil { - return fmt.Errorf("deleting orig file %q: %w", origFileName(path), err) + if err := os.Remove(paths.OrigFileName(path)); err != nil { + return fmt.Errorf("deleting orig file %q: %w", paths.OrigFileName(path), err) } return nil } @@ -1275,12 +1326,12 @@ func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) e if _, ok := newFileSet[f.Path]; ok { continue } - if _, err := os.Stat(noOrigFileStampName(f.Path)); err == nil { - if delErr := os.Remove(noOrigFileStampName(f.Path)); delErr != nil { - return fmt.Errorf("deleting noorig file stamp %q: %w", noOrigFileStampName(f.Path), delErr) + if _, err := os.Stat(dn.paths.NoOrigFileStampName(f.Path)); err == nil { + if delErr := os.Remove(dn.paths.NoOrigFileStampName(f.Path)); delErr != nil { + return fmt.Errorf("deleting noorig file stamp %q: %w", dn.paths.NoOrigFileStampName(f.Path), delErr) } glog.V(2).Infof("Removing file %q completely", f.Path) - } else if _, err := os.Stat(origFileName(f.Path)); err == nil { + } else if _, err := os.Stat(dn.paths.OrigFileName(f.Path)); err == nil { // 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 @@ -1289,7 +1340,7 @@ func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) e // 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 _, err := os.Stat(dn.paths.WithUsrPath(f.Path)); err != nil { if !os.IsNotExist(err) { return err } @@ -1301,15 +1352,15 @@ func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) e } if restore { - if err := restorePath(f.Path); err != nil { + if err := restorePath(f.Path, dn.paths); err != nil { return err } glog.V(2).Infof("Restored file %q", f.Path) continue } - if delErr := os.Remove(origFileName(f.Path)); delErr != nil { - return fmt.Errorf("deleting orig file %q: %w", origFileName(f.Path), delErr) + if delErr := os.Remove(dn.paths.OrigFileName(f.Path)); delErr != nil { + return fmt.Errorf("deleting orig file %q: %w", dn.paths.OrigFileName(f.Path), delErr) } } @@ -1335,24 +1386,24 @@ func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) e newDropinSet := make(map[string]struct{}) for _, u := range newIgnConfig.Systemd.Units { for j := range u.Dropins { - path := filepath.Join(pathSystemd, u.Name+".d", u.Dropins[j].Name) + path := dn.paths.SystemdDropinPath(u.Name, u.Dropins[j].Name) newDropinSet[path] = struct{}{} } - path := filepath.Join(pathSystemd, u.Name) + path := dn.paths.SystemdUnitPath(u.Name) newUnitSet[path] = struct{}{} } for _, u := range oldIgnConfig.Systemd.Units { for j := range u.Dropins { - path := filepath.Join(pathSystemd, u.Name+".d", u.Dropins[j].Name) + path := dn.paths.SystemdDropinPath(u.Name, u.Dropins[j].Name) if _, ok := newDropinSet[path]; !ok { - if _, err := os.Stat(noOrigFileStampName(path)); err == nil { - if delErr := os.Remove(noOrigFileStampName(path)); delErr != nil { - return fmt.Errorf("deleting noorig file stamp %q: %w", noOrigFileStampName(path), delErr) + if _, err := os.Stat(dn.paths.NoOrigFileStampName(path)); err == nil { + if delErr := os.Remove(dn.paths.NoOrigFileStampName(path)); delErr != nil { + return fmt.Errorf("deleting noorig file stamp %q: %w", dn.paths.NoOrigFileStampName(path), delErr) } glog.V(2).Infof("Removing file %q completely", path) - } else if _, err := os.Stat(origFileName(path)); err == nil { - if err := restorePath(path); err != nil { + } else if _, err := os.Stat(dn.paths.OrigFileName(path)); err == nil { + if err := restorePath(path, dn.paths); err != nil { return err } glog.V(2).Infof("Restored file %q", path) @@ -1370,7 +1421,7 @@ func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) e glog.Infof("Removed stale systemd dropin %q", path) } } - path := filepath.Join(pathSystemd, u.Name) + path := dn.paths.SystemdUnitPath(u.Name) if _, ok := newUnitSet[path]; !ok { // since the unit doesn't exist anymore within the MachineConfig, // look to restore defaults here, so that symlinks are removed first @@ -1379,13 +1430,13 @@ func (dn *Daemon) deleteStaleData(oldIgnConfig, newIgnConfig ign3types.Config) e if err := dn.presetUnit(u); err != nil { glog.Infof("Did not restore preset for %s (may not exist): %s", u.Name, err) } - if _, err := os.Stat(noOrigFileStampName(path)); err == nil { - if delErr := os.Remove(noOrigFileStampName(path)); delErr != nil { - return fmt.Errorf("deleting noorig file stamp %q: %w", noOrigFileStampName(path), delErr) + if _, err := os.Stat(dn.paths.NoOrigFileStampName(path)); err == nil { + if delErr := os.Remove(dn.paths.NoOrigFileStampName(path)); delErr != nil { + return fmt.Errorf("deleting noorig file stamp %q: %w", dn.paths.NoOrigFileStampName(path), delErr) } glog.V(2).Infof("Removing file %q completely", path) - } else if _, err := os.Stat(origFileName(path)); err == nil { - if err := restorePath(path); err != nil { + } else if _, err := os.Stat(dn.paths.OrigFileName(path)); err == nil { + if err := restorePath(path, dn.paths); err != nil { return err } glog.V(2).Infof("Restored file %q", path) @@ -1478,10 +1529,8 @@ func (dn *Daemon) writeUnits(units []ign3types.Unit) error { var enabledUnits []string var disabledUnits []string - isCoreOSVariant := dn.os.IsCoreOSVariant() - for _, u := range units { - if err := writeUnit(u, pathSystemd, isCoreOSVariant); err != nil { + if err := dn.fileWriters.WriteUnit(u); err != nil { return fmt.Errorf("daemon could not write systemd unit: %w", err) } // if the unit doesn't note if it should be enabled or disabled then @@ -1527,120 +1576,133 @@ func (dn *Daemon) writeUnits(units []ign3types.Unit) error { // writeFiles writes the given files to disk. // it doesn't fetch remote files and expects a flattened config file. func (dn *Daemon) writeFiles(files []ign3types.File) error { - return writeFiles(files) + return dn.fileWriters.WriteFiles(files) } -func (dn *Daemon) atomicallyWriteSSHKey(keys string) error { - uid, err := lookupUID(constants.CoreUserName) - if err != nil { - return err +func (dn *Daemon) atomicallyWriteSSHKey(opts sshKeyOpts) error { + if err := opts.populate(); err != nil { + return fmt.Errorf("could not populate sshKeyOpts: %w", err) } - gid, err := lookupGID(constants.CoreGroupName) - if err != nil { - return err - } - - var authKeyPath string - if dn.os.IsEL9() || dn.os.IsFCOS() { - // In FCOS and EL9, ignition writes the SSH key to ~/.ssh/authorized_keys.d/ignition, - // and the ssh-key-dir sshd AuthorizedKeysCommand binary reads it from there. - authKeyPath = filepath.Join(coreUserSSHPath, "authorized_keys.d", "ignition") - } else { - authKeyPath = filepath.Join(coreUserSSHPath, "authorized_keys") - } + authKeyPath := opts.paths.ExpectedSSHKeyPath() // Keys should only be written to "/home/core/.ssh" // Once Users are supported fully this should be writing to PasswdUser.HomeDir - glog.Infof("Writing SSHKeys at %q", authKeyPath) + glog.Infof("Writing SSH keys to %q", authKeyPath) - // Creating coreUserSSHPath in advance if it doesn't exist in order to ensure it is owned by core user + // Creating CoreUserSSHPath in advance if it doesn't exist in order to ensure it is owned by core user // See https://bugzilla.redhat.com/show_bug.cgi?id=2107113 - if _, err := os.Stat(coreUserSSHPath); os.IsNotExist(err) { - mkdirCoreCommand := exec.Command("runuser", "-u", constants.CoreUserName, "--", "mkdir", "-m", "0700", "-p", coreUserSSHPath) + authKeyDir := filepath.Dir(authKeyPath) + if _, err := os.Stat(authKeyDir); os.IsNotExist(err) { + glog.Infof("Creating missing SSH key dir at %s", authKeyDir) + mkdirCoreCommand := exec.Command("runuser", "-u", opts.systemuser, "--", "mkdir", "-m", "0700", "-p", authKeyDir) if err := mkdirCoreCommand.Run(); err != nil { return err } } - if err := writeFileAtomically(authKeyPath, []byte(keys), os.FileMode(0o700), os.FileMode(0o600), uid, gid); err != nil { + keys := []byte(concatSSHKeys(opts.ign3Users)) + if err := writeFileAtomically(authKeyPath, keys, os.FileMode(0o700), os.FileMode(0o600), opts.uid, opts.gid); err != nil { return err } - glog.V(2).Infof("Wrote SSHKeys at %s", authKeyPath) + glog.V(2).Infof("Wrote SSH keys to %q", authKeyPath) return nil } // Update a given PasswdUser's SSHKey -func (dn *Daemon) updateSSHKeys(newUsers []ign3types.PasswdUser) error { - if len(newUsers) == 0 { +func (dn *Daemon) updateSSHKeys(opts sshKeyOpts) error { + if len(opts.ign3Users) == 0 { return nil } + if err := opts.populate(); err != nil { + return fmt.Errorf("could not populate sshKeyOpts: %w", err) + } + var uErr user.UnknownUserError - switch _, err := user.Lookup(constants.CoreUserName); { + switch _, err := user.Lookup(opts.systemuser); { case err == nil: case errors.As(err, &uErr): - glog.Info("core user does not exist, and creating users is not supported, so ignoring configuration specified for core user") + glog.Infof("%s user does not exist, and creating users is not supported, so ignoring configuration specified for %s user", opts.systemuser, opts.systemuser) return nil default: - return fmt.Errorf("failed to check if user core exists: %w", err) + return fmt.Errorf("failed to check if user %s exists: %w", opts.systemuser, err) } + // In RHCOS 8.6 or lower, the keys were written to `/home/core/.ssh/authorized_keys`. + // RHCOS 9.0+, FCOS, and SCOS will however expect the keys at `/home/core/.ssh/authorized_keys.d/ignition`. + // Check if the authorized_keys file at the legacy path exists. If it does, remove it. + // It will be recreated at the new fragment path by the atomicallyWriteSSHKey function + // that is called right after. + if dn.os.IsEL9() || dn.os.IsSCOS() || dn.os.IsFCOS() { + if err := cleanSSHKeyPaths(opts); err != nil { + return err + } + } + + // Note we write keys only for the core user and so this ignores the user list + return dn.atomicallyWriteSSHKey(opts) +} + +func concatSSHKeys(newUsers []ign3types.PasswdUser) string { // we're also appending all keys for any user to core, so for now // we pass this to atomicallyWriteSSHKeys to write. // we know these users are "core" ones also cause this slice went through Reconcilable var concatSSHKeys string + for _, u := range newUsers { for _, k := range u.SSHAuthorizedKeys { concatSSHKeys = concatSSHKeys + string(k) + "\n" } } - if !dn.mock { - authKeyPath := filepath.Join(coreUserSSHPath, "authorized_keys") - authKeyFragmentDirPath := filepath.Join(coreUserSSHPath, "authorized_keys.d") - if dn.os.IsFCOS() { - // In older versions of OKD, the keys were written to `/home/core/.ssh/authorized_keys`. - // Newer versions of OKD will however expect the keys at `/home/core/.ssh/authorized_keys.d/ignition`. - // Check if the authorized_keys file at the legacy path exists. If it does, remove it. - // It will be recreated at the new fragment path by the atomicallyWriteSSHKey function - // that is called right after. - _, err := os.Stat(authKeyPath) - if err == nil { - err := os.RemoveAll(authKeyPath) - if err != nil { - return fmt.Errorf("failed to remove path '%s': %w", authKeyPath, err) - } - } else if !os.IsNotExist(err) { - // This shouldn't ever happen - return fmt.Errorf("unexpectedly failed to get info for path '%s': %w", authKeyPath, err) - } + return stripNewlines(concatSSHKeys) +} - // Ensure authorized_keys.d/ignition is the only fragment that exists - keyFragmentsDir, err := ctrlcommon.ReadDir(authKeyFragmentDirPath) - if err == nil { - for _, fragment := range keyFragmentsDir { - if fragment.Name() != "ignition" { - keyPath := filepath.Join(authKeyFragmentDirPath, fragment.Name()) - err := os.RemoveAll(keyPath) - if err != nil { - return fmt.Errorf("failed to remove path '%s': %w", keyPath, err) - } - } - } - } else if !os.IsNotExist(err) { - // This shouldn't ever happen - return fmt.Errorf("unexpectedly failed to get info for path '%s': %w", authKeyFragmentDirPath, err) - } +// Removes the old SSH key path (/home/core/.ssh/authorized_keys), if found. +func cleanSSHKeyPaths(opts sshKeyOpts) error { + oldAuthKeyPath := opts.paths.UnexpectedSSHKeyPath() + _, err := os.Stat(oldAuthKeyPath) + if err == nil { + err := os.RemoveAll(oldAuthKeyPath) + if err != nil { + return fmt.Errorf("failed to remove path '%s': %w", oldAuthKeyPath, err) } + } else if !errors.Is(err, fs.ErrNotExist) { + // This shouldn't ever happen + return fmt.Errorf("unexpectedly failed to get info for path '%s': %w", oldAuthKeyPath, err) + } - // Note we write keys only for the core user and so this ignores the user list - if err := dn.atomicallyWriteSSHKey(concatSSHKeys); err != nil { - return err + return removeNonIgnitionKeyPathFragments(opts) +} + +// Ensures authorized_keys.d/ignition is the only fragment that exists within the /home/core/.ssh dir. +func removeNonIgnitionKeyPathFragments(opts sshKeyOpts) error { + // /home/core/.ssh/authorized_keys.d/ignition + authKeyPath := opts.paths.ExpectedSSHKeyPath() + // /home/core/.ssh/authorized_keys.d + authKeyFragmentDirPath := filepath.Dir(authKeyPath) + // ignition + authKeyFragmentBasename := filepath.Base(authKeyPath) + + keyFragmentsDir, err := ctrlcommon.ReadDir(authKeyFragmentDirPath) + if err == nil { + for _, fragment := range keyFragmentsDir { + if fragment.Name() != authKeyFragmentBasename { + keyPath := filepath.Join(authKeyFragmentDirPath, fragment.Name()) + err := os.RemoveAll(keyPath) + if err != nil { + return fmt.Errorf("failed to remove path '%s': %w", keyPath, err) + } + } } + } else if !errors.Is(err, fs.ErrNotExist) { + // This shouldn't ever happen + return fmt.Errorf("unexpectedly failed to get info for path '%s': %w", authKeyPath, err) } + return nil } diff --git a/pkg/daemon/update_test.go b/pkg/daemon/update_test.go index cb724168b2..f5bb34d60c 100644 --- a/pkg/daemon/update_test.go +++ b/pkg/daemon/update_test.go @@ -10,13 +10,14 @@ import ( "path/filepath" "reflect" "strconv" + "strings" "testing" "time" ign3types "github.com/coreos/ignition/v2/config/v3_2/types" mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" - "github.com/openshift/machine-config-operator/pkg/daemon/osrelease" + "github.com/openshift/machine-config-operator/pkg/daemon/constants" "github.com/openshift/machine-config-operator/test/helpers" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -25,18 +26,98 @@ import ( k8sfake "k8s.io/client-go/kubernetes/fake" ) -func newMockDaemon() Daemon { +type osTestCase struct { + name string + os mockOS +} + +func (o *osTestCase) multiply() []osTestCase { + emptyOS := mockOS{} + if o.os != emptyOS { + return []osTestCase{*o} + } + + return []osTestCase{ + { + name: "RHCOS8 - " + o.name, + os: rhcos8(), + }, + { + name: "RHCOS9 - " + o.name, + os: rhcos9(), + }, + } +} + +// Mock of osrelease.OperatingSystem. Useful for exercising OS-dependent +// codepaths within the MCD. +type mockOS struct { + isEL bool + isEL9 bool + isFCOS bool + isSCOS bool + isLikeTraditionalRHEL7 bool + isCoreOSVariant bool +} + +func (m mockOS) IsEL() bool { + return m.isEL +} + +func (m mockOS) IsEL9() bool { + return m.isEL9 +} + +func (m mockOS) IsFCOS() bool { + return m.isFCOS +} + +func (m mockOS) IsSCOS() bool { + return m.isSCOS +} + +func (m mockOS) IsLikeTraditionalRHEL7() bool { + return m.isLikeTraditionalRHEL7 +} + +func (m mockOS) IsCoreOSVariant() bool { + return m.isCoreOSVariant +} + +// Creates a mockOS instance that will evaluate to RHCOS 8 for code paths that +// require it. +func rhcos8() mockOS { + return mockOS{ + isEL9: false, + isEL: true, + } +} + +// Creates a mockOS instance that will evaluate to RHCOS 9 for code paths that +// require it. +func rhcos9() mockOS { + return mockOS{ + isEL9: true, + isEL: true, + } +} + +func newMockDaemon(t *testing.T) Daemon { // Create a Daemon instance with mocked clients + _, paths, fw := setupTempDirWithEtc(t) + return Daemon{ mock: true, name: "nodeName", - os: osrelease.OperatingSystem{}, + os: rhcos8(), kubeClient: k8sfake.NewSimpleClientset(), bootedOSImageURL: "test", + paths: paths, + fileWriters: fw, } } -func setupTempDirWithEtc(t *testing.T) (string, func()) { +func setupTempDirWithEtc(t *testing.T) (string, Paths, fileWriters) { t.Helper() // Make a temp dir to put the testing files in, and make sure we clean it up @@ -47,21 +128,15 @@ func setupTempDirWithEtc(t *testing.T) (string, func()) { err := os.MkdirAll(etcDir, 0755) require.Nil(t, err) - oldOrigParentDirPath := origParentDirPath - oldNoOrigParentDirPath := noOrigParentDirPath + paths := GetPathsWithPrefix(rhcos8(), testDir) - // Override these package variables so files get written to our testing location - origParentDirPath = filepath.Join(testDir, origParentDirPath) - noOrigParentDirPath = filepath.Join(testDir, noOrigParentDirPath) + fw := newFileWriters(paths, rhcos8()) - return testDir, func() { - // Make sure path variables get put back for other tests - origParentDirPath = oldOrigParentDirPath - noOrigParentDirPath = oldNoOrigParentDirPath - } + return testDir, paths, fw } func TestTruncate(t *testing.T) { + t.Parallel() assert.Equal(t, truncate("", 10), "") assert.Equal(t, truncate("", 1), "") assert.Equal(t, truncate("a", 1), "a") @@ -72,6 +147,8 @@ func TestTruncate(t *testing.T) { } func TestRunCmdSync(t *testing.T) { + t.Parallel() + err := runCmdSync("echo", "hello", "world") assert.Nil(t, err) @@ -82,6 +159,8 @@ func TestRunCmdSync(t *testing.T) { // TestReconcilable attempts to verify the conditions in which configs would and would not be // reconcilable. Welcome to the longest unittest you've ever read. func TestReconcilable(t *testing.T) { + t.Parallel() + oldIgnCfg := ctrlcommon.NewIgnConfig() // oldConfig is the current config of the fake system oldConfig := helpers.CreateMachineConfigFromIgnition(oldIgnCfg) @@ -171,6 +250,8 @@ func TestReconcilable(t *testing.T) { } func TestMachineConfigDiff(t *testing.T) { + t.Parallel() + oldIgnCfg := ctrlcommon.NewIgnConfig() oldConfig := helpers.CreateMachineConfigFromIgnition(oldIgnCfg) oldConfig.ObjectMeta = metav1.ObjectMeta{Name: "oldconfig"} @@ -210,6 +291,8 @@ func newMachineConfigFromFiles(files []ign3types.File) *mcfgv1.MachineConfig { } func TestReconcilableDiff(t *testing.T) { + t.Parallel() + var oldFiles []ign3types.File nOldFiles := uint(10) for i := uint(0); i < nOldFiles; i++ { @@ -244,6 +327,8 @@ func TestReconcilableDiff(t *testing.T) { } func TestKernelAguments(t *testing.T) { + t.Parallel() + tests := []struct { oldKargs []string newKargs []string @@ -286,7 +371,10 @@ func TestKernelAguments(t *testing.T) { rand.Seed(time.Now().UnixNano()) for idx, test := range tests { + idx := idx + test := test t.Run(fmt.Sprintf("case#%d", idx), func(t *testing.T) { + t.Parallel() res := generateKargs(test.oldKargs, test.newKargs) if !reflect.DeepEqual(test.out, res) { @@ -347,10 +435,10 @@ func TestReconcilableSSH(t *testing.T) { } func TestWriteFiles(t *testing.T) { - testDir, cleanup := setupTempDirWithEtc(t) - defer cleanup() + t.Parallel() + testDir, _, _ := setupTempDirWithEtc(t) - d := newMockDaemon() + d := newMockDaemon(t) contents := []byte("hello world\n") encodedContents := dataurl.EncodeBytes(contents) @@ -414,6 +502,7 @@ func TestWriteFiles(t *testing.T) { } for _, test := range tests { + test := test t.Run(test.name, func(t *testing.T) { err := d.writeFiles(test.files) assert.Equal(t, test.expectedErr, err) @@ -427,31 +516,362 @@ func TestWriteFiles(t *testing.T) { } } +// Writes SSH keys to the temp directory and then ensures that they match the +// expected on-disk state by calling the appropriate on-disk validator. +func TestCheckSSHDiskState(t *testing.T) { + t.Parallel() + + type testCase struct { + osTestCase + fileMode os.FileMode + dirMode os.FileMode + errExpected bool + fileContents string + unexpectedPath string + } + + testCases := []testCase{ + { + osTestCase: osTestCase{name: "Happy path"}, + fileContents: "1234\n5678", + }, + { + osTestCase: osTestCase{name: "Ignores trailing newlines"}, + fileContents: "1234\n5678\n\n\n\n\n", + }, + { + osTestCase: osTestCase{name: "Enforces key order"}, + fileContents: "5678\n1234", + errExpected: true, + }, + { + osTestCase: osTestCase{name: "Ignores unnecessary newlines"}, + fileContents: "1234\n\n\n5678", + }, + { + osTestCase: osTestCase{name: "Wrong file mode"}, + fileMode: os.FileMode(0o700), + errExpected: true, + }, + { + osTestCase: osTestCase{name: "Unexpected file contents"}, + errExpected: true, + fileContents: "not-the-expected-contents", + }, + { + osTestCase: osTestCase{name: "Unexpected path"}, + errExpected: true, + }, + { + osTestCase: osTestCase{ + name: "RHCOS9 - Unexpected path fragment", + os: rhcos9(), + }, + errExpected: true, + }, + } + + testFunc := func(t *testing.T, testCase testCase) { + paths := GetPathsWithPrefix(testCase.os, t.TempDir()) + + populatedSSHKeys := []ign3types.PasswdUser{ + { + Name: constants.CoreUserName, + SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{ + "1234", + "5678", + }, + }, + } + + contents := []byte{} + if testCase.fileContents != "" { + contents = []byte(testCase.fileContents) + } else { + contents = []byte(concatSSHKeys(populatedSSHKeys)) + } + + if testCase.fileMode == 0 { + testCase.fileMode = os.FileMode(0o600) + } + + if testCase.dirMode == 0 { + testCase.dirMode = os.FileMode(0o700) + } + + expectedKeyPath := paths.ExpectedSSHKeyPath() + unexpectedKeyPath := paths.UnexpectedSSHKeyPath() + + if strings.Contains(testCase.name, "Unexpected path fragment") { + fragmentPath := filepath.Join(filepath.Dir(expectedKeyPath), "unexpected-fragment") + require.NoError(t, os.MkdirAll(filepath.Dir(fragmentPath), testCase.dirMode)) + require.NoError(t, os.WriteFile(fragmentPath, contents, testCase.fileMode)) + } else if strings.Contains(testCase.name, "Unexpected path") { + require.NoError(t, os.MkdirAll(filepath.Dir(unexpectedKeyPath), testCase.dirMode)) + require.NoError(t, os.WriteFile(unexpectedKeyPath, contents, testCase.fileMode)) + } + + require.NoError(t, os.MkdirAll(filepath.Dir(expectedKeyPath), testCase.dirMode)) + require.NoError(t, os.WriteFile(expectedKeyPath, contents, testCase.fileMode)) + + if testCase.errExpected { + assert.Error(t, checkV3SSHKeys(populatedSSHKeys, paths)) + } else { + assert.NoError(t, checkV3SSHKeys(populatedSSHKeys, paths)) + } + } + + for _, testCase := range testCases { + testCase := testCase + for _, multiplied := range testCase.multiply() { + testCase.osTestCase = multiplied + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + testFunc(t, testCase) + }) + } + } +} + +// Tests writing keys via the updateSSHKeys() method attached to the MCD as +// well as the on-disk validation to ensure that the writes performed will not +// degrade the MCD. func TestUpdateSSHKeys(t *testing.T) { - d := newMockDaemon() + t.Parallel() - // Set up machineconfigs that are identical except for SSH keys - tempUser := ign3types.PasswdUser{Name: "core", SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"1234", "4567"}} - newIgnCfg := ctrlcommon.NewIgnConfig() - newIgnCfg.Passwd.Users = []ign3types.PasswdUser{tempUser} - err := d.updateSSHKeys(newIgnCfg.Passwd.Users) - if err != nil { - t.Errorf("Expected no error. Got %s.", err) + type testCase struct { + osTestCase + ignUsers []ign3types.PasswdUser + expectedFileContents string + } + + expectedContents := "1234\n4567\n" + + testCases := []testCase{ + { + osTestCase: osTestCase{ + name: "SSH keys", + }, + ignUsers: []ign3types.PasswdUser{ + { + Name: constants.CoreUserName, + SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"1234", "4567"}, + }, + }, + expectedFileContents: expectedContents, + }, + { + osTestCase: osTestCase{ + name: "multiple core users", + }, + ignUsers: []ign3types.PasswdUser{ + { + Name: constants.CoreUserName, + SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"1234"}, + }, + { + Name: constants.CoreUserName, + SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"4567"}, + }, + }, + expectedFileContents: expectedContents, + }, + { + osTestCase: osTestCase{ + name: "gapped SSH keys", + }, + ignUsers: []ign3types.PasswdUser{ + { + Name: constants.CoreUserName, + SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{ + "", // Purposely left blank. + "1234", + "\n", + "4567", + "", // Purposely left blank. + }, + }, + }, + expectedFileContents: expectedContents, + }, + { + osTestCase: osTestCase{ + name: "empty SSH keys", + }, + ignUsers: []ign3types.PasswdUser{ + { + Name: constants.CoreUserName, + SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{}, + }, + }, + expectedFileContents: "", + }, + { + osTestCase: osTestCase{ + name: "ignored non-core user", + }, + ignUsers: []ign3types.PasswdUser{ + { + Name: constants.CoreUserName, + SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"1234", "4567"}, + }, + { + Name: "not-a-real-user-go-away", + SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"7890"}, + }, + }, + expectedFileContents: expectedContents, + }, + } + + multipliedTestCases := []testCase{} + for _, testCase := range testCases { + testCase := testCase + for _, multiplied := range testCase.multiply() { + testCase.osTestCase = multiplied + multipliedTestCases = append(multipliedTestCases, testCase) + } + } + // Use the current user so we can ensure that user lookups are successful. + currentUser, err := user.Current() + assert.Nil(t, err) + + testFunc := func(t *testing.T, testCase testCase) { + d := newMockDaemon(t) + // Override the daemon osrelease object with our mock object. + d.os = testCase.os + + paths := GetPathsWithPrefix(testCase.os, t.TempDir()) + + // Create the SSH key directory to avoid calling "runuser" in this test since doing that requires root. + require.NoError(t, os.MkdirAll(filepath.Dir(paths.ExpectedSSHKeyPath()), os.FileMode(0o700))) + + if testCase.os == rhcos9() { + unexpectedPaths := []string{paths.UnexpectedSSHKeyPath()} + + // Create additional path fragments (e.g., + // /home/core/.ssh/authorized_keys.d/fragment-{0,5}) under the expected + // path for RHCOS 9 so we can ensure they're deleted. + for i := 0; i < 5; i++ { + fragmentPath := filepath.Join(filepath.Dir(paths.ExpectedSSHKeyPath()), fmt.Sprintf("fragment-%d", i)) + unexpectedPaths = append(unexpectedPaths, fragmentPath) + } + + // Write SSH key contents to all the unexpected paths as we expect them + // to not be present after we write the SSH keys. + for _, unexpectedPath := range unexpectedPaths { + require.NoError(t, os.MkdirAll(filepath.Dir(unexpectedPath), os.FileMode(0o700))) + require.NoError(t, os.WriteFile(unexpectedPath, []byte(testCase.expectedFileContents), os.FileMode(0o600))) + } + } + + newIgnCfg := ctrlcommon.NewIgnConfig() + newIgnCfg.Passwd.Users = testCase.ignUsers + + // Ensure that we get a disk state validation error because we've + // purposely ensured that we will by writing to the unexpected paths + // above. + assert.Error(t, checkV3SSHKeys(newIgnCfg.Passwd.Users, paths)) + + // Write the SSH key to disk using the updateSSHKeys method. + sshKeyErr := d.updateSSHKeys(sshKeyOpts{ + systemuser: currentUser.Username, + ign3Users: newIgnCfg.Passwd.Users, + uid: -1, + gid: -1, + paths: paths, + }) + assert.NoError(t, sshKeyErr) + + // Ensure that writing the SSH keys clears up the invalid on-disk state. + assert.NoError(t, checkV3SSHKeys(newIgnCfg.Passwd.Users, paths)) + } + + for _, testCase := range multipliedTestCases { + testCase := testCase + for _, multiplied := range testCase.multiply() { + testCase.osTestCase = multiplied + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + testFunc(t, testCase) + }) + } + } +} + +func TestSSHKeyOpts(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + testFunc func(t *testing.T) + }{ + { + name: "Defaults to " + constants.CoreUserName, + testFunc: func(t *testing.T) { + opts := &sshKeyOpts{} + + assert.NotEqual(t, opts.systemuser, constants.CoreUserName) + + // If we cannot look up the core user here, we should expect the lookup + // to fail in the populate function. However, that should not affect + // the behavior of what we're trying to discern here (which is that the + // core username gets set). + _, err := user.Lookup(constants.CoreUserName) + if err != nil { + + assert.Error(t, opts.populate()) + } else { + assert.NoError(t, opts.populate()) + } + + assert.Equal(t, opts.systemuser, constants.CoreUserName) + }, + }, + { + name: "UID / GID overrides", + testFunc: func(t *testing.T) { + opts := &sshKeyOpts{ + systemuser: "nobody", + uid: -1, + gid: -1, + } + + assert.NoError(t, opts.populate()) + assert.Equal(t, "nobody", opts.systemuser) + assert.Equal(t, -1, opts.uid) + assert.Equal(t, -1, opts.gid) + }, + }, + { + name: "Invalid user fails lookup", + testFunc: func(t *testing.T) { + opts := &sshKeyOpts{ + systemuser: "not-a-real-user-go-away", + } + + assert.Error(t, opts.populate()) + assert.Equal(t, 0, opts.uid) + assert.Equal(t, 0, opts.gid) + }, + }, } - // if Users is empty, nothing should happen and no error should ever be generated - newIgnCfg2 := ctrlcommon.NewIgnConfig() - newIgnCfg2.Passwd.Users = []ign3types.PasswdUser{} - err = d.updateSSHKeys(newIgnCfg2.Passwd.Users) - if err != nil { - t.Errorf("Expected no error. Got: %s", err) + for _, testCase := range testCases { + testCase := testCase + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + testCase.testFunc(t) + }) } } // This test should fail until Ignition validation enabled. // Ignition validation does not permit writing files to relative paths. func TestInvalidIgnConfig(t *testing.T) { + t.Parallel() + oldIgnConfig := ctrlcommon.NewIgnConfig() oldMcfg := helpers.CreateMachineConfigFromIgnition(oldIgnConfig) @@ -476,6 +896,8 @@ func TestInvalidIgnConfig(t *testing.T) { } func TestDropinCheck(t *testing.T) { + t.Parallel() + tests := []struct { service string dropin string @@ -508,10 +930,13 @@ func TestDropinCheck(t *testing.T) { }, } - d := newMockDaemon() + d := newMockDaemon(t) for idx, test := range tests { + idx := idx + test := test t.Run(fmt.Sprintf("case#%d", idx), func(t *testing.T) { + t.Parallel() ignCfg := ctrlcommon.NewIgnConfig() ignCfg.Systemd.Units = []ign3types.Unit{ { @@ -544,6 +969,8 @@ func TestDropinCheck(t *testing.T) { // Test to see if the correct action is calculated given a machineconfig diff // i.e. whether we need to reboot and what actions need to be taken if no reboot is needed func TestCalculatePostConfigChangeAction(t *testing.T) { + t.Parallel() + files := map[string]ign3types.File{ "pullsecret1": ctrlcommon.NewIgnFile("/var/lib/kubelet/config.json", "kubelet conf 1\n"), "pullsecret2": ctrlcommon.NewIgnFile("/var/lib/kubelet/config.json", "kubelet conf 2\n"), @@ -639,7 +1066,10 @@ func TestCalculatePostConfigChangeAction(t *testing.T) { } for idx, test := range tests { + idx := idx + test := test t.Run(fmt.Sprintf("case#%d", idx), func(t *testing.T) { + t.Parallel() oldIgnConfig, err := ctrlcommon.ParseAndConvertConfig(test.oldConfig.Spec.Config.Raw) if err != nil { t.Errorf("parsing old Ignition config failed: %v", err) @@ -677,6 +1107,8 @@ func checkIrreconcilableResults(t *testing.T, key string, reconcilableError erro } func TestRunGetOut(t *testing.T) { + t.Parallel() + o, err := runGetOut("true") assert.Nil(t, err) assert.Equal(t, len(o), 0) @@ -701,8 +1133,9 @@ func TestRunGetOut(t *testing.T) { // TestOriginalFileBackupRestore tests backikg up and restoring original files (files that are present in the base image and // get overwritten by a machine configuration) func TestOriginalFileBackupRestore(t *testing.T) { - testDir, cleanup := setupTempDirWithEtc(t) - defer cleanup() + t.Parallel() + + testDir, paths, fw := setupTempDirWithEtc(t) // Write a normal file as a control to make sure normal case works controlFile := filepath.Join(testDir, "control-file") @@ -710,15 +1143,15 @@ func TestOriginalFileBackupRestore(t *testing.T) { assert.Nil(t, err) // Back up that normal file - err = createOrigFile(controlFile, controlFile) + err = fw.CreateOrigFile(controlFile, controlFile) assert.Nil(t, err) // Now try again and make sure it knows it's already backed up - err = createOrigFile(controlFile, controlFile) + err = fw.CreateOrigFile(controlFile, controlFile) assert.Nil(t, err) // Restore the normal file - err = restorePath(controlFile) + err = restorePath(controlFile, paths) assert.Nil(t, err) // The normal file worked, try it with a symlink @@ -733,7 +1166,7 @@ func TestOriginalFileBackupRestore(t *testing.T) { assert.Nil(t, err) // Back up the relative symlink - err = createOrigFile(relativeSymlink, relativeSymlink) + err = fw.CreateOrigFile(relativeSymlink, relativeSymlink) assert.Nil(t, err) // Remove the symlink and write a file over it @@ -744,11 +1177,10 @@ func TestOriginalFileBackupRestore(t *testing.T) { assert.Nil(t, err) // Try to back it up again make sure it knows it's already backed up - err = createOrigFile(relativeSymlink, relativeSymlink) + err = fw.CreateOrigFile(relativeSymlink, relativeSymlink) assert.Nil(t, err) // Finally, make sure we can restore the relative symlink if we rollback - err = restorePath(relativeSymlink) + err = restorePath(relativeSymlink, paths) assert.Nil(t, err) - } diff --git a/test/e2e-shared-tests/mcd_config_drift.go b/test/e2e-shared-tests/mcd_config_drift.go index 5801779733..11d7aa2de7 100644 --- a/test/e2e-shared-tests/mcd_config_drift.go +++ b/test/e2e-shared-tests/mcd_config_drift.go @@ -3,7 +3,6 @@ package e2e_shared_test import ( "context" "fmt" - "path/filepath" "strings" "testing" "time" @@ -24,19 +23,20 @@ import ( ) const ( - configDriftSystemdUnitFilename string = "/etc/systemd/system/e2etest.service" - configDriftSystemdUnitFileContents string = "e2etest-service-unit-contents" - configDriftSystemdDropinFilename string = "/etc/systemd/system/e2etest.service.d/10-e2etest-service.conf" - configDriftSystemdDropinFileContents string = "e2etest-service-dropin-contents" - configDriftCompressedFilename string = "/etc/compressed-file" - configDriftFilename string = "/etc/etc-file" - configDriftFileContents string = "expected-file-data" - configDriftCompressedFilenameTwo string = "/etc/compressed-file-two" - configDriftFilenameTwo string = "/etc/etc-file-two" - configDriftFileContentsTwo string = "expected-file-data-two" - configDriftMCPrefix string = "mcd-config-drift" - configDriftMonitorStartupMsg string = "Config Drift Monitor started" - configDriftMonitorShutdownMsg string = "Config Drift Monitor has shut down" + configDriftSSHKey ign3types.SSHAuthorizedKey = "abc" + configDriftSystemdUnitFilename string = "/etc/systemd/system/e2etest.service" + configDriftSystemdUnitFileContents string = "e2etest-service-unit-contents" + configDriftSystemdDropinFilename string = "/etc/systemd/system/e2etest.service.d/10-e2etest-service.conf" + configDriftSystemdDropinFileContents string = "e2etest-service-dropin-contents" + configDriftCompressedFilename string = "/etc/compressed-file" + configDriftFilename string = "/etc/etc-file" + configDriftFileContents string = "expected-file-data" + configDriftCompressedFilenameTwo string = "/etc/compressed-file-two" + configDriftFilenameTwo string = "/etc/etc-file-two" + configDriftFileContentsTwo string = "expected-file-data-two" + configDriftMCPrefix string = "mcd-config-drift" + configDriftMonitorStartupMsg string = "Config Drift Monitor started" + configDriftMonitorShutdownMsg string = "Config Drift Monitor has shut down" ) // This test does the following: @@ -169,7 +169,7 @@ func (c configDriftTest) getMachineConfig(t *testing.T) *mcfgv1.MachineConfig { Mask: helpers.BoolToPtr(true), }, }, - []ign3types.SSHAuthorizedKey{}, + []ign3types.SSHAuthorizedKey{configDriftSSHKey}, []string{}, false, []string{}, @@ -223,6 +223,32 @@ func (c configDriftTest) Run(t *testing.T) { c.runDegradeAndRecoverContentRevert(t, configDriftCompressedFilename, configDriftFileContents) }, }, + { + name: "revert file content recovery for SSH key", + rebootExpected: false, + testFunc: func(t *testing.T) { + nodeOS := helpers.GetOSReleaseForNode(t, c.ClientSet, c.node).OS + sshPaths := helpers.GetSSHPaths(nodeOS) + t.Run("expected SSH key contents is mutated", func(t *testing.T) { + c.runDegradeAndRecoverContentRevert(t, sshPaths.Expected, string(configDriftSSHKey)) + }) + + unexpectedSSHPaths := []string{sshPaths.NotExpected} + + if sshPaths.Expected == constants.RHCOS9SSHKeyPath { + unexpectedSSHPaths = append(unexpectedSSHPaths, "/home/core/.ssh/authorized_keys.d/should-degrade") + } + + for _, unexpectedSSHPath := range unexpectedSSHPaths { + t.Run("adding unexpected SSH key path causes degradation "+unexpectedSSHPath, func(t *testing.T) { + c.runDegradeAndRecover(t, unexpectedSSHPath, string(configDriftSSHKey), func() { + t.Logf("Deleting %s to initiate recovery", unexpectedSSHPath) + helpers.ExecCmdOnNode(t, c.ClientSet, c.node, "rm", helpers.CanonicalizeNodeFilePath(unexpectedSSHPath)) + }) + }) + } + }, + }, // 1. Mutates a file on the node. // 2. Creates the forcefile to cause recovery. { @@ -235,7 +261,7 @@ func (c configDriftTest) Run(t *testing.T) { c.runDegradeAndRecover(t, configDriftFilename, configDriftFileContents, func() { t.Logf("Setting forcefile to initiate recovery (%s)", constants.MachineConfigDaemonForceFile) - helpers.ExecCmdOnNode(t, c.ClientSet, c.node, "touch", filepath.Join("/rootfs", constants.MachineConfigDaemonForceFile)) + helpers.ExecCmdOnNode(t, c.ClientSet, c.node, "touch", helpers.CanonicalizeNodeFilePath(constants.MachineConfigDaemonForceFile)) }) }, }, @@ -294,6 +320,16 @@ func (c configDriftTest) Run(t *testing.T) { assertNodeAndMCPIsRecovered(t, c.ClientSet, c.node, c.mcp) }, }, + { + name: "SSH keys degrade state", + rebootExpected: false, + testFunc: func(t *testing.T) { + nodeOS := helpers.GetOSReleaseForNode(t, c.ClientSet, c.node).OS + sshPaths := helpers.GetSSHPaths(nodeOS) + + c.runDegradeAndRecoverContentRevert(t, sshPaths.Expected, string(configDriftSSHKey)) + }, + }, } for _, testCase := range testCases { @@ -359,9 +395,7 @@ func (c configDriftTest) runDegradeAndRecover(t *testing.T, filename, expectedFi func mutateFileOnNode(t *testing.T, cs *framework.ClientSet, node corev1.Node, filename, contents string) { t.Helper() - if !strings.HasPrefix(filename, "/rootfs") { - filename = filepath.Join("/rootfs", filename) - } + filename = helpers.CanonicalizeNodeFilePath(filename) bashCmd := fmt.Sprintf("printf '%s' > %s", contents, filename) t.Logf("Setting contents of %s on %s to %s", filename, node.Name, contents) @@ -372,7 +406,7 @@ func mutateFileOnNode(t *testing.T, cs *framework.ClientSet, node corev1.Node, f func assertNodeAndMCPIsDegraded(t *testing.T, cs *framework.ClientSet, node corev1.Node, mcp mcfgv1.MachineConfigPool, filename string) { t.Helper() - logEntry := fmt.Sprintf("content mismatch for file \"%s\"", filename) + logEntry := fmt.Sprintf("content mismatch for file %q", filename) // Assert that the node eventually reaches a Degraded state and has the // config mismatch as the reason diff --git a/test/e2e-single-node/sno_mcd_test.go b/test/e2e-single-node/sno_mcd_test.go index f3e5fc12b7..a86ad1716d 100644 --- a/test/e2e-single-node/sno_mcd_test.go +++ b/test/e2e-single-node/sno_mcd_test.go @@ -3,6 +3,7 @@ package e2e_sno_test import ( "context" "fmt" + "path/filepath" "strconv" "strings" "testing" @@ -246,6 +247,7 @@ func TestExtensions(t *testing.T) { t.Logf("Node %s has successfully rolled back", node.Name) } +// TODO: Can this test be moved to e2e-shared since it is very similar to the one in e2e/mcd_test.go? func TestNoReboot(t *testing.T) { cs := framework.NewClientSet("") @@ -259,14 +261,34 @@ func TestNoReboot(t *testing.T) { oldTime := strings.Split(output, " ")[0] t.Logf("Node %s initial uptime: %s", node.Name, oldTime) + sshKeyContent := "test adding authorized key without node reboot" + + nodeOS := helpers.GetOSReleaseForNode(t, cs, node).OS + + sshPaths := helpers.GetSSHPaths(nodeOS) + + t.Logf("Expecting SSH keys to be in %s", sshPaths.Expected) + + if sshPaths.Expected == constants.RHCOS9SSHKeyPath { + // Write an SSH key to the old location on the node because the update process should remove this file. + t.Logf("Writing SSH key to %s to ensure that it will be removed later", sshPaths.NotExpected) + bashCmd := fmt.Sprintf("printf '%s' > %s", sshKeyContent, filepath.Join("/rootfs", sshPaths.NotExpected)) + helpers.ExecCmdOnNode(t, cs, node, "/bin/bash", "-c", bashCmd) + } + + // Delete the expected SSH keys directory to ensure that the directories are + // (re)created correctly by the MCD. This targets the upgrade case where that + // directory may not previously exist. + helpers.ExecCmdOnNode(t, cs, node, "rm", "-rf", filepath.Join("/rootfs", filepath.Dir(sshPaths.Expected))) + // Adding authorized key for user core testIgnConfig := ctrlcommon.NewIgnConfig() - testSSHKey := ign3types.PasswdUser{Name: "core", SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"test adding authorized key without node reboot"}} + testSSHKey := ign3types.PasswdUser{Name: "core", SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{ign3types.SSHAuthorizedKey(sshKeyContent)}} testIgnConfig.Passwd.Users = append(testIgnConfig.Passwd.Users, testSSHKey) addAuthorizedKey := &mcfgv1.MachineConfig{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("authorzied-key-%s", uuid.NewUUID()), + Name: fmt.Sprintf("authorized-key-%s", uuid.NewUUID()), Labels: helpers.MCLabelForRole("master"), }, Spec: mcfgv1.MachineConfigSpec{ @@ -292,8 +314,13 @@ func TestNoReboot(t *testing.T) { assert.Equal(t, node.Annotations[constants.CurrentMachineConfigAnnotationKey], renderedConfig) assert.Equal(t, node.Annotations[constants.MachineConfigDaemonStateAnnotationKey], constants.MachineConfigDaemonStateDone) - foundSSHKey := helpers.ExecCmdOnNode(t, cs, node, "cat", "/rootfs/home/core/.ssh/authorized_keys") - if !strings.Contains(foundSSHKey, "test adding authorized key without node reboot") { + t.Logf("Expecting SSH keys to be in %s", sshPaths.Expected) + + helpers.AssertFileOnNode(t, cs, node, sshPaths.Expected) + helpers.AssertFileNotOnNode(t, cs, node, sshPaths.NotExpected) + + foundSSHKey := helpers.ExecCmdOnNode(t, cs, node, "cat", filepath.Join("/rootfs", sshPaths.Expected)) + if !strings.Contains(foundSSHKey, sshKeyContent) { t.Fatalf("updated ssh keys not found in authorized_keys, got %s", foundSSHKey) } t.Logf("Node %s has SSH key", node.Name) @@ -332,11 +359,14 @@ func TestNoReboot(t *testing.T) { assert.Equal(t, node.Annotations[constants.CurrentMachineConfigAnnotationKey], oldMasterRenderedConfig) assert.Equal(t, node.Annotations[constants.MachineConfigDaemonStateAnnotationKey], constants.MachineConfigDaemonStateDone) - foundSSHKey = helpers.ExecCmdOnNode(t, cs, node, "cat", "/rootfs/home/core/.ssh/authorized_keys") - if strings.Contains(foundSSHKey, "test adding authorized key without node reboot") { + foundSSHKey = helpers.ExecCmdOnNode(t, cs, node, "cat", filepath.Join("/rootfs", sshPaths.Expected)) + if strings.Contains(foundSSHKey, sshKeyContent) { t.Fatalf("Node %s did not rollback successfully", node.Name) } + helpers.AssertFileOnNode(t, cs, node, sshPaths.Expected) + helpers.AssertFileNotOnNode(t, cs, node, sshPaths.NotExpected) + t.Logf("Node %s has successfully rolled back", node.Name) // Ensure that node didn't reboot during rollback diff --git a/test/e2e/mcd_test.go b/test/e2e/mcd_test.go index 97580a106c..088c77031a 100644 --- a/test/e2e/mcd_test.go +++ b/test/e2e/mcd_test.go @@ -3,6 +3,7 @@ package e2e_test import ( "context" "fmt" + "path/filepath" "strconv" "strings" "testing" @@ -383,18 +384,38 @@ func TestNoReboot(t *testing.T) { infraNode := helpers.GetSingleNodeByRole(t, cs, "infra") + sshKeyContent := "test adding authorized key without node reboot" + + nodeOS := helpers.GetOSReleaseForNode(t, cs, infraNode).OS + + sshPaths := helpers.GetSSHPaths(nodeOS) + + t.Logf("Expecting SSH keys to be in %s", sshPaths.Expected) + + if sshPaths.Expected == constants.RHCOS9SSHKeyPath { + // Write an SSH key to the old location on the node because the update process should remove this file. + t.Logf("Writing SSH key to %s to ensure that it will be removed later", sshPaths.NotExpected) + bashCmd := fmt.Sprintf("printf '%s' > %s", sshKeyContent, filepath.Join("/rootfs", sshPaths.NotExpected)) + helpers.ExecCmdOnNode(t, cs, infraNode, "/bin/bash", "-c", bashCmd) + } + + // Delete the expected SSH keys directory to ensure that the directories are + // (re)created correctly by the MCD. This targets the upgrade case where that + // directory may not previously exist. + helpers.ExecCmdOnNode(t, cs, infraNode, "rm", "-rf", filepath.Join("/rootfs", filepath.Dir(sshPaths.Expected))) + output := helpers.ExecCmdOnNode(t, cs, infraNode, "cat", "/rootfs/proc/uptime") oldTime := strings.Split(output, " ")[0] t.Logf("Node %s initial uptime: %s", infraNode.Name, oldTime) // Adding authorized key for user core testIgnConfig := ctrlcommon.NewIgnConfig() - testSSHKey := ign3types.PasswdUser{Name: "core", SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"test adding authorized key without node reboot"}} + testSSHKey := ign3types.PasswdUser{Name: "core", SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{ign3types.SSHAuthorizedKey(sshKeyContent)}} testIgnConfig.Passwd.Users = append(testIgnConfig.Passwd.Users, testSSHKey) addAuthorizedKey := &mcfgv1.MachineConfig{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("authorzied-key-infra-%s", uuid.NewUUID()), + Name: fmt.Sprintf("authorized-key-infra-%s", uuid.NewUUID()), Labels: helpers.MCLabelForRole("infra"), }, Spec: mcfgv1.MachineConfigSpec{ @@ -420,13 +441,16 @@ func TestNoReboot(t *testing.T) { assert.Equal(t, infraNode.Annotations[constants.CurrentMachineConfigAnnotationKey], renderedConfig) assert.Equal(t, infraNode.Annotations[constants.MachineConfigDaemonStateAnnotationKey], constants.MachineConfigDaemonStateDone) - foundSSHKey := helpers.ExecCmdOnNode(t, cs, infraNode, "cat", "/rootfs/home/core/.ssh/authorized_keys") - if !strings.Contains(foundSSHKey, "test adding authorized key without node reboot") { + helpers.AssertFileOnNode(t, cs, infraNode, sshPaths.Expected) + helpers.AssertFileNotOnNode(t, cs, infraNode, sshPaths.NotExpected) + + foundSSHKey := helpers.ExecCmdOnNode(t, cs, infraNode, "cat", filepath.Join("/rootfs", sshPaths.Expected)) + if !strings.Contains(foundSSHKey, sshKeyContent) { t.Fatalf("updated ssh keys not found in authorized_keys, got %s", foundSSHKey) } t.Logf("Node %s has SSH key", infraNode.Name) - usernameAndGroup := strings.Split(strings.TrimSuffix(helpers.ExecCmdOnNode(t, cs, infraNode, "chroot", "/rootfs", "stat", "--format=%U %G", "/home/core/.ssh/authorized_keys"), "\n"), " ") + usernameAndGroup := strings.Split(strings.TrimSuffix(helpers.ExecCmdOnNode(t, cs, infraNode, "chroot", "/rootfs", "stat", "--format=%U %G", sshPaths.Expected), "\n"), " ") assert.Equal(t, usernameAndGroup, []string{constants.CoreUserName, constants.CoreGroupName}) output = helpers.ExecCmdOnNode(t, cs, infraNode, "cat", "/rootfs/proc/uptime") @@ -463,11 +487,14 @@ func TestNoReboot(t *testing.T) { assert.Equal(t, infraNode.Annotations[constants.CurrentMachineConfigAnnotationKey], oldInfraRenderedConfig) assert.Equal(t, infraNode.Annotations[constants.MachineConfigDaemonStateAnnotationKey], constants.MachineConfigDaemonStateDone) - foundSSHKey = helpers.ExecCmdOnNode(t, cs, infraNode, "cat", "/rootfs/home/core/.ssh/authorized_keys") - if strings.Contains(foundSSHKey, "test adding authorized key without node reboot") { + foundSSHKey = helpers.ExecCmdOnNode(t, cs, infraNode, "cat", filepath.Join("/rootfs", sshPaths.Expected)) + if strings.Contains(foundSSHKey, sshKeyContent) { t.Fatalf("Node %s did not rollback successfully", infraNode.Name) } + helpers.AssertFileOnNode(t, cs, infraNode, sshPaths.Expected) + helpers.AssertFileNotOnNode(t, cs, infraNode, sshPaths.NotExpected) + t.Logf("Node %s has successfully rolled back", infraNode.Name) // Ensure that node didn't reboot during rollback diff --git a/test/helpers/utils.go b/test/helpers/utils.go index f749cf4ec7..eb9814cff0 100644 --- a/test/helpers/utils.go +++ b/test/helpers/utils.go @@ -31,6 +31,7 @@ import ( "k8s.io/client-go/util/retry" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" ) @@ -382,6 +383,28 @@ func CreateMCP(t *testing.T, cs *framework.ClientSet, mcpName string) func() { } } +type SSHPaths struct { + // The path where SSH keys are expected to be found. + Expected string + // The path where SSH keys are *not* expected to be found. + NotExpected string +} + +// Determines where to expect SSH keys for the core user on a given node based upon the node's OS. +func GetSSHPaths(os osrelease.OperatingSystem) SSHPaths { + if os.IsEL9() || os.IsSCOS() || os.IsFCOS() { + return SSHPaths{ + Expected: constants.RHCOS9SSHKeyPath, + NotExpected: constants.RHCOS8SSHKeyPath, + } + } + + return SSHPaths{ + Expected: constants.RHCOS8SSHKeyPath, + NotExpected: constants.RHCOS9SSHKeyPath, + } +} + // MCPNameToRole converts a mcpName to a node role label func MCPNameToRole(mcpName string) string { return fmt.Sprintf("node-role.kubernetes.io/%s", mcpName) @@ -392,23 +415,87 @@ func CreateMC(name, role string) *mcfgv1.MachineConfig { return NewMachineConfig(name, MCLabelForRole(role), "", nil) } +// Asserts that a given file is present on the underlying node. +func AssertFileOnNode(t *testing.T, cs *framework.ClientSet, node corev1.Node, path string) bool { + t.Helper() + + path = CanonicalizeNodeFilePath(path) + + out, err := ExecCmdOnNodeWithError(cs, node, "stat", path) + + return assert.NoError(t, err, "expected to find file %s on %s, got:\n%s\nError: %s", path, node.Name, out, err) +} + +// Asserts that a given file is *not* present on the underlying node. +func AssertFileNotOnNode(t *testing.T, cs *framework.ClientSet, node corev1.Node, path string) bool { + t.Helper() + + path = CanonicalizeNodeFilePath(path) + + out, err := ExecCmdOnNodeWithError(cs, node, "stat", path) + + return assert.Error(t, err, "expected not to find file %s on %s, got:\n%s", path, node.Name, out) && + assert.Contains(t, out, "No such file or directory", "expected command output to contain 'No such file or directory', got: %s", out) +} + +// Adds the /rootfs onto a given file path, if not already present. +func CanonicalizeNodeFilePath(path string) string { + rootfs := "/rootfs" + + if !strings.HasPrefix(path, rootfs) { + return filepath.Join(rootfs, path) + } + + return path +} + // ExecCmdOnNode finds a node's mcd, and oc rsh's into it to execute a command on the node // all commands should use /rootfs as root func ExecCmdOnNode(t *testing.T, cs *framework.ClientSet, node corev1.Node, subArgs ...string) string { + t.Helper() + + cmd, err := execCmdOnNode(cs, node, subArgs...) + require.Nil(t, err, "could not prepare to exec cmd %v on node %s: %s", subArgs, node.Name, err) + cmd.Stderr = os.Stderr + + out, err := cmd.Output() + require.Nil(t, err, "failed to exec cmd %v on node %s: %s", subArgs, node.Name, string(out)) + return string(out) +} + +// ExecCmdOnNodeWithError behaves like ExecCmdOnNode, with the exception that +// any errors are returned to the caller for inspection. This allows one to +// execute a command that is expected to fail; e.g., stat /nonexistant/file. +func ExecCmdOnNodeWithError(cs *framework.ClientSet, node corev1.Node, subArgs ...string) (string, error) { + cmd, err := execCmdOnNode(cs, node, subArgs...) + if err != nil { + return "", err + } + + out, err := cmd.CombinedOutput() + return string(out), err +} + +// ExecCmdOnNode finds a node's mcd, and oc rsh's into it to execute a command on the node +// all commands should use /rootfs as root +func execCmdOnNode(cs *framework.ClientSet, node corev1.Node, subArgs ...string) (*exec.Cmd, error) { // Check for an oc binary in $PATH. path, err := exec.LookPath("oc") if err != nil { - t.Fatalf("could not locate oc command: %s", err) + return nil, fmt.Errorf("could not locate oc command: %w", err) } // Get the kubeconfig file path kubeconfig, err := cs.GetKubeconfig() if err != nil { - t.Fatalf("could not get kubeconfig: %s", err) + return nil, fmt.Errorf("could not get kubeconfig: %w", err) } mcd, err := mcdForNode(cs, &node) - require.Nil(t, err) + if err != nil { + return nil, fmt.Errorf("could not get MCD for node %s: %w", node.Name, err) + } + mcdName := mcd.ObjectMeta.Name entryPoint := path @@ -423,23 +510,19 @@ func ExecCmdOnNode(t *testing.T, cs *framework.ClientSet, node corev1.Node, subA // $KUBECONFIG, oc will be unaware of it. To remedy, we explicitly set // KUBECONFIG to the value held by the clientset. cmd.Env = append(cmd.Env, "KUBECONFIG="+kubeconfig) - cmd.Stderr = os.Stderr - - out, err := cmd.Output() - require.Nil(t, err, "failed to exec cmd %v on node %s: %s", subArgs, node.Name, string(out)) - return string(out) + return cmd, nil } -// IsOKDCluster checks whether the Upstream field on the CV spec references OKD's update server +// IsOKDCluster checks whether the Upstream field on the CV spec references an OKD release controller func IsOKDCluster(cs *framework.ClientSet) (bool, error) { cv, err := cs.ClusterVersions().Get(context.TODO(), "version", metav1.GetOptions{}) if err != nil { return false, err } - if cv.Spec.Upstream == "https://origin-release.svc.ci.openshift.org/graph" { - return true, nil - } - return false, nil + + // TODO: Adjust this as OKD becomes available for different platforms, e.g., arm64. + okdReleaseControllers := sets.NewString("https://amd64.origin.releases.ci.openshift.org/graph") + return okdReleaseControllers.Has(string(cv.Spec.Upstream)), nil } func MCLabelForRole(role string) map[string]string {