bug 1727104: daemon/controller: handle empty ignition configs

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
+}

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)
+}

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)

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,27 +281,34 @@ 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) {
 	f := newFixture(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{

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

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,
 		},

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{