Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
57 changes: 57 additions & 0 deletions pkg/controller/common/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -658,3 +658,60 @@ func dedupePasswdUserSSHKeys(passwdUser ign2types.PasswdUser) ign2types.PasswdUs

return passwdUser
}

// CalculateConfigFileDiffs compares the files present in two ignition configurations and returns the list of files
// that are different between them
func CalculateConfigFileDiffs(oldIgnConfig, newIgnConfig *ign3types.Config) []string {
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about returning a struct type with the diff that distinguishes add/remove/modify?

This is like https://github.com/coreos/bootupd/blob/499e2af364dee4bbc9811c62ddfd001b4686cd9d/src/filetree.rs#L46

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need that info somewhere in MCO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could use it for isDrainRequired (see #2851)

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be at least useful to log messages by caller instead of doing it here,in the context of https://github.com/openshift/machine-config-operator/pull/2870/files#r768161851

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jkyros wdyt? Time to pull out the howitzer or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think given this is needed for #2851 which I think we want to get in before Friday, it might be better to fix this later if we decide it's worth it

Copy link
Member

Choose a reason for hiding this comment

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

I think we end up having to someday, but that day doesn't necessarily have to be today if there are timeline concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure, let's create a card for this in Jira so that someone pick this up later on.

// Go through the files and see what is new or different
oldFileSet := make(map[string]ign3types.File)
for _, f := range oldIgnConfig.Storage.Files {
oldFileSet[f.Path] = f
}
newFileSet := make(map[string]ign3types.File)
for _, f := range newIgnConfig.Storage.Files {
newFileSet[f.Path] = f
}
diffFileSet := []string{}

// First check if any files were removed
for path := range oldFileSet {
_, ok := newFileSet[path]
if !ok {
// debug: remove
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sinnykumari @jkyros I think the conclusion from the thread on John's PR was to leave the debug statements in. Are we wanting to keep or remove the comment?

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd vote for having the caller log the diff as a whole, which could be done more easily with the above change of returning a struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Returning struct with diff would be much better.

glog.Infof("File diff: %v was deleted", path)
diffFileSet = append(diffFileSet, path)
}
}

// Now check if any files were added/changed
for path, newFile := range newFileSet {
oldFile, ok := oldFileSet[path]
if !ok {
// debug: remove
glog.Infof("File diff: %v was added", path)
diffFileSet = append(diffFileSet, path)
} else if !reflect.DeepEqual(oldFile, newFile) {
// debug: remove
glog.Infof("File diff: detected change to %v", newFile.Path)
diffFileSet = append(diffFileSet, path)
}
}
return diffFileSet
}

// GetIgnitionFileDataByPath retrieves the file data for a specified path from a given ignition config
func GetIgnitionFileDataByPath(config *ign3types.Config, path string) ([]byte, error) {
for _, f := range config.Storage.Files {
if path == f.Path {
// Convert whatever we have to the actual bytes so we can inspect them
if f.Contents.Source != nil {
contents, err := dataurl.DecodeString(*f.Contents.Source)
if err != nil {
return nil, err
}
return contents.Data, err
}
}
}
return nil, nil
}
30 changes: 30 additions & 0 deletions pkg/controller/common/helpers_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package common

import (
"reflect"
"testing"

"github.com/clarketm/json"
Expand Down Expand Up @@ -380,3 +381,32 @@ func TestRemoveIgnDuplicateFilesAndUnits(t *testing.T) {

assert.Equal(t, expectedIgn2Config, convertedIgn2Config)
}

func TestCalculateConfigFileDiffs(t *testing.T) {
var testIgn3ConfigOld ign3types.Config
var testIgn3ConfigNew ign3types.Config

oldTempFile := helpers.NewIgnFile("/etc/kubernetes/kubelet-ca.crt", "oldcertificates")
newTempFile := helpers.NewIgnFile("/etc/kubernetes/kubelet-ca.crt", "newcertificates")

// Make an "old" config with the existing file in it
testIgn3ConfigOld.Ignition.Version = "3.2.0"
testIgn3ConfigOld.Storage.Files = append(testIgn3ConfigOld.Storage.Files, oldTempFile)

// Make a "new" config with a change to that file
testIgn3ConfigNew.Ignition.Version = "3.2.0"
testIgn3ConfigNew.Storage.Files = append(testIgn3ConfigNew.Storage.Files, newTempFile)

// If it works, it should notice the file changed
expectedDiffFileSet := []string{"/etc/kubernetes/kubelet-ca.crt"}
actualDiffFileSet := CalculateConfigFileDiffs(&testIgn3ConfigOld, &testIgn3ConfigNew)
unchangedDiffFileset := CalculateConfigFileDiffs(&testIgn3ConfigOld, &testIgn3ConfigOld)

if !reflect.DeepEqual(expectedDiffFileSet, actualDiffFileSet) {
t.Errorf("Actual file diff: %s did not match expected: %s", actualDiffFileSet, expectedDiffFileSet)
}

if !reflect.DeepEqual(unchangedDiffFileset, []string{}) {
t.Errorf("File changes detected where there should have been none: %s", unchangedDiffFileset)
}
}
37 changes: 12 additions & 25 deletions pkg/daemon/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -582,35 +582,22 @@ func TestDropinCheck(t *testing.T) {
}
}

func newFile(path, contents string) ign3types.File {
return ign3types.File{
Node: ign3types.Node{
Path: path,
},
FileEmbedded1: ign3types.FileEmbedded1{
Contents: ign3types.Resource{
Source: helpers.StrToPtr(dataurl.EncodeBytes([]byte(contents))),
},
},
}
}

// 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) {
files := map[string]ign3types.File{
"pullsecret1": newFile("/var/lib/kubelet/config.json", "kubelet conf 1\n"),
"pullsecret2": newFile("/var/lib/kubelet/config.json", "kubelet conf 2\n"),
"registries1": newFile("/etc/containers/registries.conf", "registries content 1\n"),
"registries2": newFile("/etc/containers/registries.conf", "registries content 2\n"),
"randomfile1": newFile("/etc/random-reboot-file", "test\n"),
"randomfile2": newFile("/etc/random-reboot-file", "test 2\n"),
"kubeletCA1": newFile("/etc/kubernetes/kubelet-ca.crt", "kubeletCA1\n"),
"kubeletCA2": newFile("/etc/kubernetes/kubelet-ca.crt", "kubeletCA2\n"),
"policy1": newFile("/etc/containers/policy.json", "policy1"),
"policy2": newFile("/etc/containers/policy.json", "policy2"),
"containers-gpg1": newFile("/etc/machine-config-daemon/no-reboot/containers-gpg.pub", "containers-gpg1"),
"containers-gpg2": newFile("/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"),
}

tests := []struct {
Expand Down
14 changes: 14 additions & 0 deletions test/helpers/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/clarketm/json"
ign3types "github.com/coreos/ignition/v2/config/v3_2/types"
"github.com/vincent-petithory/dataurl"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -150,6 +151,19 @@ func NewMachineConfigPool(name string, mcSelector, nodeSelector *metav1.LabelSel
}
}

// NewIgnFile returns a simple ignition3 file from just path and file contents
func NewIgnFile(path, contents string) ign3types.File {
return ign3types.File{
Node: ign3types.Node{
Path: path,
},
FileEmbedded1: ign3types.FileEmbedded1{
Contents: ign3types.Resource{
Source: StrToPtr(dataurl.EncodeBytes([]byte(contents)))},
},
}
}

// CreateMachineConfigFromIgnition returns a MachineConfig object from an Ignition config passed to it
func CreateMachineConfigFromIgnition(ignCfg interface{}) *mcfgv1.MachineConfig {
return &mcfgv1.MachineConfig{
Expand Down