Add friendly image name support in v1a2 and improve duplicate check

docs/concepts/images/vm-image.md

@@ -44,7 +44,7 @@ For example, if `vmi-0a0044d7c690bcbea` refers to an image with a friendly name
 * There is no other `VirtualMachineImage` in the same namespace with that friendly name.
 * There is no other `ClusterVirtualMachineImage` with the same friendly name.
 
-If the friendly name unambiguously resolves to the distinct, VM image `vmi-0a0044d7c690bcbea`, then a mutation webhook replaces `spec.imageName: photonos-5-x64` with `spec.imageName: vmi-0a0044d7c690bcbea`.
+If the friendly name unambiguously resolves to the distinct, VM image `vmi-0a0044d7c690bcbea`, then a mutation webhook replaces `spec.imageName: photonos-5-x64` with `spec.imageName: vmi-0a0044d7c690bcbea`. If the friendly name resolves to multiple or no VM images, then the mutation webhook denies the request and outputs an error message accordingly.
 
 
 ## Recommended Images

webhooks/virtualmachine/v1alpha1/mutation/virtualmachine_mutator.go

@@ -199,6 +199,7 @@ func ResolveImageName(
 		return false, nil
 	}
 
+	var determinedImageName string
 	// Check if a single namespace scope image exists by the status name.
 	vmiList := &vmopv1.VirtualMachineImageList{}
 	if err := c.List(ctx, vmiList, client.InNamespace(vm.Namespace),
@@ -208,9 +209,13 @@ func ResolveImageName(
 	); err != nil {
 		return false, err
 	}
-	if len(vmiList.Items) == 1 {
-		vm.Spec.ImageName = vmiList.Items[0].Name
-		return true, nil
+	switch len(vmiList.Items) {
+	case 0:
+		break
+	case 1:
+		determinedImageName = vmiList.Items[0].Name
+	default:
+		return false, errors.Errorf("multiple VM images exist for %q in namespace scope", imageName)
 	}
 
 	// Check if a single cluster scope image exists by the status name.
@@ -220,12 +225,24 @@ func ResolveImageName(
 	}); err != nil {
 		return false, err
 	}
-	if len(cvmiList.Items) == 1 {
-		vm.Spec.ImageName = cvmiList.Items[0].Name
-		return true, nil
+	switch len(cvmiList.Items) {
+	case 0:
+		break
+	case 1:
+		if determinedImageName != "" {
+			return false, errors.Errorf("multiple VM images exist for %q in namespace and cluster scope", imageName)
+		}
+		determinedImageName = cvmiList.Items[0].Name
+	default:
+		return false, errors.Errorf("multiple VM images exist for %q in cluster scope", imageName)
+	}
+
+	if determinedImageName == "" {
+		return false, errors.Errorf("no VM image exists for %q in namespace or cluster scope", imageName)
 	}
 
-	return false, errors.Errorf("no single VM image exists for %q", imageName)
+	vm.Spec.ImageName = determinedImageName
+	return true, nil
 }
 
 // SetNextRestartTime sets spec.nextRestartTime for a VM if the field's

webhooks/virtualmachine/v1alpha1/mutation/virtualmachine_mutator_intg_test.go

@@ -182,7 +182,7 @@ func intgTestsMutating() {
 
 			When("Creating VirtualMachine", func() {
 
-				When("When VM ImageName is already a vmi resource name", func() {
+				When("VM ImageName is already a vmi resource name", func() {
 
 					BeforeEach(func() {
 						vm.Spec.ImageName = "vmi-123"

webhooks/virtualmachine/v1alpha1/mutation/virtualmachine_mutator_unit_test.go

@@ -248,6 +248,10 @@ func unitTestsMutating() {
 	})
 
 	Describe(("ResolveImageName"), func() {
+		const (
+			dupImageStatusName    = "dup-status-name"
+			uniqueImageStatusName = "unique-status-name"
+		)
 
 		BeforeEach(func() {
 			// Replace the client with a fake client that has the index of VM images.
@@ -279,62 +283,77 @@ func unitTestsMutating() {
 			})
 
 			It("Should not mutate ImageName", func() {
-				oldVM := ctx.vm.DeepCopy()
 				mutated, err := mutation.ResolveImageName(&ctx.WebhookRequestContext, ctx.Client, ctx.vm)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(mutated).To(BeFalse())
-				Expect(ctx.vm.Spec.ImageName).Should(Equal(oldVM.Spec.ImageName))
+				Expect(ctx.vm.Spec.ImageName).Should(Equal("vmi-xxx"))
 			})
 		})
 
 		Context("When VM ImageName is set to a status name matching multiple namespace scope images", func() {
 
 			BeforeEach(func() {
-				dupStatusName := "dup-status-name"
 				vmi1 := builder.DummyVirtualMachineImage("vmi-1")
-				vmi1.Status.ImageName = dupStatusName
+				vmi1.Status.ImageName = dupImageStatusName
 				vmi2 := builder.DummyVirtualMachineImage("vmi-2")
-				vmi2.Status.ImageName = dupStatusName
+				vmi2.Status.ImageName = dupImageStatusName
 				Expect(ctx.Client.Create(ctx, vmi1)).To(Succeed())
 				Expect(ctx.Client.Create(ctx, vmi2)).To(Succeed())
-				ctx.vm.Spec.ImageName = dupStatusName
+				ctx.vm.Spec.ImageName = dupImageStatusName
 			})
 
 			It("Should return an error", func() {
 				_, err := mutation.ResolveImageName(&ctx.WebhookRequestContext, ctx.Client, ctx.vm)
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(Equal("no single VM image exists for \"dup-status-name\""))
+				Expect(err.Error()).To(Equal("multiple VM images exist for \"dup-status-name\" in namespace scope"))
 			})
 		})
 
 		Context("When VM ImageName is set to a status name matching multiple cluster scope images", func() {
 
 			BeforeEach(func() {
-				dupStatusName := "dup-status-name"
 				cvmi1 := builder.DummyClusterVirtualMachineImage("cvmi-1")
-				cvmi1.Status.ImageName = dupStatusName
+				cvmi1.Status.ImageName = dupImageStatusName
 				cvmi2 := builder.DummyClusterVirtualMachineImage("cvmi-2")
-				cvmi2.Status.ImageName = dupStatusName
+				cvmi2.Status.ImageName = dupImageStatusName
 				Expect(ctx.Client.Create(ctx, cvmi1)).To(Succeed())
 				Expect(ctx.Client.Create(ctx, cvmi2)).To(Succeed())
-				ctx.vm.Spec.ImageName = dupStatusName
+				ctx.vm.Spec.ImageName = dupImageStatusName
+			})
+
+			It("Should return an error", func() {
+				_, err := mutation.ResolveImageName(&ctx.WebhookRequestContext, ctx.Client, ctx.vm)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(Equal("multiple VM images exist for \"dup-status-name\" in cluster scope"))
+			})
+		})
+
+		Context("When VM ImageName is set to a status name matching one namespace and one cluster scope images", func() {
+
+			BeforeEach(func() {
+				vmi := builder.DummyVirtualMachineImage("vmi-123")
+				vmi.Status.ImageName = dupImageStatusName
+				cvmi := builder.DummyClusterVirtualMachineImage("cvmi-123")
+				cvmi.Status.ImageName = dupImageStatusName
+				Expect(ctx.Client.Create(ctx, vmi)).To(Succeed())
+				Expect(ctx.Client.Create(ctx, cvmi)).To(Succeed())
+				ctx.vm.Spec.ImageName = dupImageStatusName
 			})
 
 			It("Should return an error", func() {
 				_, err := mutation.ResolveImageName(&ctx.WebhookRequestContext, ctx.Client, ctx.vm)
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(Equal("no single VM image exists for \"dup-status-name\""))
+				Expect(err.Error()).To(Equal("multiple VM images exist for \"dup-status-name\" in namespace and cluster scope"))
 			})
 		})
 
 		Context("When VM ImageName is set to a status name matching a single namespace scope image", func() {
 
 			BeforeEach(func() {
-				uniqueStatusName := "unique-status-name"
 				vmi := builder.DummyVirtualMachineImage("vmi-123")
-				vmi.Status.ImageName = uniqueStatusName
+				vmi.Status.ImageName = uniqueImageStatusName
 				Expect(ctx.Client.Create(ctx, vmi)).To(Succeed())
-				ctx.vm.Spec.ImageName = uniqueStatusName
+				ctx.vm.Spec.ImageName = uniqueImageStatusName
 			})
 
 			It("Should mutate ImageName to the resource name of the namespace scope image", func() {
@@ -348,11 +367,10 @@ func unitTestsMutating() {
 		Context("When VM ImageName is set to a status name matching a single cluster scope image", func() {
 
 			BeforeEach(func() {
-				uniqueStatusName := "unique-status-name"
 				cvmi := builder.DummyClusterVirtualMachineImage("vmi-123")
-				cvmi.Status.ImageName = uniqueStatusName
+				cvmi.Status.ImageName = uniqueImageStatusName
 				Expect(ctx.Client.Create(ctx, cvmi)).To(Succeed())
-				ctx.vm.Spec.ImageName = uniqueStatusName
+				ctx.vm.Spec.ImageName = uniqueImageStatusName
 			})
 
 			It("Should mutate ImageName to the resource name of the cluster scope image", func() {

webhooks/virtualmachine/v1alpha2/mutation/virtualmachine_mutator.go

@@ -22,6 +22,7 @@ import (
 	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha2"
 	"github.com/vmware-tanzu/vm-operator/pkg/builder"
 	"github.com/vmware-tanzu/vm-operator/pkg/context"
+	"github.com/vmware-tanzu/vm-operator/pkg/lib"
 )
 
 const (
@@ -31,9 +32,36 @@ const (
 // +kubebuilder:webhook:path=/default-mutate-vmoperator-vmware-com-v1alpha2-virtualmachine,mutating=true,failurePolicy=fail,groups=vmoperator.vmware.com,resources=virtualmachines,verbs=create;update,versions=v1alpha2,name=default.mutating.virtualmachine.v1alpha2.vmoperator.vmware.com,sideEffects=None,admissionReviewVersions=v1;v1beta1
 // +kubebuilder:rbac:groups=vmoperator.vmware.com,resources=virtualmachine,verbs=get;list
 // +kubebuilder:rbac:groups=vmoperator.vmware.com,resources=virtualmachine/status,verbs=get
+// +kubebuilder:rbac:groups=vmoperator.vmware.com,resources=virtualmachineimages,verbs=get;list;watch
+// +kubebuilder:rbac:groups=vmoperator.vmware.com,resources=virtualmachineimages/status,verbs=get;list;watch
+// +kubebuilder:rbac:groups=vmoperator.vmware.com,resources=clustervirtualmachineimages,verbs=get;list;watch
+// +kubebuilder:rbac:groups=vmoperator.vmware.com,resources=clustervirtualmachineimages/status,verbs=get;list;watch
 
 // AddToManager adds the webhook to the provided manager.
 func AddToManager(ctx *context.ControllerManagerContext, mgr ctrlmgr.Manager) error {
+	// Index the VirtualMachineImage and ClusterVirtualMachineImage objects by
+	// status.name field to allow efficient querying in ResolveImageName().
+	if err := mgr.GetFieldIndexer().IndexField(
+		ctx,
+		&vmopv1.VirtualMachineImage{},
+		"status.name",
+		func(rawObj client.Object) []string {
+			vmi := rawObj.(*vmopv1.VirtualMachineImage)
+			return []string{vmi.Status.Name}
+		}); err != nil {
+		return err
+	}
+	if err := mgr.GetFieldIndexer().IndexField(
+		ctx,
+		&vmopv1.ClusterVirtualMachineImage{},
+		"status.name",
+		func(rawObj client.Object) []string {
+			cvmi := rawObj.(*vmopv1.ClusterVirtualMachineImage)
+			return []string{cvmi.Status.Name}
+		}); err != nil {
+		return err
+	}
+
 	hook, err := builder.NewMutatingWebhook(ctx, mgr, webHookName, NewMutator(mgr.GetClient()))
 	if err != nil {
 		return errors.Wrapf(err, "failed to create mutation webhook")
@@ -70,8 +98,13 @@ func (m mutator) Mutate(ctx *context.WebhookRequestContext) admission.Response {
 	original := vm
 	modified := original.DeepCopy()
 
-	//nolint:gocritic
 	switch ctx.Op {
+	case admissionv1.Create:
+		if mutated, err := ResolveImageName(ctx, m.client, modified); err != nil {
+			return admission.Denied(err.Error())
+		} else if mutated {
+			wasMutated = true
+		}
 	case admissionv1.Update:
 		oldVM, err := m.vmFromUnstructured(ctx.OldObj)
 		if err != nil {
@@ -142,3 +175,62 @@ func SetNextRestartTime(
 		newVM.Spec.NextRestartTime,
 		`may only be set to "now"`)
 }
+
+// ResolveImageName mutates the vm.spec.imageName if it's not set to a vmi name
+// and there is a single namespace or cluster scope image with that status name.
+func ResolveImageName(
+	ctx *context.WebhookRequestContext,
+	c client.Client,
+	vm *vmopv1.VirtualMachine) (bool, error) {
+	// Return early if the VM image name is empty or already set to a vmi name.
+	imageName := vm.Spec.ImageName
+	if imageName == "" || !lib.IsWCPVMImageRegistryEnabled() ||
+		strings.HasPrefix(imageName, "vmi-") {
+		return false, nil
+	}
+
+	var determinedImageName string
+	// Check if a single namespace scope image exists by the status name.
+	vmiList := &vmopv1.VirtualMachineImageList{}
+	if err := c.List(ctx, vmiList, client.InNamespace(vm.Namespace),
+		client.MatchingFields{
+			"status.name": imageName,
+		},
+	); err != nil {
+		return false, err
+	}
+	switch len(vmiList.Items) {
+	case 0:
+		break
+	case 1:
+		determinedImageName = vmiList.Items[0].Name
+	default:
+		return false, errors.Errorf("multiple VM images exist for %q in namespace scope", imageName)
+	}
+
+	// Check if a single cluster scope image exists by the status name.
+	cvmiList := &vmopv1.ClusterVirtualMachineImageList{}
+	if err := c.List(ctx, cvmiList, client.MatchingFields{
+		"status.name": imageName,
+	}); err != nil {
+		return false, err
+	}
+	switch len(cvmiList.Items) {
+	case 0:
+		break
+	case 1:
+		if determinedImageName != "" {
+			return false, errors.Errorf("multiple VM images exist for %q in namespace and cluster scope", imageName)
+		}
+		determinedImageName = cvmiList.Items[0].Name
+	default:
+		return false, errors.Errorf("multiple VM images exist for %q in cluster scope", imageName)
+	}
+
+	if determinedImageName == "" {
+		return false, errors.Errorf("no VM image exists for %q in namespace or cluster scope", imageName)
+	}
+
+	vm.Spec.ImageName = determinedImageName
+	return true, nil
+}

webhooks/virtualmachine/v1alpha2/mutation/virtualmachine_mutator_intg_test.go

@@ -4,6 +4,7 @@
 package mutation_test
 
 import (
+	"os"
 	"time"
 
 	. "github.com/onsi/ginkgo"
@@ -14,6 +15,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha2"
+	"github.com/vmware-tanzu/vm-operator/pkg/lib"
 	"github.com/vmware-tanzu/vm-operator/test/builder"
 )
 
@@ -148,4 +150,32 @@ func intgTestsMutating() {
 			})
 		})
 	})
+
+	Context("ResolveImageName", func() {
+
+		BeforeEach(func() {
+			Expect(os.Setenv(lib.VMImageRegistryFSS, lib.TrueString)).To(Succeed())
+		})
+
+		AfterEach(func() {
+			Expect(os.Unsetenv(lib.VMImageRegistryFSS)).To(Succeed())
+		})
+
+		When("Creating VirtualMachine", func() {
+
+			When("VM ImageName is already a vmi resource name", func() {
+
+				BeforeEach(func() {
+					ctx.vm.Spec.ImageName = "vmi-123"
+				})
+
+				It("Should not mutate ImageName", func() {
+					Expect(ctx.Client.Create(ctx, ctx.vm)).To(Succeed())
+					modified := &vmopv1.VirtualMachine{}
+					Expect(ctx.Client.Get(ctx, client.ObjectKeyFromObject(ctx.vm), modified)).Should(Succeed())
+					Expect(modified.Spec.ImageName).Should(Equal("vmi-123"))
+				})
+			})
+		})
+	})
 }