Improve provider spec validation

openshift · Jun 1, 2020 · 16e6ee8 · 16e6ee8
1 parent d19e8d0
commit 16e6ee8
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 9 deletions.
diff --git a/pkg/controller/vsphere/machine_scope.go b/pkg/controller/vsphere/machine_scope.go
@@ -8,6 +8,7 @@ import (
 	machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
 	apivsphere "github.com/openshift/machine-api-operator/pkg/apis/vsphereprovider/v1beta1"
 	machineapierros "github.com/openshift/machine-api-operator/pkg/controller/machine"
+	machinecontroller "github.com/openshift/machine-api-operator/pkg/controller/machine"
 	"github.com/openshift/machine-api-operator/pkg/controller/vsphere/session"
 	apicorev1 "k8s.io/api/core/v1"
 	apimachineryerrors "k8s.io/apimachinery/pkg/api/errors"
@@ -133,7 +134,7 @@ func (s *machineScope) GetSession() *session.Session {
 // provider spec, if one is set.
 func (s *machineScope) GetUserData() ([]byte, error) {
 	if s.providerSpec == nil || s.providerSpec.UserDataSecret == nil {
-		return nil, nil
+		return nil, machinecontroller.InvalidMachineConfiguration("user data secret is missing in provider spec")
 	}
 
 	userDataSecret := &apicorev1.Secret{}

diff --git a/pkg/controller/vsphere/machine_scope_test.go b/pkg/controller/vsphere/machine_scope_test.go
@@ -94,14 +94,14 @@ func TestGetUserData(t *testing.T) {
 			testCase:         "no provider spec",
 			userDataSecret:   nil,
 			providerSpec:     nil,
-			expectError:      false,
+			expectError:      true,
 			expectedUserdata: nil,
 		},
 		{
 			testCase:         "no user-data in provider spec",
 			userDataSecret:   nil,
 			providerSpec:     &vspherev1.VSphereMachineProviderSpec{},
-			expectError:      false,
+			expectError:      true,
 			expectedUserdata: nil,
 		},
 	}

diff --git a/pkg/controller/vsphere/reconciler.go b/pkg/controller/vsphere/reconciler.go
@@ -410,7 +410,9 @@ func clone(s *machineScope) (string, error) {
 
 	vmTemplate, err := s.GetSession().FindVM(*s, "", s.providerSpec.Template)
 	if err != nil {
-		return "", err
+		notFoundMsg := "template not found, specify valid value"
+		defaultError := fmt.Errorf("unable to get template %q: %w", s.providerSpec.Template, err)
+		return "", handleVSphereError("", notFoundMsg, defaultError, err)
 	}
 
 	var snapshotRef *types.ManagedObjectReference
@@ -589,7 +591,10 @@ func getNetworkDevices(s *machineScope, devices object.VirtualDeviceList) ([]typ
 
 		ref, err := s.GetSession().Finder.Network(s.Context, netSpec.NetworkName)
 		if err != nil {
-			return nil, fmt.Errorf("unable to find network %q: %w", netSpec.NetworkName, err)
+			multipleFoundMsg := "multiple networks found, specify one in config"
+			notFoundMsg := "network not found, specify valid value"
+			defaultError := fmt.Errorf("unable to get network for %q: %w", netSpec.NetworkName, err)
+			return nil, handleVSphereError(multipleFoundMsg, notFoundMsg, defaultError, err)
 		}
 
 		backing, err := ref.EthernetCardBackingInfo(s.Context)

diff --git a/pkg/controller/vsphere/reconciler_test.go b/pkg/controller/vsphere/reconciler_test.go
@@ -95,6 +95,7 @@ func TestClone(t *testing.T) {
 	password, _ := server.URL.User.Password()
 	namespace := "test"
 	vm := simulator.Map.Any("VirtualMachine").(*simulator.VirtualMachine)
+
 	credentialsSecret := corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test",
@@ -106,6 +107,17 @@ func TestClone(t *testing.T) {
 		},
 	}
 
+	userDataSecretName := "vsphere-ignition"
+	userDataSecret := corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      userDataSecretName,
+			Namespace: namespace,
+		},
+		Data: map[string][]byte{
+			userDataSecretKey: []byte("{}"),
+		},
+	}
+
 	testCases := []struct {
 		testCase              string
 		cloneVM               bool
@@ -123,6 +135,10 @@ func TestClone(t *testing.T) {
 				Workspace: &vsphereapi.Workspace{
 					Server: server.URL.Host,
 				},
+				Template: vm.Name,
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
 			},
 			cloneVM:     true,
 			machineName: "test0",
@@ -137,6 +153,10 @@ func TestClone(t *testing.T) {
 					Server: server.URL.Host,
 					Folder: "custom-folder",
 				},
+				Template: vm.Name,
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
 			},
 			cloneVM:     true,
 			machineName: "test1",
@@ -151,6 +171,10 @@ func TestClone(t *testing.T) {
 					Server:       server.URL.Host,
 					ResourcePool: "invalid",
 				},
+				Template: vm.Name,
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
 			},
 			expectedError: errors.New("resource pool not found, specify valid value"),
 		},
