From ebf6c4e8d565bdd2ba47b3e18fd7c2771ad8a7ab Mon Sep 17 00:00:00 2001 From: Zack Zlotnik Date: Tue, 14 Dec 2021 16:21:31 -0500 Subject: [PATCH] makes config drift monitor aware of compressed files The Config Drift Monitor (https://github.com/openshift/machine-config-operator/pull/2795) was previously unaware of compressed files. What would happen is the MCD would unzip a compressed file payload and write that to disk. However, the Config Drift Monitor was unaware that the file was compressed, so it was comparing the compressed contents of the MachineConfig against the uncompressed contents that were written to disk. Because of that, the Config Drift Monitor would erroneously degrade the node / MCP. Fixes: #2032565 --- pkg/daemon/config_drift_monitor_test.go | 70 ++++++++++++++++++++--- pkg/daemon/on_disk_validation.go | 19 +++--- test/e2e-shared-tests/mcd_config_drift.go | 55 ++++++++++++------ test/helpers/utils.go | 33 +++++++++++ 4 files changed, 137 insertions(+), 40 deletions(-) diff --git a/pkg/daemon/config_drift_monitor_test.go b/pkg/daemon/config_drift_monitor_test.go index 026893bfff..dbd9af345e 100644 --- a/pkg/daemon/config_drift_monitor_test.go +++ b/pkg/daemon/config_drift_monitor_test.go @@ -6,7 +6,6 @@ import ( "io/ioutil" "os" "path/filepath" - "strings" "sync" "testing" "time" @@ -101,6 +100,40 @@ func TestConfigDriftMonitor(t *testing.T) { expectedErr: fileErr, mutateFile: chmodFile, }, + // Compressed Ignition File + // These target the file called /etc/a-compressed-file defined by the test + // fixture. + // Targets a regression identified by: + // https://bugzilla.redhat.com/show_bug.cgi?id=2032565 + { + name: "ign compressed file content drift", + expectedErr: fileErr, + mutateCompressedFile: changeFileContent, + }, + { + name: "ign compressed file touch", + mutateCompressedFile: touchFile, + }, + { + name: "ign compressed file rename", + expectedErr: fileErr, + mutateCompressedFile: renameFile, + }, + { + name: "ign compressed file delete", + expectedErr: fileErr, + mutateCompressedFile: os.Remove, + }, + { + name: "ign compressed file overwrite", + expectedErr: fileErr, + mutateCompressedFile: overwriteFile, + }, + { + name: "ign compressed file chmod", + expectedErr: fileErr, + mutateCompressedFile: chmodFile, + }, // Systemd Unit tests // These target the systemd unit files in the test fixture: // /etc/systemd/system/unittest.service @@ -196,6 +229,8 @@ type configDriftMonitorTestCase struct { // Only one of these may be used per testcase: // The mutation to apply to the Ignition file mutateFile func(string) error + // The mutation to apply to the compressed Ignition file + mutateCompressedFile func(string) error // The mutation to apply to the systemd unit file mutateUnit func(string) error // The mutation to apply to the systemd dropin file @@ -314,14 +349,18 @@ func (tc configDriftMonitorTestCase) run(t *testing.T) { } // Creates the Ignition Config test fixture -func (tc configDriftMonitorTestCase) getIgnConfig() ign3types.Config { +func (tc configDriftMonitorTestCase) getIgnConfig(t *testing.T) ign3types.Config { + compressedFile, err := helpers.CreateGzippedIgn3File("/etc/a-compressed-file", "thefilecontents", int(defaultFilePermissions)) + require.Nil(t, err) + return ign3types.Config{ Ignition: ign3types.Ignition{ Version: ign3types.MaxVersion.String(), }, Storage: ign3types.Storage{ Files: []ign3types.File{ - helpers.CreateIgn3File("/etc/a-config-file", "data:,theafilecontents", int(defaultFilePermissions)), + helpers.CreateEncodedIgn3File("/etc/a-config-file", "thefilecontents", int(defaultFilePermissions)), + compressedFile, }, }, Systemd: ign3types.Systemd{ @@ -350,6 +389,10 @@ func (tc configDriftMonitorTestCase) mutate(ignConfig ign3types.Config) error { return tc.mutateFile(ignConfig.Storage.Files[0].Path) } + if tc.mutateCompressedFile != nil { + return tc.mutateCompressedFile(ignConfig.Storage.Files[1].Path) + } + if tc.mutateDropin != nil { dropinPath := getIgn3SystemdDropinPath(tc.systemdPath, ignConfig.Systemd.Units[0], ignConfig.Systemd.Units[0].Dropins[0]) return tc.mutateDropin(dropinPath) @@ -368,13 +411,13 @@ func (tc configDriftMonitorTestCase) mutate(ignConfig ign3types.Config) error { func (tc configDriftMonitorTestCase) getFixtures(t *testing.T) (ign3types.Config, *mcfgv1.MachineConfig) { t.Helper() - ignConfig := tc.getIgnConfig() + ignConfig := tc.getIgnConfig(t) - // Prefix all the ignition files with the temp directory and write there. + // Prefix all the ignition files with the temp directory and write to disk. for i, file := range ignConfig.Storage.Files { file.Path = filepath.Join(tc.tmpDir, file.Path) - tc.writeFileForTest(t, file.Path, file.Contents.Source) ignConfig.Storage.Files[i] = file + tc.writeIgn3FileForTest(t, file) } // Prefix all the systemd files with the temp dir and write them. @@ -418,13 +461,22 @@ func (tc configDriftMonitorTestCase) onDriftFunc(t *testing.T, err error) { } } +func (tc configDriftMonitorTestCase) writeIgn3FileForTest(t *testing.T, file ign3types.File) { + t.Helper() + + decodedContents, err := decodeContents(file.Contents.Source, file.Contents.Compression) + require.Nil(t, err) + + require.Nil(t, writeFileAtomicallyWithDefaults(file.Path, decodedContents)) +} + func (tc configDriftMonitorTestCase) writeFileForTest(t *testing.T, path string, contents *string) { t.Helper() - // writeFiles doesn't actually work like this. Writing files this way was - // done for the sake of simplicity. + out := "" if contents != nil { - out = strings.TrimPrefix(*contents, "data:,") + out = *contents } + require.Nil(t, writeFileAtomicallyWithDefaults(path, []byte(out))) } diff --git a/pkg/daemon/on_disk_validation.go b/pkg/daemon/on_disk_validation.go index 44c0c41e20..b4445c8b9b 100644 --- a/pkg/daemon/on_disk_validation.go +++ b/pkg/daemon/on_disk_validation.go @@ -15,7 +15,6 @@ import ( 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/pkg/errors" - "github.com/vincent-petithory/dataurl" ) // Validates that the on-disk state matches a given MachineConfig. @@ -151,15 +150,11 @@ func checkV3Files(files []ign3types.File) error { if f.Mode != nil { mode = os.FileMode(*f.Mode) } - contents := &dataurl.DataURL{} - if f.Contents.Source != nil { - var err error - contents, err = dataurl.DecodeString(*f.Contents.Source) - if err != nil { - return errors.Wrapf(err, "couldn't parse file %q", f.Path) - } + contents, err := decodeContents(f.Contents.Source, f.Contents.Compression) + if err != nil { + return fmt.Errorf("couldn't decode file %q: %w", f.Path, err) } - if err := checkFileContentsAndMode(f.Path, contents.Data, mode); err != nil { + if err := checkFileContentsAndMode(f.Path, contents, mode); err != nil { return err } } @@ -182,11 +177,11 @@ func checkV2Files(files []ign2types.File) error { if f.Mode != nil { mode = os.FileMode(*f.Mode) } - contents, err := dataurl.DecodeString(f.Contents.Source) + contents, err := decodeContents(&f.Contents.Source, &f.Contents.Compression) if err != nil { - return errors.Wrapf(err, "couldn't parse file %q", f.Path) + return fmt.Errorf("couldn't decode file %q: %w", f.Path, err) } - if err := checkFileContentsAndMode(f.Path, contents.Data, mode); err != nil { + if err := checkFileContentsAndMode(f.Path, contents, mode); err != nil { return err } checkedFiles[f.Path] = true diff --git a/test/e2e-shared-tests/mcd_config_drift.go b/test/e2e-shared-tests/mcd_config_drift.go index dde473eab8..a1dad18312 100644 --- a/test/e2e-shared-tests/mcd_config_drift.go +++ b/test/e2e-shared-tests/mcd_config_drift.go @@ -23,6 +23,7 @@ const ( configDriftSystemdUnitFileContents string = "unittest-service-unit-contents" configDriftSystemdDropinFilename string = "/etc/systemd/system/unittest.service.d/10-unittest-service.conf" configDriftSystemdDropinFileContents string = "unittest-service-dropin-contents" + configDriftCompressedFilename string = "/etc/compressed-file" configDriftFilename string = "/etc/etc-file" configDriftFileContents string = "expected-file-data" configDriftMCPrefix string = "mcd-config-drift" @@ -100,11 +101,36 @@ func (c *configDriftTest) Setup(t *testing.T) { } // This is the Machine Config that we drift from - c.mc = helpers.NewMachineConfigExtended( + c.mc = c.getMachineConfig(t) + + // Delegate to the attached Setup Func for MachineConfig, MachineConfigPool + // creation. + c.SetupFunc(c.mc) + + mcp, err := c.ClientSet.MachineConfigPools().Get(context.TODO(), c.MCPName, metav1.GetOptions{}) + require.Nil(t, err) + c.mcp = *mcp + + // Get the target node + c.node = helpers.GetSingleNodeByRole(t, c.ClientSet, c.MCPName) + + // Get the MCD pod + pod, err := helpers.MCDForNode(c.ClientSet, &c.node) + require.Nil(t, err) + + c.pod = *pod +} + +func (c configDriftTest) getMachineConfig(t *testing.T) *mcfgv1.MachineConfig { + compressedFile, err := helpers.CreateGzippedIgn3File(configDriftCompressedFilename, configDriftFileContents, 420) + require.Nil(t, err) + + return helpers.NewMachineConfigExtended( fmt.Sprintf("%s-%s", configDriftMCPrefix, string(uuid.NewUUID())), helpers.MCLabelForRole(c.MCPName), []ign3types.File{ - helpers.CreateIgn3File(configDriftFilename, "data:,"+configDriftFileContents, 420), + helpers.CreateEncodedIgn3File(configDriftFilename, configDriftFileContents, 420), + compressedFile, }, []ign3types.Unit{ { @@ -128,23 +154,6 @@ func (c *configDriftTest) Setup(t *testing.T) { "", "", ) - - // Delegate to the attached Setup Func for MachineConfig, MachineConfigPool - // creation. - c.SetupFunc(c.mc) - - mcp, err := c.ClientSet.MachineConfigPools().Get(context.TODO(), c.MCPName, metav1.GetOptions{}) - require.Nil(t, err) - c.mcp = *mcp - - // Get the target node - c.node = helpers.GetSingleNodeByRole(t, c.ClientSet, c.MCPName) - - // Get the MCD pod - pod, err := helpers.MCDForNode(c.ClientSet, &c.node) - require.Nil(t, err) - - c.pod = *pod } // Tears down the test objects by delegating to the attached TeardownFunc @@ -185,6 +194,14 @@ func (c configDriftTest) Run(t *testing.T) { c.runDegradeAndRecoverContentRevert(t, configDriftSystemdUnitFilename, configDriftSystemdUnitFileContents) }, }, + // Targets a regression identified by: + // https://bugzilla.redhat.com/show_bug.cgi?id=2032565 + { + name: "revert file content recovery for compressed file", + testFunc: func(t *testing.T) { + c.runDegradeAndRecoverContentRevert(t, configDriftCompressedFilename, configDriftFileContents) + }, + }, // 1. Mutates a file on the node. // 2. Creates the forcefile to cause recovery. { diff --git a/test/helpers/utils.go b/test/helpers/utils.go index cdebb02fc8..a8762de639 100644 --- a/test/helpers/utils.go +++ b/test/helpers/utils.go @@ -1,6 +1,8 @@ package helpers import ( + "bytes" + "compress/gzip" "context" "fmt" "math/rand" @@ -15,6 +17,7 @@ import ( "github.com/openshift/machine-config-operator/test/framework" "github.com/pkg/errors" "github.com/stretchr/testify/require" + "github.com/vincent-petithory/dataurl" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" @@ -383,6 +386,36 @@ func MCLabelForWorkers() map[string]string { // } // } +// Creates an Ign3 file whose contents are gzipped and encoded according to +// https://datatracker.ietf.org/doc/html/rfc2397 +func CreateGzippedIgn3File(path, content string, mode int) (ign3types.File, error) { + ign3File := ign3types.File{} + + buf := bytes.NewBuffer([]byte{}) + + gzipWriter := gzip.NewWriter(buf) + if _, err := gzipWriter.Write([]byte(content)); err != nil { + return ign3File, err + } + + if err := gzipWriter.Close(); err != nil { + return ign3File, err + } + + ign3File = CreateEncodedIgn3File(path, buf.String(), mode) + ign3File.Contents.Compression = StrToPtr("gzip") + + return ign3File, nil +} + +// Creates an Ign3 file whose contents are encoded according to +// https://datatracker.ietf.org/doc/html/rfc2397 +func CreateEncodedIgn3File(path, content string, mode int) ign3types.File { + encoded := dataurl.EncodeBytes([]byte(content)) + + return CreateIgn3File(path, encoded, mode) +} + func CreateIgn3File(path, content string, mode int) ign3types.File { return ign3types.File{ FileEmbedded1: ign3types.FileEmbedded1{