diff --git a/pkg/daemon/osrelease.go b/pkg/daemon/osrelease.go index fc42c075e8..205ff7bcd9 100644 --- a/pkg/daemon/osrelease.go +++ b/pkg/daemon/osrelease.go @@ -17,14 +17,25 @@ type OperatingSystem struct { VersionID string } +var FCOS = OperatingSystem{ + ID: "fedora", + VariantID: "coreos", +} + +var RHCOS = OperatingSystem{ + ID: "rhcos", + // per https://github.com/openshift/os/commit/31f295e3362a6622749a64a6ff610b727560bda1 + // there's no VARIANT_ID and confirmed by looking at /etc/os-release +} + // IsRHCOS is true if the OS is RHEL CoreOS func (os OperatingSystem) IsRHCOS() bool { - return os.ID == "rhcos" + return os.ID == RHCOS.ID } // IsFCOS is true if the OS is RHEL CoreOS func (os OperatingSystem) IsFCOS() bool { - return os.ID == "fedora" && os.VariantID == "coreos" + return os.ID == FCOS.ID && os.VariantID == FCOS.VariantID } // IsCoreOSVariant is true if the OS is FCOS or a derivative (ostree+Ignition) diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index cccbfe0c2b..18b0511518 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -61,8 +61,11 @@ const ( ) var ( - origParentDirPath = filepath.Join("/etc", "machine-config-daemon", "orig") - noOrigParentDirPath = filepath.Join("/etc", "machine-config-daemon", "noorig") + origParentDirPath = filepath.Join("/etc", "machine-config-daemon", "orig") + noOrigParentDirPath = filepath.Join("/etc", "machine-config-daemon", "noorig") + authKeyFragmentDirPath = filepath.Join(coreUserSSHPath, "authorized_keys.d") + fcosAuthKeyPath = filepath.Join(authKeyFragmentDirPath, "ignition") + nonFCOSAuthKeyPath = filepath.Join(coreUserSSHPath, "authorized_keys") ) func writeFileAtomicallyWithDefaults(fpath string, b []byte) error { @@ -427,50 +430,58 @@ func (dn *CoreOSDaemon) applyOSChanges(mcDiff machineConfigDiff, oldConfig, newC } -func calculatePostConfigChangeActionFromFileDiffs(diffFileSet []string) (actions []string) { +func (dn *Daemon) calculatePostConfigChangeActionFromFiles(diffFileSet []string) ([]string, error) { + // If a machine-config-daemon-force file is present, it means the user wants to + // move to desired state without additional validation. We will reboot the node in + // this case regardless of what MachineConfig diff is. + if _, err := os.Stat(constants.MachineConfigDaemonForceFile); err == nil { + if err := os.Remove(constants.MachineConfigDaemonForceFile); err != nil { + return []string{}, errors.Wrap(err, "failed to remove force validation file") + } + glog.Infof("Setting post config change action to postConfigChangeActionReboot; %s present", constants.MachineConfigDaemonForceFile) + return []string{postConfigChangeActionReboot}, nil + } + filesPostConfigChangeActionNone := []string{ "/etc/kubernetes/kubelet-ca.crt", "/var/lib/kubelet/config.json", } + if dn.os.IsFCOS() { + filesPostConfigChangeActionNone = append(filesPostConfigChangeActionNone, fcosAuthKeyPath) + } else { + filesPostConfigChangeActionNone = append(filesPostConfigChangeActionNone, nonFCOSAuthKeyPath) + } + filesPostConfigChangeActionReloadCrio := []string{ constants.ContainerRegistryConfPath, GPGNoRebootPath, "/etc/containers/policy.json", } - actions = []string{postConfigChangeActionNone} + actions := []string{postConfigChangeActionNone} for _, path := range diffFileSet { if ctrlcommon.InSlice(path, filesPostConfigChangeActionNone) { continue } else if ctrlcommon.InSlice(path, filesPostConfigChangeActionReloadCrio) { actions = []string{postConfigChangeActionReloadCrio} } else { - actions = []string{postConfigChangeActionReboot} - return + return []string{postConfigChangeActionReboot}, nil } } - return + return actions, nil } -func calculatePostConfigChangeAction(diff *machineConfigDiff, diffFileSet []string) ([]string, error) { - // If a machine-config-daemon-force file is present, it means the user wants to - // move to desired state without additional validation. We will reboot the node in - // this case regardless of what MachineConfig diff is. - if _, err := os.Stat(constants.MachineConfigDaemonForceFile); err == nil { - if err := os.Remove(constants.MachineConfigDaemonForceFile); err != nil { - return []string{}, errors.Wrap(err, "failed to remove force validation file") - } - glog.Infof("Setting post config change action to postConfigChangeActionReboot; %s present", constants.MachineConfigDaemonForceFile) - return []string{postConfigChangeActionReboot}, nil - } +func (dn *Daemon) calculatePostConfigChangeActionWithMCDiff(diff *machineConfigDiff, diffFileSet []string) ([]string, error) { + // Note this function may only return []string{postConfigChangeActionReboot} directly, + // since calculatePostConfigChangeActionFromFiles may find files that require a reboot + // We don't actually have to consider ssh keys changes, which is the only section of passwd that is allowed to change if diff.osUpdate || diff.kargs || diff.fips || diff.units || diff.kernelType || diff.extensions { // must reboot return []string{postConfigChangeActionReboot}, nil } - // We don't actually have to consider ssh keys changes, which is the only section of passwd that is allowed to change - return calculatePostConfigChangeActionFromFileDiffs(diffFileSet), nil + return dn.calculatePostConfigChangeActionFromFiles(diffFileSet) } // update the node to the provided node configuration. @@ -530,7 +541,7 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig) (retErr err dn.logSystem("Starting update from %s to %s: %+v", oldConfigName, newConfigName, diff) diffFileSet := ctrlcommon.CalculateConfigFileDiffs(&oldIgnConfig, &newIgnConfig) - actions, err := calculatePostConfigChangeAction(diff, diffFileSet) + actions, err := dn.calculatePostConfigChangeActionWithMCDiff(diff, diffFileSet) if err != nil { return err } @@ -1671,9 +1682,9 @@ func (dn *Daemon) atomicallyWriteSSHKey(keys string) error { var authKeyPath string if dn.os.IsFCOS() { - authKeyPath = filepath.Join(coreUserSSHPath, "authorized_keys.d", "ignition") + authKeyPath = fcosAuthKeyPath } else { - authKeyPath = filepath.Join(coreUserSSHPath, "authorized_keys") + authKeyPath = nonFCOSAuthKeyPath } // Keys should only be written to "/home/core/.ssh" @@ -1715,24 +1726,21 @@ func (dn *Daemon) updateSSHKeys(newUsers []ign3types.PasswdUser) error { } } 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) + _, err := os.Stat(nonFCOSAuthKeyPath) if err == nil { - err := os.RemoveAll(authKeyPath) + err := os.RemoveAll(nonFCOSAuthKeyPath) if err != nil { - return fmt.Errorf("failed to remove path '%s': %v", authKeyPath, err) + return fmt.Errorf("failed to remove path '%s': %v", nonFCOSAuthKeyPath, err) } } else if !os.IsNotExist(err) { // This shouldn't ever happen - return fmt.Errorf("unexpectedly failed to get info for path '%s': %v", authKeyPath, err) + return fmt.Errorf("unexpectedly failed to get info for path '%s': %v", nonFCOSAuthKeyPath, err) } // Ensure authorized_keys.d/ignition is the only fragment that exists diff --git a/pkg/daemon/update_test.go b/pkg/daemon/update_test.go index 82ac65931c..88119e03e2 100644 --- a/pkg/daemon/update_test.go +++ b/pkg/daemon/update_test.go @@ -164,8 +164,8 @@ func TestReconcilable(t *testing.T) { // Verify Raid changes react as expected oldIgnCfg.Storage.Raid = []ign3types.Raid{ { - Name: "data", - Level: "stripe", + Name: "data", + Level: "stripe", Devices: []ign3types.Device{"/dev/vda", "/dev/vdb"}, }, } @@ -579,97 +579,140 @@ func TestDropinCheck(t *testing.T) { // i.e. whether we need to reboot and what actions need to be taken if no reboot is needed func TestCalculatePostConfigChangeAction(t *testing.T) { files := map[string]ign3types.File{ - "pullsecret1": helpers.NewIgnFile("/var/lib/kubelet/config.json", "kubelet conf 1\n"), - "pullsecret2": helpers.NewIgnFile("/var/lib/kubelet/config.json", "kubelet conf 2\n"), - "registries1": helpers.NewIgnFile("/etc/containers/registries.conf", "registries content 1\n"), - "registries2": helpers.NewIgnFile("/etc/containers/registries.conf", "registries content 2\n"), - "randomfile1": helpers.NewIgnFile("/etc/random-reboot-file", "test\n"), - "randomfile2": helpers.NewIgnFile("/etc/random-reboot-file", "test 2\n"), - "kubeletCA1": helpers.NewIgnFile("/etc/kubernetes/kubelet-ca.crt", "kubeletCA1\n"), - "kubeletCA2": helpers.NewIgnFile("/etc/kubernetes/kubelet-ca.crt", "kubeletCA2\n"), - "policy1": helpers.NewIgnFile("/etc/containers/policy.json", "policy1"), - "policy2": helpers.NewIgnFile("/etc/containers/policy.json", "policy2"), - "containers-gpg1": helpers.NewIgnFile("/etc/machine-config-daemon/no-reboot/containers-gpg.pub", "containers-gpg1"), - "containers-gpg2": helpers.NewIgnFile("/etc/machine-config-daemon/no-reboot/containers-gpg.pub", "containers-gpg2"), + "pullsecret1": helpers.NewIgnFile("/var/lib/kubelet/config.json", "kubelet conf 1\n"), + "pullsecret2": helpers.NewIgnFile("/var/lib/kubelet/config.json", "kubelet conf 2\n"), + "registries1": helpers.NewIgnFile("/etc/containers/registries.conf", "registries content 1\n"), + "registries2": helpers.NewIgnFile("/etc/containers/registries.conf", "registries content 2\n"), + "randomfile1": helpers.NewIgnFile("/etc/random-reboot-file", "test\n"), + "randomfile2": helpers.NewIgnFile("/etc/random-reboot-file", "test 2\n"), + "kubeletCA1": helpers.NewIgnFile("/etc/kubernetes/kubelet-ca.crt", "kubeletCA1\n"), + "kubeletCA2": helpers.NewIgnFile("/etc/kubernetes/kubelet-ca.crt", "kubeletCA2\n"), + "policy1": helpers.NewIgnFile("/etc/containers/policy.json", "policy1"), + "policy2": helpers.NewIgnFile("/etc/containers/policy.json", "policy2"), + "containers-gpg1": helpers.NewIgnFile("/etc/machine-config-daemon/no-reboot/containers-gpg.pub", "containers-gpg1"), + "containers-gpg2": helpers.NewIgnFile("/etc/machine-config-daemon/no-reboot/containers-gpg.pub", "containers-gpg2"), + "fcos_authorized_keys": helpers.NewIgnFile(fcosAuthKeyPath, "authorized_keys"), + "non_fcos_authorized_keys": helpers.NewIgnFile(nonFCOSAuthKeyPath, "authorized_keys"), } tests := []struct { oldConfig *mcfgv1.MachineConfig newConfig *mcfgv1.MachineConfig + os OperatingSystem expectedAction []string }{ { // test that a normal file change is reboot oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{files["randomfile1"]}), newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["randomfile2"]}), + os: FCOS, expectedAction: []string{postConfigChangeActionReboot}, }, { // test that a pull secret change is none oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{files["pullsecret1"]}), newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["pullsecret2"]}), + os: FCOS, expectedAction: []string{postConfigChangeActionNone}, }, { // test that a SSH key change is none oldConfig: helpers.NewMachineConfigExtended("00-test", nil, []ign3types.File{}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key1"}, []string{}, false, []string{}, "default", "dummy://"), newConfig: helpers.NewMachineConfigExtended("01-test", nil, []ign3types.File{}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key2"}, []string{}, false, []string{}, "default", "dummy://"), + os: FCOS, expectedAction: []string{postConfigChangeActionNone}, }, { // test that a registries change is reload oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{files["registries1"]}), newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["registries2"]}), + os: FCOS, expectedAction: []string{postConfigChangeActionReloadCrio}, }, { // test that a kubelet CA change is none oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{files["kubeletCA1"]}), newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["kubeletCA2"]}), + os: FCOS, expectedAction: []string{postConfigChangeActionNone}, }, { // test that a registries change (reload) overwrites pull secret (none) oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{files["registries1"], files["pullsecret1"]}), newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["registries2"], files["pullsecret2"]}), + os: FCOS, expectedAction: []string{postConfigChangeActionReloadCrio}, }, { // test that a osImage change (reboot) overwrites registries (reload) and SSH keys (none) oldConfig: helpers.NewMachineConfigExtended("00-test", nil, []ign3types.File{files["registries1"]}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key1"}, []string{}, false, []string{}, "default", "dummy://"), newConfig: helpers.NewMachineConfigExtended("01-test", nil, []ign3types.File{files["registries2"]}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key2"}, []string{}, false, []string{}, "default", "dummy1://"), + os: FCOS, expectedAction: []string{postConfigChangeActionReboot}, }, { // test that adding a pull secret is none oldConfig: helpers.NewMachineConfigExtended("00-test", nil, []ign3types.File{files["registries1"]}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key1"}, []string{}, false, []string{}, "default", "dummy://"), newConfig: helpers.NewMachineConfigExtended("01-test", nil, []ign3types.File{files["registries1"], files["pullsecret2"]}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key1"}, []string{}, false, []string{}, "default", "dummy://"), + os: FCOS, expectedAction: []string{postConfigChangeActionNone}, }, { // test that removing a registries is crio reload oldConfig: helpers.NewMachineConfigExtended("00-test", nil, []ign3types.File{files["randomfile1"], files["registries1"]}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key1"}, []string{}, false, []string{}, "default", "dummy://"), newConfig: helpers.NewMachineConfigExtended("01-test", nil, []ign3types.File{files["randomfile1"]}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key1"}, []string{}, false, []string{}, "default", "dummy://"), + os: FCOS, expectedAction: []string{postConfigChangeActionReloadCrio}, }, { // mixed test - final should be reboot due to kargs changes oldConfig: helpers.NewMachineConfigExtended("00-test", nil, []ign3types.File{files["registries1"]}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key1"}, []string{}, false, []string{}, "default", "dummy://"), newConfig: helpers.NewMachineConfigExtended("01-test", nil, []ign3types.File{files["pullsecret2"], files["kubeletCA1"]}, []ign3types.Unit{}, []ign3types.SSHAuthorizedKey{"key2"}, []string{}, false, []string{"karg1"}, "default", "dummy://"), + os: FCOS, expectedAction: []string{postConfigChangeActionReboot}, }, { // test that updating policy.json is crio reload oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{files["policy1"]}), newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["policy2"]}), + os: FCOS, expectedAction: []string{postConfigChangeActionReloadCrio}, }, { // test that updating containers-gpg.pub is crio reload oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{files["containers-gpg1"]}), newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["containers-gpg2"]}), + os: FCOS, expectedAction: []string{postConfigChangeActionReloadCrio}, }, + { + // test that on FCOS authorized_keys are ignored + oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{}), + newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["fcos_authorized_keys"]}), + os: FCOS, + expectedAction: []string{postConfigChangeActionNone}, + }, + { + // test that on FCOS authorized_keys for RHEL are not ignored + oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{}), + newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["non_fcos_authorized_keys"]}), + os: FCOS, + expectedAction: []string{postConfigChangeActionReboot}, + }, + { + // test that on non-FCOS authorized_keys are ignored + oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{}), + newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["non_fcos_authorized_keys"]}), + os: RHCOS, + expectedAction: []string{postConfigChangeActionNone}, + }, + { + // test that on non-FCOS authorized_keys for FCOS are not ignored + oldConfig: helpers.NewMachineConfig("00-test", nil, "dummy://", []ign3types.File{}), + newConfig: helpers.NewMachineConfig("01-test", nil, "dummy://", []ign3types.File{files["fcos_authorized_keys"]}), + os: RHCOS, + expectedAction: []string{postConfigChangeActionReboot}, + }, } for idx, test := range tests { @@ -687,7 +730,10 @@ func TestCalculatePostConfigChangeAction(t *testing.T) { t.Errorf("error creating machineConfigDiff: %v", err) } diffFileSet := ctrlcommon.CalculateConfigFileDiffs(&oldIgnConfig, &newIgnConfig) - calculatedAction, err := calculatePostConfigChangeAction(mcDiff, diffFileSet) + dn := Daemon{ + os: test.os, + } + calculatedAction, err := dn.calculatePostConfigChangeActionWithMCDiff(mcDiff, diffFileSet) if !reflect.DeepEqual(test.expectedAction, calculatedAction) { t.Errorf("Failed calculating config change action: expected: %v but result is: %v. Error: %v", test.expectedAction, calculatedAction, err)