From 46a7bce45b8afc8e2e0b6be80123dd9acb7e7878 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Tue, 1 Mar 2022 10:37:18 -0500 Subject: [PATCH 1/5] controller: Add unit test for built-in templating Prep for further patches to use butane. --- pkg/controller/common/helpers.go | 2 +- pkg/controller/common/helpers_test.go | 52 +++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/pkg/controller/common/helpers.go b/pkg/controller/common/helpers.go index fe71d097a0..ed544a8e1b 100644 --- a/pkg/controller/common/helpers.go +++ b/pkg/controller/common/helpers.go @@ -563,7 +563,7 @@ func TranspileCoreOSConfigToIgn(files, units []string) (*ign3types.Config, error for _, d := range files { f := new(fcctbase.File) if err := yaml.Unmarshal([]byte(d), f); err != nil { - return nil, fmt.Errorf("failed to unmarshal file into struct: %v", err) + return nil, fmt.Errorf("failed to unmarshal %q into struct: %v", d, err) } f.Overwrite = &overwrite diff --git a/pkg/controller/common/helpers_test.go b/pkg/controller/common/helpers_test.go index 918917962c..292555d92f 100644 --- a/pkg/controller/common/helpers_test.go +++ b/pkg/controller/common/helpers_test.go @@ -2,11 +2,13 @@ package common import ( "reflect" + "strings" "testing" "github.com/clarketm/json" ign2types "github.com/coreos/ignition/config/v2_2/types" ign3types "github.com/coreos/ignition/v2/config/v3_2/types" + validate3 "github.com/coreos/ignition/v2/config/validate" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime" @@ -15,6 +17,56 @@ import ( "github.com/openshift/machine-config-operator/test/helpers" ) +func TestTranspileCoreOSConfig(t *testing.T) { + kubeletConfig := ` +mode: 0644 +path: "/etc/kubernetes/kubelet.conf" +contents: + inline: | + kind: KubeletConfiguration + apiVersion: kubelet.config.k8s.io/v1beta1 +` + auditConfig := ` +mode: 0644 +path: "/etc/audit/rules.d/mco-audit.rules" +contents: + inline: | + -a exclude,always -F msgtype=NETFILTER_CFG +` + kubeletService := `name: kubelet.service +enabled: true +contents: | + [Unit] + Description=kubelet + [Service] + ExecStart=/usr/bin/hyperkube +` + crioDropin := ` +name: crio.service +dropins: + - name: 10-mco-default-madv.conf + contents: | + [Service] + Environment="GODEBUG=x509ignoreCN=0,madvdontneed=1" +` + dockerDropin := ` +name: docker.socket +dropins: +- name: mco-disabled.conf + contents: | + [Unit] + ConditionPathExists=/enoent +` + config, err := TranspileCoreOSConfigToIgn([]string{kubeletConfig, auditConfig}, []string{kubeletService, crioDropin, dockerDropin}) + require.Nil(t, err) + if report := validate3.ValidateWithContext(config, nil); report.IsFatal() { + t.Fatalf("invalid ignition V3 config found: %v", report) + } + require.Equal(t, len(config.Storage.Files), 2) + require.True(t, strings.HasPrefix(*config.Storage.Files[0].Contents.Source, "data:,kind%3A%20KubeletConfiguration%0Aapi")) + require.Equal(t, len(config.Systemd.Units), 3) +} + func TestValidateIgnition(t *testing.T) { // Test that an empty ignition config returns nil testIgn2Config := ign2types.Config{} From d44202bb21715adeb0b2be28425334b7791d64dd Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 2 Mar 2022 15:57:20 -0500 Subject: [PATCH 2/5] Move helper to read Ignition file contents into controller/common We have other various places that are directly trying to do this, and will fail if files are compressed. (This function should probably be upstreamed into Ignition actually) --- pkg/controller/common/helpers.go | 39 +++++++++++++++++++++++++ pkg/daemon/config_drift_monitor_test.go | 3 +- pkg/daemon/on_disk_validation.go | 4 +-- pkg/daemon/update.go | 38 +----------------------- 4 files changed, 44 insertions(+), 40 deletions(-) diff --git a/pkg/controller/common/helpers.go b/pkg/controller/common/helpers.go index ed544a8e1b..310f953cf9 100644 --- a/pkg/controller/common/helpers.go +++ b/pkg/controller/common/helpers.go @@ -1,8 +1,11 @@ package common import ( + "bytes" + "compress/gzip" "context" "fmt" + "io" "io/ioutil" "net/url" "os" @@ -370,6 +373,42 @@ func validateIgn3FileModes(cfg ign3types.Config) error { return nil } +// DecodeIgnitionFileContents returns uncompressed, decoded inline file contents. +// This function does not handle remote resources; it assumes they have already +// been fetched. +func DecodeIgnitionFileContents(source, compression *string) ([]byte, error) { + var contentsBytes []byte + + // To allow writing of "empty" files we'll allow source to be nil + if source != nil { + source, err := dataurl.DecodeString(*source) + if err != nil { + return []byte{}, fmt.Errorf("could not decode file content string: %w", err) + } + if compression != nil { + switch *compression { + case "": + contentsBytes = source.Data + case "gzip": + reader, err := gzip.NewReader(bytes.NewReader(source.Data)) + if err != nil { + return []byte{}, fmt.Errorf("could not create gzip reader: %w", err) + } + defer reader.Close() + contentsBytes, err = io.ReadAll(reader) + if err != nil { + return []byte{}, fmt.Errorf("failed decompressing: %w", err) + } + default: + return []byte{}, fmt.Errorf("unsupported compression type %q", *compression) + } + } else { + contentsBytes = source.Data + } + } + return contentsBytes, nil +} + // InSlice search for an element in slice and return true if found, otherwise return false func InSlice(elem string, slice []string) bool { for _, k := range slice { diff --git a/pkg/daemon/config_drift_monitor_test.go b/pkg/daemon/config_drift_monitor_test.go index dbd9af345e..35d5062240 100644 --- a/pkg/daemon/config_drift_monitor_test.go +++ b/pkg/daemon/config_drift_monitor_test.go @@ -12,6 +12,7 @@ import ( 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/test/helpers" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -464,7 +465,7 @@ 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) + decodedContents, err := ctrlcommon.DecodeIgnitionFileContents(file.Contents.Source, file.Contents.Compression) require.Nil(t, err) require.Nil(t, writeFileAtomicallyWithDefaults(file.Path, decodedContents)) diff --git a/pkg/daemon/on_disk_validation.go b/pkg/daemon/on_disk_validation.go index b4445c8b9b..8d8f6f330d 100644 --- a/pkg/daemon/on_disk_validation.go +++ b/pkg/daemon/on_disk_validation.go @@ -150,7 +150,7 @@ func checkV3Files(files []ign3types.File) error { if f.Mode != nil { mode = os.FileMode(*f.Mode) } - contents, err := decodeContents(f.Contents.Source, f.Contents.Compression) + contents, err := ctrlcommon.DecodeIgnitionFileContents(f.Contents.Source, f.Contents.Compression) if err != nil { return fmt.Errorf("couldn't decode file %q: %w", f.Path, err) } @@ -177,7 +177,7 @@ func checkV2Files(files []ign2types.File) error { if f.Mode != nil { mode = os.FileMode(*f.Mode) } - contents, err := decodeContents(&f.Contents.Source, &f.Contents.Compression) + contents, err := ctrlcommon.DecodeIgnitionFileContents(&f.Contents.Source, &f.Contents.Compression) if err != nil { return fmt.Errorf("couldn't decode file %q: %w", f.Path, err) } diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index 4799bd879c..9c965d2009 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -3,9 +3,7 @@ package daemon import ( "bufio" "bytes" - "compress/gzip" "fmt" - "io" "io/ioutil" "os" "os/exec" @@ -22,7 +20,6 @@ import ( "github.com/golang/glog" "github.com/google/renameio" errors "github.com/pkg/errors" - "github.com/vincent-petithory/dataurl" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -1529,39 +1526,6 @@ func (dn *Daemon) writeUnits(units []ign3types.Unit) error { return nil } -func decodeContents(source, compression *string) ([]byte, error) { - var contentsBytes []byte - - // To allow writing of "empty" files we'll allow source to be nil - if source != nil { - source, err := dataurl.DecodeString(*source) - if err != nil { - return []byte{}, fmt.Errorf("could not decode file content string: %w", err) - } - if compression != nil { - switch *compression { - case "": - contentsBytes = source.Data - case "gzip": - reader, err := gzip.NewReader(bytes.NewReader(source.Data)) - if err != nil { - return []byte{}, fmt.Errorf("could not create gzip reader: %w", err) - } - defer reader.Close() - contentsBytes, err = io.ReadAll(reader) - if err != nil { - return []byte{}, fmt.Errorf("failed decompressing: %w", err) - } - default: - return []byte{}, fmt.Errorf("unsupported compression type %q", *compression) - } - } else { - contentsBytes = source.Data - } - } - return contentsBytes, nil -} - // 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 { @@ -1574,7 +1538,7 @@ func (dn *Daemon) writeFiles(files []ign3types.File) error { return fmt.Errorf("found an append section when writing files. Append is not supported") } - decodedContents, err := decodeContents(file.Contents.Source, file.Contents.Compression) + decodedContents, err := ctrlcommon.DecodeIgnitionFileContents(file.Contents.Source, file.Contents.Compression) if err != nil { return fmt.Errorf("could not decode file %q: %w", file.Path, err) } From 918f97bc1c097bd9b6cf045b02b6a8863ccdceac Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 2 Mar 2022 16:12:00 -0500 Subject: [PATCH 3/5] controller: Convert all things reading files from Ignition to use helper This way they all correctly handle compression. Will be needed for a further patch which will use butane, which seems to use compression by default. --- pkg/controller/bootstrap/bootstrap_test.go | 13 ++++++------- pkg/controller/common/helpers_test.go | 2 +- .../container_runtime_config_controller.go | 13 ++++++------- .../container_runtime_config_controller_test.go | 13 ++++++------- .../kubelet-config/kubelet_config_bootstrap_test.go | 5 ++--- .../kubelet-config/kubelet_config_controller.go | 5 ++--- .../kubelet-config/kubelet_config_features_test.go | 10 +++++----- 7 files changed, 28 insertions(+), 33 deletions(-) diff --git a/pkg/controller/bootstrap/bootstrap_test.go b/pkg/controller/bootstrap/bootstrap_test.go index 3578672ce0..bd4553f4b2 100644 --- a/pkg/controller/bootstrap/bootstrap_test.go +++ b/pkg/controller/bootstrap/bootstrap_test.go @@ -12,7 +12,6 @@ import ( ign3types "github.com/coreos/ignition/v2/config/v3_2/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/vincent-petithory/dataurl" "k8s.io/apimachinery/pkg/util/diff" mcoResourceRead "github.com/openshift/machine-config-operator/lib/resourceread" @@ -190,15 +189,15 @@ func TestBootstrapRun(t *testing.T) { } } require.NotNil(t, registriesConfig) - dataURL, err := dataurl.DecodeString(*registriesConfig.Contents.Source) + contents, err := ctrlcommon.DecodeIgnitionFileContents(registriesConfig.Contents.Source, registriesConfig.Contents.Compression) require.NoError(t, err) // Only a minimal presence check; more comprehensive tests that the contents correspond to the ICSP semantics are // maintained in pkg/controller/container-runtime-config. - assert.Contains(t, string(dataURL.Data), "registry.mirror.example.com/ocp") - assert.Contains(t, string(dataURL.Data), "insecure-reg-1.io") - assert.Contains(t, string(dataURL.Data), "insecure-reg-2.io") - assert.Contains(t, string(dataURL.Data), "blocked-reg.io") - assert.NotContains(t, string(dataURL.Data), "release-registry.product.example.org") + assert.Contains(t, string(contents), "registry.mirror.example.com/ocp") + assert.Contains(t, string(contents), "insecure-reg-1.io") + assert.Contains(t, string(contents), "insecure-reg-2.io") + assert.Contains(t, string(contents), "blocked-reg.io") + assert.NotContains(t, string(contents), "release-registry.product.example.org") }) } } diff --git a/pkg/controller/common/helpers_test.go b/pkg/controller/common/helpers_test.go index 292555d92f..2256349684 100644 --- a/pkg/controller/common/helpers_test.go +++ b/pkg/controller/common/helpers_test.go @@ -300,7 +300,7 @@ func TestMergeMachineConfigs(t *testing.T) { }, Passwd: ign3types.Passwd{ Users: []ign3types.PasswdUser{ - ign3types.PasswdUser{Name: "core", SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"1234"}}, + {Name: "core", SSHAuthorizedKeys: []ign3types.SSHAuthorizedKey{"1234"}}, }, }, } diff --git a/pkg/controller/container-runtime-config/container_runtime_config_controller.go b/pkg/controller/container-runtime-config/container_runtime_config_controller.go index 6a38ac5c61..6edae9057b 100644 --- a/pkg/controller/container-runtime-config/container_runtime_config_controller.go +++ b/pkg/controller/container-runtime-config/container_runtime_config_controller.go @@ -18,7 +18,6 @@ import ( cligolistersv1 "github.com/openshift/client-go/config/listers/config/v1" operatorinformersv1alpha1 "github.com/openshift/client-go/operator/informers/externalversions/operator/v1alpha1" operatorlistersv1alpha1 "github.com/openshift/client-go/operator/listers/operator/v1alpha1" - "github.com/vincent-petithory/dataurl" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" @@ -659,11 +658,11 @@ func mergeConfigChanges(origFile *ign3types.File, cfg *mcfgv1.ContainerRuntimeCo if origFile.Contents.Source == nil { return nil, fmt.Errorf("original Container Runtime config is empty") } - dataURL, err := dataurl.DecodeString(*origFile.Contents.Source) + contents, err := ctrlcommon.DecodeIgnitionFileContents(origFile.Contents.Source, origFile.Contents.Compression) if err != nil { return nil, fmt.Errorf("could not decode original Container Runtime config: %v", err) } - cfgTOML, err := update(dataURL.Data, cfg.Spec.ContainerRuntimeConfig) + cfgTOML, err := update(contents, cfg.Spec.ContainerRuntimeConfig) if err != nil { return nil, fmt.Errorf("could not update container runtime config with new changes: %v", err) } @@ -824,11 +823,11 @@ func registriesConfigIgnition(templateDir string, controllerConfig *mcfgv1.Contr if originalRegistriesIgn.Contents.Source == nil { return nil, fmt.Errorf("original registries config is empty") } - dataURL, err := dataurl.DecodeString(*originalRegistriesIgn.Contents.Source) + contents, err := ctrlcommon.DecodeIgnitionFileContents(originalRegistriesIgn.Contents.Source, originalRegistriesIgn.Contents.Compression) if err != nil { return nil, fmt.Errorf("could not decode original registries config: %v", err) } - registriesTOML, err = updateRegistriesConfig(dataURL.Data, insecureRegs, blockedRegs, icspRules) + registriesTOML, err = updateRegistriesConfig(contents, insecureRegs, blockedRegs, icspRules) if err != nil { return nil, fmt.Errorf("could not update registries config with new changes: %v", err) } @@ -837,11 +836,11 @@ func registriesConfigIgnition(templateDir string, controllerConfig *mcfgv1.Contr if originalPolicyIgn.Contents.Source == nil { return nil, fmt.Errorf("original policy json is empty") } - dataURL, err := dataurl.DecodeString(*originalPolicyIgn.Contents.Source) + contents, err := ctrlcommon.DecodeIgnitionFileContents(originalPolicyIgn.Contents.Source, originalPolicyIgn.Contents.Compression) if err != nil { return nil, fmt.Errorf("could not decode original policy json: %v", err) } - policyJSON, err = updatePolicyJSON(dataURL.Data, blockedRegs, allowedRegs) + policyJSON, err = updatePolicyJSON(contents, blockedRegs, allowedRegs) if err != nil { return nil, fmt.Errorf("could not update policy json with new changes: %v", err) } diff --git a/pkg/controller/container-runtime-config/container_runtime_config_controller_test.go b/pkg/controller/container-runtime-config/container_runtime_config_controller_test.go index 05f20123ef..6a37edd135 100644 --- a/pkg/controller/container-runtime-config/container_runtime_config_controller_test.go +++ b/pkg/controller/container-runtime-config/container_runtime_config_controller_test.go @@ -12,7 +12,6 @@ import ( "github.com/golang/glog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/vincent-petithory/dataurl" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -391,9 +390,9 @@ func verifyRegistriesConfigAndPolicyJSONContents(t *testing.T, mc *mcfgv1.Machin regfile = ignCfg.Storage.Files[1] } assert.Equal(t, registriesConfigPath, regfile.Node.Path) - registriesConf, err := dataurl.DecodeString(*regfile.Contents.Source) + registriesConf, err := ctrlcommon.DecodeIgnitionFileContents(regfile.Contents.Source, regfile.Contents.Compression) require.NoError(t, err) - assert.Equal(t, string(expectedRegistriesConf), string(registriesConf.Data)) + assert.Equal(t, string(expectedRegistriesConf), string(registriesConf)) // Validate the policy.json contents if a change is expected from the tests if verifyPolicyJSON { @@ -406,9 +405,9 @@ func verifyRegistriesConfigAndPolicyJSONContents(t *testing.T, mc *mcfgv1.Machin policyfile = ignCfg.Storage.Files[0] } assert.Equal(t, policyConfigPath, policyfile.Node.Path) - policyJSON, err := dataurl.DecodeString(*policyfile.Contents.Source) + policyJSON, err := ctrlcommon.DecodeIgnitionFileContents(policyfile.Contents.Source, policyfile.Contents.Compression) require.NoError(t, err) - assert.Equal(t, string(expectedPolicyJSON), string(policyJSON.Data)) + assert.Equal(t, string(expectedPolicyJSON), string(policyJSON)) } // Validate the drop-in search registries file contents if a change is expected from the tests @@ -419,9 +418,9 @@ func verifyRegistriesConfigAndPolicyJSONContents(t *testing.T, mc *mcfgv1.Machin dropinfile = ignCfg.Storage.Files[1] } assert.Equal(t, searchRegDropInFilePath, dropinfile.Node.Path) - searchRegsConf, err := dataurl.DecodeString(*dropinfile.Contents.Source) + searchRegsConf, err := ctrlcommon.DecodeIgnitionFileContents(dropinfile.Contents.Source, dropinfile.Contents.Compression) require.NoError(t, err) - assert.Equal(t, string(expectedSearchRegsConf[0].data), string(searchRegsConf.Data)) + assert.Equal(t, string(expectedSearchRegsConf[0].data), string(searchRegsConf)) } } diff --git a/pkg/controller/kubelet-config/kubelet_config_bootstrap_test.go b/pkg/controller/kubelet-config/kubelet_config_bootstrap_test.go index d5aee588da..c1c81223d2 100644 --- a/pkg/controller/kubelet-config/kubelet_config_bootstrap_test.go +++ b/pkg/controller/kubelet-config/kubelet_config_bootstrap_test.go @@ -9,7 +9,6 @@ import ( ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" "github.com/openshift/machine-config-operator/test/helpers" "github.com/stretchr/testify/require" - "github.com/vincent-petithory/dataurl" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1" @@ -91,9 +90,9 @@ func verifyKubeletConfigJSONContents(t *testing.T, mc *mcfgv1.MachineConfig, mcN ignCfg, err := ctrlcommon.ParseAndConvertConfig(mc.Spec.Config.Raw) require.NoError(t, err) regfile := ignCfg.Storage.Files[0] - conf, err := dataurl.DecodeString(*regfile.Contents.Source) + conf, err := ctrlcommon.DecodeIgnitionFileContents(regfile.Contents.Source, regfile.Contents.Compression) require.NoError(t, err) - require.Contains(t, string(conf.Data), `"maxPods": 100`) + require.Contains(t, string(conf), `"maxPods": 100`) } func TestGenerateDefaultManagedKeyKubelet(t *testing.T) { diff --git a/pkg/controller/kubelet-config/kubelet_config_controller.go b/pkg/controller/kubelet-config/kubelet_config_controller.go index a3d45d2a60..b6572236e8 100644 --- a/pkg/controller/kubelet-config/kubelet_config_controller.go +++ b/pkg/controller/kubelet-config/kubelet_config_controller.go @@ -13,7 +13,6 @@ import ( ign3types "github.com/coreos/ignition/v2/config/v3_2/types" "github.com/golang/glog" "github.com/imdario/mergo" - "github.com/vincent-petithory/dataurl" corev1 "k8s.io/api/core/v1" macherrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -340,11 +339,11 @@ func generateOriginalKubeletConfigWithFeatureGates(cc *mcfgv1.ControllerConfig, if originalKubeletIgn.Contents.Source == nil { return nil, fmt.Errorf("the original Kubelet source string is empty: %v", err) } - dataURL, err := dataurl.DecodeString(*originalKubeletIgn.Contents.Source) + contents, err := ctrlcommon.DecodeIgnitionFileContents(originalKubeletIgn.Contents.Source, originalKubeletIgn.Contents.Compression) if err != nil { return nil, fmt.Errorf("could not decode the original Kubelet source string: %v", err) } - originalKubeConfig, err := decodeKubeletConfig(dataURL.Data) + originalKubeConfig, err := decodeKubeletConfig(contents) if err != nil { return nil, fmt.Errorf("could not deserialize the Kubelet source: %v", err) } diff --git a/pkg/controller/kubelet-config/kubelet_config_features_test.go b/pkg/controller/kubelet-config/kubelet_config_features_test.go index ecc7cbbf17..8fb6143fd3 100644 --- a/pkg/controller/kubelet-config/kubelet_config_features_test.go +++ b/pkg/controller/kubelet-config/kubelet_config_features_test.go @@ -7,7 +7,6 @@ import ( ign3types "github.com/coreos/ignition/v2/config/v3_2/types" configv1 "github.com/openshift/api/config/v1" osev1 "github.com/openshift/api/config/v1" - "github.com/vincent-petithory/dataurl" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kubeletconfigv1beta1 "k8s.io/kubelet/config/v1beta1" @@ -29,8 +28,9 @@ func TestFeatureGateDrift(t *testing.T) { if err != nil { t.Errorf("could not generate kubelet config from templates %v", err) } - dataURL, _ := dataurl.DecodeString(*kubeletConfig.Contents.Source) - originalKubeConfig, _ := decodeKubeletConfig(dataURL.Data) + contents, err := ctrlcommon.DecodeIgnitionFileContents(kubeletConfig.Contents.Source, kubeletConfig.Contents.Compression) + require.Nil(t, err) + originalKubeConfig, _ := decodeKubeletConfig(contents) defaultFeatureGates, err := generateFeatureMap(createNewDefaultFeatureGate()) if err != nil { t.Errorf("could not generate defaultFeatureGates: %v", err) @@ -190,10 +190,10 @@ func TestBootstrapFeaturesCustomNoUpgrade(t *testing.T) { for _, mc := range mcs { ignCfg, err := ctrlcommon.ParseAndConvertConfig(mc.Spec.Config.Raw) regfile := ignCfg.Storage.Files[0] - conf, err := dataurl.DecodeString(*regfile.Contents.Source) + conf, err := ctrlcommon.DecodeIgnitionFileContents(regfile.Contents.Source, regfile.Contents.Compression) require.NoError(t, err) - originalKubeConfig, _ := decodeKubeletConfig(conf.Data) + originalKubeConfig, _ := decodeKubeletConfig(conf) defaultFeatureGates, err := generateFeatureMap(createNewDefaultFeatureGate()) if err != nil { t.Errorf("could not generate defaultFeatureGates: %v", err) From 8d66925a6bdbe66fd515c20d4f678f369bbfa996 Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 3 Mar 2022 10:36:18 -0500 Subject: [PATCH 4/5] helpers: Use a more descriptive variable name --- pkg/controller/common/helpers.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/controller/common/helpers.go b/pkg/controller/common/helpers.go index 310f953cf9..ddfb155400 100644 --- a/pkg/controller/common/helpers.go +++ b/pkg/controller/common/helpers.go @@ -599,10 +599,10 @@ func TranspileCoreOSConfigToIgn(files, units []string) (*ign3types.Config, error overwrite := true outConfig := ign3types.Config{} // Convert data to Ignition resources - for _, d := range files { + for _, contents := range files { f := new(fcctbase.File) - if err := yaml.Unmarshal([]byte(d), f); err != nil { - return nil, fmt.Errorf("failed to unmarshal %q into struct: %v", d, err) + if err := yaml.Unmarshal([]byte(contents), f); err != nil { + return nil, fmt.Errorf("failed to unmarshal %q into struct: %v", contents, err) } f.Overwrite = &overwrite @@ -617,9 +617,9 @@ func TranspileCoreOSConfigToIgn(files, units []string) (*ign3types.Config, error outConfig = ign3.Merge(outConfig, ign3_2config) } - for _, d := range units { + for _, contents := range units { u := new(fcctbase.Unit) - if err := yaml.Unmarshal([]byte(d), u); err != nil { + if err := yaml.Unmarshal([]byte(contents), u); err != nil { return nil, fmt.Errorf("failed to unmarshal systemd unit into struct: %v", err) } From 077b97f13c45677a598d16267833be8138dae5af Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Thu, 3 Mar 2022 10:44:42 -0500 Subject: [PATCH 5/5] controller/tests/kubelet: More consistently check errors Came up in review, but these are far from the only thing. --- .../kubelet_config_features_test.go | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/pkg/controller/kubelet-config/kubelet_config_features_test.go b/pkg/controller/kubelet-config/kubelet_config_features_test.go index 8fb6143fd3..80b63b2011 100644 --- a/pkg/controller/kubelet-config/kubelet_config_features_test.go +++ b/pkg/controller/kubelet-config/kubelet_config_features_test.go @@ -29,8 +29,9 @@ func TestFeatureGateDrift(t *testing.T) { t.Errorf("could not generate kubelet config from templates %v", err) } contents, err := ctrlcommon.DecodeIgnitionFileContents(kubeletConfig.Contents.Source, kubeletConfig.Contents.Compression) - require.Nil(t, err) - originalKubeConfig, _ := decodeKubeletConfig(contents) + require.NoError(t, err) + originalKubeConfig, err := decodeKubeletConfig(contents) + require.NoError(t, err) defaultFeatureGates, err := generateFeatureMap(createNewDefaultFeatureGate()) if err != nil { t.Errorf("could not generate defaultFeatureGates: %v", err) @@ -53,8 +54,10 @@ func TestFeaturesDefault(t *testing.T) { mcp2 := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v0") kc1 := newKubeletConfig("smaller-max-pods", &kubeletconfigv1beta1.KubeletConfiguration{MaxPods: 100}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", "")) kc2 := newKubeletConfig("bigger-max-pods", &kubeletconfigv1beta1.KubeletConfiguration{MaxPods: 250}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", "")) - kubeletConfigKey1, _ := getManagedKubeletConfigKey(mcp, f.client, kc1) - kubeletConfigKey2, _ := getManagedKubeletConfigKey(mcp2, f.client, kc2) + kubeletConfigKey1, err := getManagedKubeletConfigKey(mcp, f.client, kc1) + require.NoError(t, err) + kubeletConfigKey2, err := getManagedKubeletConfigKey(mcp2, f.client, kc2) + require.NoError(t, err) mcs := helpers.NewMachineConfig(kubeletConfigKey1, map[string]string{"node-role/master": ""}, "dummy://", []ign3types.File{{}}) mcs2 := helpers.NewMachineConfig(kubeletConfigKey2, map[string]string{"node-role/worker": ""}, "dummy://", []ign3types.File{{}}) mcsDeprecated := mcs.DeepCopy() @@ -92,8 +95,10 @@ func TestFeaturesCustomNoUpgrade(t *testing.T) { mcp2 := helpers.NewMachineConfigPool("worker", nil, helpers.WorkerSelector, "v0") kc1 := newKubeletConfig("smaller-max-pods", &kubeletconfigv1beta1.KubeletConfiguration{MaxPods: 100}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", "")) kc2 := newKubeletConfig("bigger-max-pods", &kubeletconfigv1beta1.KubeletConfiguration{MaxPods: 250}, metav1.AddLabelToSelector(&metav1.LabelSelector{}, "pools.operator.machineconfiguration.openshift.io/master", "")) - kubeletConfigKey1, _ := getManagedKubeletConfigKey(mcp, f.client, kc1) - kubeletConfigKey2, _ := getManagedKubeletConfigKey(mcp2, f.client, kc2) + kubeletConfigKey1, err := getManagedKubeletConfigKey(mcp, f.client, kc1) + require.NoError(t, err) + kubeletConfigKey2, err := getManagedKubeletConfigKey(mcp2, f.client, kc2) + require.NoError(t, err) mcs := helpers.NewMachineConfig(kubeletConfigKey1, map[string]string{"node-role/master": ""}, "dummy://", []ign3types.File{{}}) mcs2 := helpers.NewMachineConfig(kubeletConfigKey2, map[string]string{"node-role/worker": ""}, "dummy://", []ign3types.File{{}}) mcsDeprecated := mcs.DeepCopy() @@ -193,7 +198,8 @@ func TestBootstrapFeaturesCustomNoUpgrade(t *testing.T) { conf, err := ctrlcommon.DecodeIgnitionFileContents(regfile.Contents.Source, regfile.Contents.Compression) require.NoError(t, err) - originalKubeConfig, _ := decodeKubeletConfig(conf) + originalKubeConfig, err := decodeKubeletConfig(conf) + require.NoError(t, err) defaultFeatureGates, err := generateFeatureMap(createNewDefaultFeatureGate()) if err != nil { t.Errorf("could not generate defaultFeatureGates: %v", err)