From 526031f5925a8ff1be2047e16b8380be277ecdea Mon Sep 17 00:00:00 2001 From: Bryan Venteicher Date: Thu, 23 Jan 2025 19:25:42 -0600 Subject: [PATCH] Handle invalid VC Session in GetVirtualMachine() If the VC session is invalid - like because the VC creds had rotated - GetVirtualMachine() could incorrectly return that the VM does not exist. If the lookup is being done to in response to the k8s VM being deleted, we would not actually delete the VC VM. While here, improve how looking up by the MoID was done: we don't need to use the Finder instead just use the PropertyCollector. The old Finder lookup would always return an error if the MoID doesn't exist, which means that we could get stuck if the VM actually didn't exist. This would really only be an issue of we removed the VC VM but failed to remove our finalizer. --- pkg/providers/vsphere/vcenter/getvm.go | 48 +++++++++----- pkg/providers/vsphere/vcenter/getvm_test.go | 71 ++++++++++++++++++++- pkg/providers/vsphere/vmprovider.go | 2 +- 3 files changed, 103 insertions(+), 18 deletions(-) diff --git a/pkg/providers/vsphere/vcenter/getvm.go b/pkg/providers/vsphere/vcenter/getvm.go index 2e7b89c11..42540df65 100644 --- a/pkg/providers/vsphere/vcenter/getvm.go +++ b/pkg/providers/vsphere/vcenter/getvm.go @@ -5,27 +5,37 @@ package vcenter import ( + "errors" "fmt" - "github.com/vmware/govmomi/find" + "github.com/vmware/govmomi/fault" "github.com/vmware/govmomi/object" + "github.com/vmware/govmomi/property" "github.com/vmware/govmomi/vim25" + "github.com/vmware/govmomi/vim25/mo" vimtypes "github.com/vmware/govmomi/vim25/types" pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context" ) +type getVMNotFoundError struct{} + +func (n getVMNotFoundError) Error() string { + return "vm not found" +} + // GetVirtualMachine gets the VM from VC, either by the Instance UUID, BIOS UUID, or MoID. func GetVirtualMachine( vmCtx pkgctx.VirtualMachineContext, vimClient *vim25.Client, - datacenter *object.Datacenter, - finder *find.Finder) (*object.VirtualMachine, error) { + datacenter *object.Datacenter) (*object.VirtualMachine, error) { // Find by Instance UUID. if id := vmCtx.VM.UID; id != "" { if vm, err := findVMByUUID(vmCtx, vimClient, datacenter, string(id), true); err == nil { return vm, nil + } else if !errors.Is(err, getVMNotFoundError{}) { + return nil, err } } @@ -33,13 +43,17 @@ func GetVirtualMachine( if id := vmCtx.VM.Spec.BiosUUID; id != "" { if vm, err := findVMByUUID(vmCtx, vimClient, datacenter, id, false); err == nil { return vm, nil + } else if !errors.Is(err, getVMNotFoundError{}) { + return nil, err } } // Find by MoRef. if id := vmCtx.VM.Status.UniqueID; id != "" { - if vm, err := findVMByMoID(vmCtx, finder, id); err == nil { + if vm, err := findVMByMoID(vmCtx, vimClient, id); err == nil { return vm, nil + } else if !errors.Is(err, getVMNotFoundError{}) { + return nil, err } } @@ -48,21 +62,25 @@ func GetVirtualMachine( func findVMByMoID( vmCtx pkgctx.VirtualMachineContext, - finder *find.Finder, + vimClient *vim25.Client, moID string) (*object.VirtualMachine, error) { - ref, err := finder.ObjectReference(vmCtx, vimtypes.ManagedObjectReference{Type: "VirtualMachine", Value: moID}) - if err != nil { - return nil, err + moRef := vimtypes.ManagedObjectReference{ + Type: "VirtualMachine", + Value: moID, } - vm, ok := ref.(*object.VirtualMachine) - if !ok { - return nil, fmt.Errorf("found VM reference was not a VM but a %T", ref) + vm := mo.VirtualMachine{} + if err := property.DefaultCollector(vimClient).RetrieveOne(vmCtx, moRef, []string{"name"}, &vm); err != nil { + var f *vimtypes.ManagedObjectNotFound + if _, ok := fault.As(err, &f); ok { + return nil, getVMNotFoundError{} + } + return nil, fmt.Errorf("error retreiving VM via MoID: %w", err) } - vmCtx.Logger.V(4).Info("Found VM via MoID", "path", vm.InventoryPath, "moID", moID) - return vm, nil + vmCtx.Logger.V(4).Info("Found VM via MoID", "moID", moID) + return object.NewVirtualMachine(vimClient, moRef), nil } func findVMByUUID( @@ -74,9 +92,9 @@ func findVMByUUID( ref, err := object.NewSearchIndex(vimClient).FindByUuid(vmCtx, datacenter, uuid, true, &isInstanceUUID) if err != nil { - return nil, fmt.Errorf("error finding object by UUID %q: %w", uuid, err) + return nil, fmt.Errorf("error finding VM by UUID %q: %w", uuid, err) } else if ref == nil { - return nil, fmt.Errorf("no VM found for UUID %q (instanceUUID: %v)", uuid, isInstanceUUID) + return nil, getVMNotFoundError{} } vm, ok := ref.(*object.VirtualMachine) diff --git a/pkg/providers/vsphere/vcenter/getvm_test.go b/pkg/providers/vsphere/vcenter/getvm_test.go index 65246e333..b14c62cba 100644 --- a/pkg/providers/vsphere/vcenter/getvm_test.go +++ b/pkg/providers/vsphere/vcenter/getvm_test.go @@ -56,11 +56,23 @@ func getVM() { }) It("returns success", func() { - vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder) + vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter) Expect(err).ToNot(HaveOccurred()) Expect(vm).ToNot(BeNil()) Expect(vm.Reference().Value).To(Equal(vmCtx.VM.Status.UniqueID)) }) + + Context("VC client is logged out", func() { + BeforeEach(func() { + Expect(ctx.VCClient.Logout(ctx)).To(Succeed()) + }) + + It("returns error", func() { + vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter) + Expect(err).To(HaveOccurred()) + Expect(vm).To(BeNil()) + }) + }) }) Context("Gets VM by UUID", func() { @@ -74,9 +86,64 @@ func getVM() { }) It("returns success", func() { - vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter, ctx.Finder) + vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter) + Expect(err).ToNot(HaveOccurred()) + Expect(vm).ToNot(BeNil()) + }) + + Context("VC client is logged out", func() { + BeforeEach(func() { + Expect(ctx.VCClient.Logout(ctx)).To(Succeed()) + }) + + It("returns error", func() { + vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter) + Expect(err).To(HaveOccurred()) + Expect(vm).To(BeNil()) + }) + }) + }) + + Context("Gets VM by BiosUUID", func() { + BeforeEach(func() { + vm, err := ctx.Finder.VirtualMachine(ctx, vcVMName) + Expect(err).ToNot(HaveOccurred()) + + var o mo.VirtualMachine + Expect(vm.Properties(ctx, vm.Reference(), nil, &o)).To(Succeed()) + vmCtx.VM.Spec.BiosUUID = o.Config.Uuid + }) + + It("returns success", func() { + vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter) Expect(err).ToNot(HaveOccurred()) Expect(vm).ToNot(BeNil()) }) + + Context("VC client is logged out", func() { + BeforeEach(func() { + Expect(ctx.VCClient.Logout(ctx)).To(Succeed()) + }) + + It("returns error", func() { + vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter) + Expect(err).To(HaveOccurred()) + Expect(vm).To(BeNil()) + }) + }) + }) + + Context("VM does not exist", func() { + BeforeEach(func() { + vmCtx.VM.UID = "bogus-uid" + vmCtx.VM.Spec.BiosUUID = "bogus-bios-uuid" + vmCtx.VM.Status.UniqueID = "bogus-moid" + }) + + It("returns success with nil vm", func() { + vm, err := vcenter.GetVirtualMachine(vmCtx, ctx.VCClient.Client, ctx.Datacenter) + Expect(err).ToNot(HaveOccurred()) + Expect(vm).To(BeNil()) + }) }) } diff --git a/pkg/providers/vsphere/vmprovider.go b/pkg/providers/vsphere/vmprovider.go index b56c8b553..d71225261 100644 --- a/pkg/providers/vsphere/vmprovider.go +++ b/pkg/providers/vsphere/vmprovider.go @@ -356,7 +356,7 @@ func (vs *vSphereVMProvider) getVM( client *vcclient.Client, notFoundReturnErr bool) (*object.VirtualMachine, error) { - vcVM, err := vcenter.GetVirtualMachine(vmCtx, client.VimClient(), client.Datacenter(), client.Finder()) + vcVM, err := vcenter.GetVirtualMachine(vmCtx, client.VimClient(), client.Datacenter()) if err != nil { return nil, err }