Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1843327: [vSphere] Improve provider spec validation #605

Merged
merged 1 commit into from
Jul 4, 2020
Merged

Bug 1843327: [vSphere] Improve provider spec validation #605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions pkg/controller/vsphere/actuator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,22 @@ func TestMachineEvents(t *testing.T) {
g.Expect(k8sClient.Delete(context.Background(), infra)).To(Succeed())
}()

userDataSecretName := "vsphere-ignition"
userDataSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: userDataSecretName,
Namespace: testNamespaceName,
},
Data: map[string][]byte{
userDataSecretKey: []byte("{}"),
},
}

g.Expect(k8sClient.Create(context.Background(), userDataSecret)).To(Succeed())
defer func() {
g.Expect(k8sClient.Delete(context.Background(), userDataSecret)).To(Succeed())
}()

createTagAndCategory(session, "CLUSTERID", "CLUSTERID")

ctx := context.Background()
Expand Down Expand Up @@ -241,6 +257,9 @@ func TestMachineEvents(t *testing.T) {
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
UserDataSecret: &corev1.LocalObjectReference{
Name: userDataSecretName,
},
})
gs.Expect(err).ToNot(HaveOccurred())

Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/vsphere/machine_scope.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I think this is fine and it makes sense as we don't currently support any model where this could be empty fwiw I don't think we are doing the same in other providers.
In the near future with a model where MCO generate secrets for machines on demand this might need to change.
openshift/enhancements#368
openshift/enhancements#201

}

userDataSecret := &apicorev1.Secret{}
Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/vsphere/machine_scope_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}
Expand Down
21 changes: 13 additions & 8 deletions pkg/controller/vsphere/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,9 @@ func clone(s *machineScope) (string, error) {

vmTemplate, err := s.GetSession().FindVM(*s, "", s.providerSpec.Template)
if err != nil {
return "", err
const 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
Expand Down Expand Up @@ -500,24 +502,24 @@ func clone(s *machineScope) (string, error) {

folder, err := s.GetSession().Finder.FolderOrDefault(s, folderPath)
if err != nil {
multipleFoundMsg := "multiple folders found, specify one in config"
notFoundMsg := "folder not found, specify valid value"
const multipleFoundMsg = "multiple folders found, specify one in config"
const notFoundMsg = "folder not found, specify valid value"
defaultError := fmt.Errorf("unable to get folder for %q: %w", folderPath, err)
return "", handleVSphereError(multipleFoundMsg, notFoundMsg, defaultError, err)
}

datastore, err := s.GetSession().Finder.DatastoreOrDefault(s, datastorePath)
if err != nil {
multipleFoundMsg := "multiple datastores found, specify one in config"
notFoundMsg := "datastore not found, specify valid value"
const multipleFoundMsg = "multiple datastores found, specify one in config"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this using const and := unconsistently??
https://github.com/openshift/machine-api-operator/pull/605/files#diff-11b6610f82e5d47fdfb8dda44b90dd1eR469
https://github.com/openshift/machine-api-operator/pull/605/files#diff-11b6610f82e5d47fdfb8dda44b90dd1eR470
https://github.com/openshift/machine-api-operator/pull/605/files#diff-11b6610f82e5d47fdfb8dda44b90dd1eR594
https://github.com/openshift/machine-api-operator/pull/605/files#diff-11b6610f82e5d47fdfb8dda44b90dd1eR595

const notFoundMsg = "datastore not found, specify valid value"
defaultError := fmt.Errorf("unable to get datastore for %q: %w", datastorePath, err)
return "", handleVSphereError(multipleFoundMsg, notFoundMsg, defaultError, err)
}

resourcepool, err := s.GetSession().Finder.ResourcePoolOrDefault(s, resourcepoolPath)
if err != nil {
multipleFoundMsg := "multiple resource pools found, specify one in config"
notFoundMsg := "resource pool not found, specify valid value"
const multipleFoundMsg = "multiple resource pools found, specify one in config"
const notFoundMsg = "resource pool not found, specify valid value"
defaultError := fmt.Errorf("unable to get resource pool for %q: %w", resourcepool, err)
return "", handleVSphereError(multipleFoundMsg, notFoundMsg, defaultError, err)
}
Expand Down Expand Up @@ -639,7 +641,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)
const multipleFoundMsg = "multiple networks found, specify one in config"
const 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)
Expand Down
75 changes: 71 additions & 4 deletions pkg/controller/vsphere/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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"),
},
Expand All @@ -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 {
Expand Down Expand Up @@ -195,6 +223,10 @@ func TestClone(t *testing.T) {
Server: server.URL.Host,
Folder: "invalid",
},
Template: vm.Name,
UserDataSecret: &corev1.LocalObjectReference{
Name: userDataSecretName,
},
},
},
{
Expand All @@ -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 {
Expand Down Expand Up @@ -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"),
},
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -1264,6 +1327,9 @@ func TestCreate(t *testing.T) {
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
UserDataSecret: &corev1.LocalObjectReference{
Name: userDataSecretName,
},
},
},
{
Expand Down Expand Up @@ -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 {
Expand Down