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

Jump to bottom

Bug 1838504: [vSphere] Fail machine on invalid provider spec values #593

Merged
merged 1 commit into from
May 21, 2020
Merged

Bug 1838504: [vSphere] Fail machine on invalid provider spec values #593

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 26 additions & 14 deletions pkg/controller/vsphere/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,28 +456,26 @@ func clone(s *machineScope) (string, error) {

folder, err := s.GetSession().Finder.FolderOrDefault(s, folderPath)
if err != nil {
return "", fmt.Errorf("unable to get folder for %q: %w", folderPath, err)
multipleFoundMsg := "multiple folders found, specify one in config"
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 {
return "", fmt.Errorf("unable to get datastore for %q: %w", datastorePath, err)
multipleFoundMsg := "multiple datastores found, specify one in config"
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 {
// TODO: move error checks to provider spec validation
var multipleFoundError *find.MultipleFoundError
if errors.As(err, &multipleFoundError) {
return "", machinecontroller.InvalidMachineConfiguration("multiple resource pools found, specify one in config")
}

var notFoundError *find.NotFoundError
if errors.As(err, &notFoundError) {
return "", machinecontroller.InvalidMachineConfiguration("resource pool not found, specify valid value")
}

return "", fmt.Errorf("unable to get resource pool for %q: %w", resourcepool, err)
multipleFoundMsg := "multiple resource pools found, specify one in config"
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)
}

numCPUs := s.providerSpec.NumCPUs
Expand Down Expand Up @@ -677,6 +675,20 @@ func setProviderStatus(taskRef string, condition vspherev1.VSphereMachineProvide
return nil
}

func handleVSphereError(multipleFoundMsg, notFoundMsg string, defaultError, vsphereError error) error {
var multipleFoundError *find.MultipleFoundError
if errors.As(vsphereError, &multipleFoundError) {
return machinecontroller.InvalidMachineConfiguration(multipleFoundMsg)
}

var notFoundError *find.NotFoundError
if errors.As(vsphereError, &notFoundError) {
return machinecontroller.InvalidMachineConfiguration(notFoundMsg)
}

return defaultError
}

type virtualMachine struct {
context.Context
Ref types.ManagedObjectReference
Expand Down
283 changes: 193 additions & 90 deletions pkg/controller/vsphere/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"encoding/base64"
"errors"
"fmt"
"io/ioutil"
"reflect"
"testing"

Expand Down Expand Up @@ -77,7 +78,7 @@ func initSimulator(t *testing.T) (*simulator.Model, *session.Session, *simulator
}

func TestClone(t *testing.T) {
model, _, server := initSimulator(t)
model, session, server := initSimulator(t)
defer model.Remove()
defer server.Close()
credentialsSecretUsername := fmt.Sprintf("%s.username", server.URL.Host)
Expand All @@ -98,128 +99,230 @@ func TestClone(t *testing.T) {
}

testCases := []struct {
testCase string
machine func(t *testing.T) *machinev1.Machine
expectError bool
cloneVM bool
testCase string
cloneVM bool
expectedError error
setupFailureCondition func() error
providerSpec vsphereapi.VSphereMachineProviderSpec
machineName string
}{
{
testCase: "clone machine from default values",
machine: func(t *testing.T) *machinev1.Machine {
providerSpec := vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
},
providerSpec: vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
},
},
cloneVM: true,
machineName: "test0",
},
{
testCase: "clone machine in specific folder",
providerSpec: vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
Folder: "custom-folder",
},
},
cloneVM: true,
machineName: "test1",
},
{
testCase: "fail on invalid resource pool",
providerSpec: vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
ResourcePool: "invalid",
},
},
expectedError: errors.New("resource pool not found, specify valid value"),
},
{
testCase: "fail on multiple resource pools",
providerSpec: vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
ResourcePool: "/DC0/host/DC0_C0/Resources/...",
},
},
expectedError: errors.New("multiple resource pools found, specify one in config"),
setupFailureCondition: func() error {
// Create resource pools
defaultResourcePool, err := session.Finder.ResourcePool(context.Background(), "/DC0/host/DC0_C0/Resources")
Copy link
Member

Choose a reason for hiding this comment

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

is this magic value "/DC0/host/DC0_C0/Resources" completely arbitrary? may be add a comment explaining where is it coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not a magic value, just the default value that 'simulator' has set. I had to debug this a bit.

if err != nil {
return err
}
raw, err := vsphereapi.RawExtensionFromProviderSpec(&providerSpec)
spec := types.DefaultResourceConfigSpec()
_, err = defaultResourcePool.Create(context.Background(), "resourcePool1", spec)
if err != nil {
t.Fatal(err)
return err
}
return &machinev1.Machine{
ObjectMeta: metav1.ObjectMeta{
UID: "1",
Name: "defaultFolder",
Namespace: namespace,
},
Spec: machinev1.MachineSpec{
ProviderSpec: machinev1.ProviderSpec{
Value: raw,
},
},
_, err = defaultResourcePool.Create(context.Background(), "resourcePool2", spec)
if err != nil {
return err
}
return nil
},
expectError: false,
cloneVM: true,
},
{
testCase: "does not clone machine if folder does not exist",
machine: func(t *testing.T) *machinev1.Machine {
providerSpec := vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
Folder: "does-not-exists",
},
testCase: "fail on invalid folder",
expectedError: errors.New("folder not found, specify valid value"),
providerSpec: vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
Folder: "invalid",
},
},
},
{
testCase: "fail on multiple folders",
providerSpec: vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
Folder: "/DC0/vm/...",
},
},
expectedError: errors.New("multiple folders found, specify one in config"),
setupFailureCondition: func() error {
// Create folders
defaultFolder, err := session.Finder.Folder(context.Background(), "/DC0/vm")
if err != nil {
return err
}
raw, err := vsphereapi.RawExtensionFromProviderSpec(&providerSpec)
_, err = defaultFolder.CreateFolder(context.Background(), "folder1")
if err != nil {
t.Fatal(err)
return err
}
return &machinev1.Machine{
ObjectMeta: metav1.ObjectMeta{
UID: "2",
Name: "missingFolder",
Namespace: namespace,
},
Spec: machinev1.MachineSpec{
ProviderSpec: machinev1.ProviderSpec{
Value: raw,
},
},
_, err = defaultFolder.CreateFolder(context.Background(), "folder2")
if err != nil {
return err
}
return nil
},
expectError: true,
},
{
testCase: "clone machine in specific folder",
machine: func(t *testing.T) *machinev1.Machine {
providerSpec := vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},

Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
Folder: "custom-folder",
},
testCase: "fail on invalid datastore",
providerSpec: vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
Datastore: "invalid",
},
},
expectedError: errors.New("datastore not found, specify valid value"),
},
{
testCase: "fail on multiple datastores",
providerSpec: vsphereapi.VSphereMachineProviderSpec{
CredentialsSecret: &corev1.LocalObjectReference{
Name: "test",
},
Workspace: &vsphereapi.Workspace{
Server: server.URL.Host,
Datastore: "/DC0/...",
},
},
expectedError: errors.New("multiple datastores found, specify one in config"),
setupFailureCondition: func() error {
// Create datastores
hostSystem, err := session.Finder.HostSystem(context.Background(), "/DC0/host/DC0_C0/DC0_C0_H0")
if err != nil {
return err
}
raw, err := vsphereapi.RawExtensionFromProviderSpec(&providerSpec)
dss, err := hostSystem.ConfigManager().DatastoreSystem(context.Background())
if err != nil {
t.Fatal(err)
return err
}
return &machinev1.Machine{
ObjectMeta: metav1.ObjectMeta{
UID: "3",
Name: "customFolder",
Namespace: namespace,
},
Spec: machinev1.MachineSpec{
ProviderSpec: machinev1.ProviderSpec{
Value: raw,
},
},
dir, err := ioutil.TempDir("", fmt.Sprintf("tmpdir"))
if err != nil {
return err
}
_, err = dss.CreateLocalDatastore(context.Background(), "datastore1", dir)
if err != nil {
return err
}
_, err = dss.CreateLocalDatastore(context.Background(), "datastore2", dir)
if err != nil {
return err
}
return nil
},
expectError: false,
cloneVM: true,
},
}

