Skip to content

update.go: refactor calculatePostConfigChangeAction() #3022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 2 commits into from
Mar 30, 2022
Merged

update.go: refactor calculatePostConfigChangeAction() #3022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e5704b1
update.go: refactor calculatePostConfigChangeAction()
mkenigs Feb 25, 2022
657f3d0
test calculatePostConfigChangeAction for ssh keys
mkenigs Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions pkg/daemon/osrelease.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
68 changes: 38 additions & 30 deletions pkg/daemon/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Copy link
Member

@jkyros jkyros Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels maybe little weird having the forcefile check inside calculatePostConfigChangeActionFromFiles but its' no worse than what we had. Like, we factored out the other criteria so this function would suppoooosedly be making a decision just on this list of files provided...but then we also sneak out to the host's disk to check for a file (which means this function can't run as a 'hypothetical' against a list of files, it would need access to a host disk).

From a "find the largest unit of code that both layered and non-layered can use" standpoint this is good.

Cool for now, not worth holding this up for that, just something to think about for later.

Copy link
Contributor Author

@mkenigs mkenigs Mar 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good point

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.
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
76 changes: 61 additions & 15 deletions pkg/daemon/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
},
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand Down