diff --git a/pkg/controller/common/helpers.go b/pkg/controller/common/helpers.go index 8c246cf8fd..355656880e 100644 --- a/pkg/controller/common/helpers.go +++ b/pkg/controller/common/helpers.go @@ -2,9 +2,12 @@ package common import ( "io/ioutil" + "reflect" igntypes "github.com/coreos/ignition/config/v2_2/types" + validate "github.com/coreos/ignition/config/validate" "github.com/golang/glog" + errors "github.com/pkg/errors" ) // NewIgnConfig returns an empty ignition config with version set as latest version @@ -22,3 +25,19 @@ func WriteTerminationError(err error) { ioutil.WriteFile("/dev/termination-log", []byte(msg), 0644) glog.Fatal(msg) } + +// ValidateIgnition wraps the underlying Ignition validation, but explicitly supports +// a completely empty Ignition config as valid. This is because we +// want to allow MachineConfig objects which just have e.g. KernelArguments +// set, but no Ignition config. +// Returns nil if the config is valid (per above) or an error containing a Report otherwise. +func ValidateIgnition(cfg igntypes.Config) error { + // only validate if Ignition Config is not empty + if reflect.DeepEqual(igntypes.Config{}, cfg) { + return nil + } + if report := validate.ValidateWithoutSource(reflect.ValueOf(cfg)); report.IsFatal() { + return errors.Errorf("invalid Ignition config found: %v", report) + } + return nil +} diff --git a/pkg/controller/common/helpers_test.go b/pkg/controller/common/helpers_test.go new file mode 100644 index 0000000000..917e35908a --- /dev/null +++ b/pkg/controller/common/helpers_test.go @@ -0,0 +1,26 @@ +package common + +import ( + "testing" + + igntypes "github.com/coreos/ignition/config/v2_2/types" + "github.com/stretchr/testify/require" +) + +func TestValidateIgnition(t *testing.T) { + // Test that an empty ignition config returns nil + testIgnConfig := igntypes.Config{} + isValid := ValidateIgnition(testIgnConfig) + require.Nil(t, isValid) + + // Test that an invalid ignition config returns and error + tempUser1 := igntypes.PasswdUser{Name: "core", SSHAuthorizedKeys: []igntypes.SSHAuthorizedKey{"5678", "abc"}} + testIgnConfig.Passwd.Users = []igntypes.PasswdUser{tempUser1} + isValid = ValidateIgnition(testIgnConfig) + require.NotNil(t, isValid) + + // Test that a valid ignition config returns nil + testIgnConfig.Ignition.Version = "2.0.0" + isValid = ValidateIgnition(testIgnConfig) + require.Nil(t, isValid) +} diff --git a/pkg/controller/render/render_controller.go b/pkg/controller/render/render_controller.go index 92c410c0b8..3bf662a89f 100644 --- a/pkg/controller/render/render_controller.go +++ b/pkg/controller/render/render_controller.go @@ -5,11 +5,11 @@ import ( "reflect" "time" - "github.com/coreos/ignition/config/validate" "github.com/golang/glog" "github.com/openshift/machine-config-operator/lib/resourceapply" mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" "github.com/openshift/machine-config-operator/pkg/controller/common" + ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" mcfgclientset "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned" "github.com/openshift/machine-config-operator/pkg/generated/clientset/versioned/scheme" mcfginformersv1 "github.com/openshift/machine-config-operator/pkg/generated/informers/externalversions/machineconfiguration.openshift.io/v1" @@ -526,9 +526,8 @@ func (ctrl *Controller) syncGeneratedMachineConfig(pool *mcfgv1.MachineConfigPoo func generateRenderedMachineConfig(pool *mcfgv1.MachineConfigPool, configs []*mcfgv1.MachineConfig, cconfig *mcfgv1.ControllerConfig) (*mcfgv1.MachineConfig, error) { // Before merging all MCs for a specific pool, let's make sure each contains a valid Ignition Config for _, config := range configs { - rpt := validate.ValidateWithoutSource(reflect.ValueOf(config.Spec.Config.Ignition)) - if rpt.IsFatal() { - return nil, fmt.Errorf("machine config: %v contains invalid ignition config: %v", config.ObjectMeta.Name, rpt) + if err := ctrlcommon.ValidateIgnition(config.Spec.Config); err != nil { + return nil, err } } merged := mcfgv1.MergeMachineConfigs(configs, cconfig.Spec.OSImageURL) diff --git a/pkg/controller/render/render_controller_test.go b/pkg/controller/render/render_controller_test.go index 1db0a4c390..4aeb17acf5 100644 --- a/pkg/controller/render/render_controller_test.go +++ b/pkg/controller/render/render_controller_test.go @@ -265,11 +265,13 @@ func TestIgnValidationGenerateRenderedMachineConfig(t *testing.T) { mcp := helpers.NewMachineConfigPool("test-cluster-master", helpers.MasterSelector, nil, "") files := []igntypes.File{{ Node: igntypes.Node{ - Path: "/dummy/0", + Filesystem: "root", + Path: "/dummy/0", }, }, { Node: igntypes.Node{ - Path: "/dummy/1", + Filesystem: "root", + Path: "/dummy/1", }, }} mcs := []*mcfgv1.MachineConfig{ @@ -279,15 +281,20 @@ func TestIgnValidationGenerateRenderedMachineConfig(t *testing.T) { cc := newControllerConfig(ctrlcommon.ControllerConfigName) _, err := generateRenderedMachineConfig(mcp, mcs, cc) - if err != nil { - t.Fatalf("expected no error. Got: %v", err) - } + require.Nil(t, err) + // verify that an invalid igntion config (here a config with content and an empty version, + // will fail validation mcs[1].Spec.Config.Ignition.Version = "" _, err = generateRenderedMachineConfig(mcp, mcs, cc) - if err == nil { - t.Fatalf("expected error. mcs contains a machine config with invalid ignconfig version") - } + require.NotNil(t, err) + + // verify that a machine config with no ignition content will not fail validation + mcs[1].Spec.Config = igntypes.Config{} + mcs[1].Spec.KernelArguments = append(mcs[1].Spec.KernelArguments, "test1") + _, err = generateRenderedMachineConfig(mcp, mcs, cc) + require.Nil(t, err) + } func TestUpdatesGeneratedMachineConfig(t *testing.T) { @@ -295,11 +302,13 @@ func TestUpdatesGeneratedMachineConfig(t *testing.T) { mcp := helpers.NewMachineConfigPool("test-cluster-master", helpers.MasterSelector, nil, "") files := []igntypes.File{{ Node: igntypes.Node{ - Path: "/dummy/0", + Filesystem: "root", + Path: "/dummy/0", }, }, { Node: igntypes.Node{ - Path: "/dummy/1", + Filesystem: "root", + Path: "/dummy/1", }, }} mcs := []*mcfgv1.MachineConfig{ @@ -359,11 +368,13 @@ func TestDoNothing(t *testing.T) { mcp := helpers.NewMachineConfigPool("test-cluster-master", helpers.MasterSelector, nil, "") files := []igntypes.File{{ Node: igntypes.Node{ - Path: "/dummy/0", + Filesystem: "root", + Path: "/dummy/0", }, }, { Node: igntypes.Node{ - Path: "/dummy/1", + Filesystem: "root", + Path: "/dummy/1", }, }} mcs := []*mcfgv1.MachineConfig{ diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index 8f8b052886..025e06efb0 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -16,11 +16,11 @@ import ( "time" igntypes "github.com/coreos/ignition/config/v2_2/types" - "github.com/coreos/ignition/config/validate" "github.com/golang/glog" "github.com/google/renameio" drain "github.com/openshift/kubernetes-drain" 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/pkg/daemon/constants" errors "github.com/pkg/errors" "github.com/vincent-petithory/dataurl" @@ -316,9 +316,8 @@ func Reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) (*MachineConfigDif // Ignition section // First check if this is a generally valid Ignition Config - rpt := validate.ValidateWithoutSource(reflect.ValueOf(newIgn)) - if rpt.IsFatal() { - return nil, errors.Errorf("invalid Ignition config found: %v", rpt) + if err := ctrlcommon.ValidateIgnition(newIgn); err != nil { + return nil, err } // if the config versions are different, all bets are off. this probably diff --git a/test/e2e/mcd_test.go b/test/e2e/mcd_test.go index 521e22f050..eccb54f3ac 100644 --- a/test/e2e/mcd_test.go +++ b/test/e2e/mcd_test.go @@ -9,7 +9,7 @@ import ( "time" igntypes "github.com/coreos/ignition/config/v2_2/types" - mcv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" + 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/pkg/daemon/constants" "github.com/openshift/machine-config-operator/test/e2e/framework" @@ -70,9 +70,9 @@ func createIgnFile(path, content, fs string, mode int) igntypes.File { } } -func createMCToAddFile(name, filename, data, fs string) *mcv1.MachineConfig { +func createMCToAddFile(name, filename, data, fs string) *mcfgv1.MachineConfig { // create a dummy MC - mcadd := &mcv1.MachineConfig{} + mcadd := &mcfgv1.MachineConfig{} mcadd.ObjectMeta = metav1.ObjectMeta{ Name: fmt.Sprintf("%s-%s", name, uuid.NewUUID()), // TODO(runcom): hardcoded to workers for safety @@ -122,7 +122,7 @@ func waitForPoolComplete(t *testing.T, cs *framework.ClientSet, pool, target str if mcp.Status.Configuration.Name != target { return false, nil } - if mcv1.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcv1.MachineConfigPoolUpdated) { + if mcfgv1.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdated) { return true, nil } return false, nil @@ -204,7 +204,7 @@ func TestUpdateSSH(t *testing.T) { // create a dummy MC with an sshKey for user Core mcName := fmt.Sprintf("sshkeys-worker-%s", uuid.NewUUID()) - mcadd := &mcv1.MachineConfig{} + mcadd := &mcfgv1.MachineConfig{} mcadd.ObjectMeta = metav1.ObjectMeta{ Name: mcName, Labels: mcLabelForWorkers(), @@ -256,12 +256,12 @@ func TestUpdateSSH(t *testing.T) { func TestKernelArguments(t *testing.T) { cs := framework.NewClientSet("") bumpPoolMaxUnavailableTo(t, cs, 3) - kargsMC := &mcv1.MachineConfig{ + kargsMC := &mcfgv1.MachineConfig{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("kargs-%s", uuid.NewUUID()), Labels: mcLabelForWorkers(), }, - Spec: mcv1.MachineConfigSpec{ + Spec: mcfgv1.MachineConfigSpec{ Config: ctrlcommon.NewIgnConfig(), KernelArguments: []string{"nosmt", "foo=bar"}, }, @@ -323,7 +323,7 @@ func TestPoolDegradedOnFailToRender(t *testing.T) { if err != nil { return false, err } - if mcv1.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcv1.MachineConfigPoolDegraded) { + if mcfgv1.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded) { return true, nil } return false, nil @@ -342,7 +342,7 @@ func TestPoolDegradedOnFailToRender(t *testing.T) { if err != nil { return false, err } - if mcv1.IsMachineConfigPoolConditionFalse(mcp.Status.Conditions, mcv1.MachineConfigPoolRenderDegraded) { + if mcfgv1.IsMachineConfigPoolConditionFalse(mcp.Status.Conditions, mcfgv1.MachineConfigPoolRenderDegraded) { return true, nil } return false, nil @@ -355,8 +355,16 @@ func TestReconcileAfterBadMC(t *testing.T) { cs := framework.NewClientSet("") bumpPoolMaxUnavailableTo(t, cs, 3) - // create a bad MC w/o a filesystem field which is going to fail reconciling - mcadd := createMCToAddFile("add-a-file", "/etc/mytestconfs", "test", "") + // create a MC that contains a valid ignition config but is not reconcilable + mcadd := createMCToAddFile("add-a-file", "/etc/mytestconfs", "test", "root") + mcadd.Spec.Config.Networkd = igntypes.Networkd{ + Units: []igntypes.Networkdunit{ + igntypes.Networkdunit{ + Name: "test.network", + Contents: "test contents", + }, + }, + } // grab the initial machineconfig used by the worker pool // this MC is gonna be the one which is going to be reapplied once the bad MC is deleted @@ -400,7 +408,7 @@ func TestReconcileAfterBadMC(t *testing.T) { if err != nil { return false, err } - if mcv1.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcv1.MachineConfigPoolNodeDegraded) && mcp.Status.DegradedMachineCount >= 1 { + if mcfgv1.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolNodeDegraded) && mcp.Status.DegradedMachineCount >= 1 { return true, nil } return false, nil @@ -508,12 +516,12 @@ func TestDontDeleteRPMFiles(t *testing.T) { func TestFIPS(t *testing.T) { cs := framework.NewClientSet("") bumpPoolMaxUnavailableTo(t, cs, 3) - fipsMC := &mcv1.MachineConfig{ + fipsMC := &mcfgv1.MachineConfig{ ObjectMeta: metav1.ObjectMeta{ Name: fmt.Sprintf("fips-%s", uuid.NewUUID()), Labels: mcLabelForWorkers(), }, - Spec: mcv1.MachineConfigSpec{ + Spec: mcfgv1.MachineConfigSpec{ Config: ctrlcommon.NewIgnConfig(), Fips: true, }, diff --git a/test/helpers/helpers.go b/test/helpers/helpers.go index a70b853028..3534f03d0c 100644 --- a/test/helpers/helpers.go +++ b/test/helpers/helpers.go @@ -11,11 +11,15 @@ import ( ) var ( + // MasterSelector returns a label selector for masters nodes MasterSelector = metav1.AddLabelToSelector(&metav1.LabelSelector{}, "node-role/master", "") + // WorkerSelector returns a label selector for workers nodes WorkerSelector = metav1.AddLabelToSelector(&metav1.LabelSelector{}, "node-role/worker", "") - InfraSelector = metav1.AddLabelToSelector(&metav1.LabelSelector{}, "node-role/infra", "") + // InfraSelector returns a label selector for infra nodes + InfraSelector = metav1.AddLabelToSelector(&metav1.LabelSelector{}, "node-role/infra", "") ) +// NewMachineConfig returns a basic machine config with supplied labels, osurl & files added func NewMachineConfig(name string, labels map[string]string, osurl string, files []igntypes.File) *mcfgv1.MachineConfig { if labels == nil { labels = map[string]string{} @@ -43,6 +47,7 @@ func NewMachineConfig(name string, labels map[string]string, osurl string, files } } +// NewMachineConfigPool returns a MCP with supplied mcSelector, nodeSelector and machineconfig func NewMachineConfigPool(name string, mcSelector, nodeSelector *metav1.LabelSelector, currentMachineConfig string) *mcfgv1.MachineConfigPool { return &mcfgv1.MachineConfigPool{ TypeMeta: metav1.TypeMeta{