for _, tc := range testCases {
t.Run(tc.testCase, func(t *testing.T) {
machineScope, err := newMachineScope(machineScopeParams{
client: fake.NewFakeClientWithScheme(scheme.Scheme, &credentialsSecret),
if tc.setupFailureCondition != nil {
if err := tc.setupFailureCondition(); err != nil {
t.Fatal(err)
}
}

machineScope := machineScope{
Context: context.TODO(),
machine: tc.machine(t),
})
if err != nil {
t.Fatal(err)
machine: &machinev1.Machine{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "test",
Labels: map[string]string{
machinev1.MachineClusterIDLabel: "CLUSTERID",
},
},
},
providerSpec: &tc.providerSpec,
session: session,
providerStatus: &vsphereapi.VSphereMachineProviderStatus{},
client: fake.NewFakeClientWithScheme(scheme.Scheme, &credentialsSecret),
}

machineScope.providerSpec.Template = vm.Name

if _, err := clone(machineScope); (err != nil) != tc.expectError {
t.Errorf("Got: %v. Expected: %v", err, tc.expectError)
if tc.machineName != "" {
machineScope.machine.Name = tc.machineName
}
if tc.cloneVM {
model.Machine++

taskRef, err := clone(&machineScope)

if tc.expectedError != nil {
if taskRef != "" {
t.Fatalf("task reference was expected to be empty, got: %s", taskRef)
}
if err == nil {
t.Fatal("clone() was expected to return error")
}
if err.Error() != tc.expectedError.Error() {
t.Fatalf("expected: %v, got %v", tc.expectedError, err)
}
} else {
if err != nil {
t.Fatalf("clone() was not expected to return error: %v", err)
}
}
if model.Machine != model.Count().Machine {
t.Errorf("Unexpected number of machines. Expected: %v, got: %v", model.Machine, model.Count().Machine)

if tc.cloneVM {
if taskRef == "" {
t.Fatal("task reference was not expected to be empty")
}
}
})
}
Expand Down