@@ -164,6 +188,10 @@ func TestClone(t *testing.T) {
 					Server:       server.URL.Host,
 					ResourcePool: "/DC0/host/DC0_C0/Resources/...",
 				},
+				Template: vm.Name,
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
 			},
 			expectedError: errors.New("multiple resource pools found, specify one in config"),
 			setupFailureCondition: func() error {
@@ -195,6 +223,10 @@ func TestClone(t *testing.T) {
 					Server: server.URL.Host,
 					Folder: "invalid",
 				},
+				Template: vm.Name,
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
 			},
 		},
 		{
@@ -207,6 +239,10 @@ func TestClone(t *testing.T) {
 					Server: server.URL.Host,
 					Folder: "/DC0/vm/...",
 				},
+				Template: vm.Name,
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
 			},
 			expectedError: errors.New("multiple folders found, specify one in config"),
 			setupFailureCondition: func() error {
@@ -236,6 +272,10 @@ func TestClone(t *testing.T) {
 					Server:    server.URL.Host,
 					Datastore: "invalid",
 				},
+				Template: vm.Name,
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
 			},
 			expectedError: errors.New("datastore not found, specify valid value"),
 		},
@@ -249,6 +289,10 @@ func TestClone(t *testing.T) {
 					Server:    server.URL.Host,
 					Datastore: "/DC0/...",
 				},
+				Template: vm.Name,
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
 			},
 			expectedError: errors.New("multiple datastores found, specify one in config"),
 			setupFailureCondition: func() error {
@@ -276,6 +320,16 @@ func TestClone(t *testing.T) {
 				return nil
 			},
 		},
+		{
+			testCase: "fail on invalid template",
+			providerSpec: vsphereapi.VSphereMachineProviderSpec{
+				Template: "invalid",
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
+			},
+			expectedError: errors.New("template not found, specify valid value"),
+		},
 	}
 
 	for _, tc := range testCases {
@@ -300,11 +354,9 @@ func TestClone(t *testing.T) {
 				providerSpec:   &tc.providerSpec,
 				session:        session,
 				providerStatus: &vsphereapi.VSphereMachineProviderStatus{},
-				client:         fake.NewFakeClientWithScheme(scheme.Scheme, &credentialsSecret),
+				client:         fake.NewFakeClientWithScheme(scheme.Scheme, &credentialsSecret, &userDataSecret),
 			}
 
-			machineScope.providerSpec.Template = vm.Name
-
 			if tc.machineName != "" {
 				machineScope.machine.Name = tc.machineName
 			}
@@ -1247,6 +1299,17 @@ func TestCreate(t *testing.T) {
 		},
 	}
 
+	userDataSecretName := "vsphere-ignition"
+	userDataSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      userDataSecretName,
+			Namespace: namespace,
+		},
+		Data: map[string][]byte{
+			userDataSecretKey: []byte("{}"),
+		},
+	}
+
 	cases := []struct {
 		name                  string
 		expectedError         error
@@ -1264,6 +1327,9 @@ func TestCreate(t *testing.T) {
 				CredentialsSecret: &corev1.LocalObjectReference{
 					Name: "test",
 				},
+				UserDataSecret: &corev1.LocalObjectReference{
+					Name: userDataSecretName,
+				},
 			},
 		},
 		{
@@ -1315,7 +1381,8 @@ func TestCreate(t *testing.T) {
 			client := fake.NewFakeClientWithScheme(scheme.Scheme,
 				credentialsSecret,
 				configMap,
-				infra)
+				infra,
+				userDataSecret)
 
 			rawProviderSpec, err := vsphereapi.RawExtensionFromProviderSpec(&tc.providerSpec)
 			if err != nil {