validate and merge storage controller configurations (#1315)

Add validation and merging logic for storage controller
configurations from VM Class, VM Image (OVF), and user-specified
hardware spec. The webhook validates conflicts during VM creation,
while the provider merges controllers during VM provisioning.

Controller conflicts are detected when controllers at the same bus
number have incompatible properties (e.g., different SCSI
SharingMode or controller subtypes).

This prevents runtime errors by catching incompatible controller
configurations early and ensures proper controller merging during
VM creation.

Author: zhangheliu

pkg/providers/vsphere/vmprovider_vm.go

@@ -19,7 +19,6 @@ import (
 	"github.com/go-logr/logr"
 	"github.com/vmware/govmomi/fault"
 	"github.com/vmware/govmomi/object"
-	"github.com/vmware/govmomi/ovf"
 	"github.com/vmware/govmomi/pbm"
 	pbmtypes "github.com/vmware/govmomi/pbm/types"
 	"github.com/vmware/govmomi/property"
@@ -34,7 +33,6 @@ import (
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
-	"sigs.k8s.io/yaml"
 
 	imgregv1a1 "github.com/vmware-tanzu/image-registry-operator-api/api/v1alpha1"
 	imgregv1 "github.com/vmware-tanzu/image-registry-operator-api/api/v1alpha2"
@@ -2434,55 +2432,22 @@ func (vs *vSphereVMProvider) vmCreateGenConfigSpecImage(
 		}
 	}
 
-	var (
-		vmiCache    vmopv1.VirtualMachineImageCache
-		vmiCacheKey = ctrlclient.ObjectKey{
-			Namespace: pkgcfg.FromContext(vmCtx).PodNamespace,
-			Name:      pkgutil.VMIName(itemID),
-		}
-	)
-
-	// Get the VirtualMachineImageCache resource.
-	if err := vs.k8sClient.Get(
+	// Get and validate the VirtualMachineImageCache
+	vmiCache, err := GetVirtualMachineImageCache(
 		vmCtx,
-		vmiCacheKey,
-		&vmiCache); err != nil {
-
-		return fmt.Errorf(
-			"failed to get vmi cache object: %w, %w",
-			err,
-			pkgerr.VMICacheNotReadyError{Name: vmiCacheKey.Name})
-	}
-
-	// Check if the hardware is ready.
-	hardwareReadyCondition := pkgcnd.Get(
-		vmiCache,
-		vmopv1.VirtualMachineImageCacheConditionHardwareReady)
-
-	switch {
-	case hardwareReadyCondition != nil &&
-		hardwareReadyCondition.Status == metav1.ConditionFalse:
-
-		return fmt.Errorf(
-			"failed to get hardware: %s: %w",
-			hardwareReadyCondition.Message,
-			pkgerr.VMICacheNotReadyError{Name: vmiCache.Name})
-
-	case hardwareReadyCondition == nil,
-		hardwareReadyCondition.Status != metav1.ConditionTrue,
-		imageType == imageTypeOVF && vmiCache.Status.OVF == nil,
-		imageType == imageTypeOVF && vmiCache.Status.OVF.ProviderVersion != itemVersion:
-
-		return pkgerr.VMICacheNotReadyError{
-			Message: "hardware not ready",
-			Name:    vmiCacheKey.Name,
-		}
+		vs.k8sClient,
+		itemID,
+		itemVersion,
+		imageType,
+		pkgcfg.FromContext(vmCtx).PodNamespace)
+	if err != nil {
+		return err
 	}
 
 	var imgConfigSpec vimtypes.VirtualMachineConfigSpec
 
 	if imageType == imageTypeOVF {
-		cs, err := vs.getConfigSpecFromOVF(vmCtx, vmiCache)
+		cs, err := GetConfigSpecFromOVFCache(vmCtx, vs.k8sClient, vmiCache)
 		if err != nil {
 			return err
 		}
@@ -2538,11 +2503,47 @@ func (vs *vSphereVMProvider) vmCreateGenConfigSpecImage(
 		}
 	}
 
-	// Inherit the image's disks and their controllers.
-	pkgutil.CopyStorageControllersAndDisks(
-		&createArgs.ConfigSpec,
-		imgConfigSpec,
-		createArgs.StorageProfileID)
+	if !pkgcfg.FromContext(vmCtx).Features.VMSharedDisks {
+		// Inherit the image's disks and their controllers.
+		pkgutil.CopyStorageControllersAndDisks(
+			&createArgs.ConfigSpec,
+			imgConfigSpec,
+			createArgs.StorageProfileID)
+	} else {
+		dstKeyToSrcKey, controllersToRemove, err :=
+			pkgutil.ValidateStorageControllerCompatibility(
+				createArgs.ConfigSpec,
+				imgConfigSpec)
+
+		if err != nil {
+			return fmt.Errorf(
+				"%s: %w", pkgutil.ControllerConflictVMClassImage, err)
+		}
+
+		pkgutil.MergeStorageControllersAndDisks(
+			&createArgs.ConfigSpec,
+			imgConfigSpec,
+			dstKeyToSrcKey,
+			controllersToRemove,
+			createArgs.StorageProfileID)
+
+		specControllersConfigSpec :=
+			pkgutil.CreateUserStorageControllersConfigSpec(
+				vmCtx,
+				vmCtx.VM,
+				createArgs.ConfigSpec)
+
+		// DeviceChanges for spec controllers will be reconciled by the virtual
+		// controller reconciler after creation; hence, the controllers are only
+		// validated here.
+		if _, _, err := pkgutil.ValidateStorageControllerCompatibility(
+			specControllersConfigSpec,
+			createArgs.ConfigSpec); err != nil {
+
+			return fmt.Errorf(
+				"%s: %w", pkgutil.ControllerConflictVMClassImageUser, err)
+		}
+	}
 
 	if pkgcfg.FromContext(vmCtx).Features.AllDisksArePVCs {
 		if err := vs.vmCreateGenConfigSpecImagePVCDataSourceRefs(
@@ -2687,42 +2688,6 @@ func (vs *vSphereVMProvider) updateDiskDeviceFromPVC(
 	return nil
 }
 
-func (vs *vSphereVMProvider) getConfigSpecFromOVF(
-	vmCtx pkgctx.VirtualMachineContext,
-	vmiCache vmopv1.VirtualMachineImageCache) (vimtypes.VirtualMachineConfigSpec, error) {
-
-	var ovfConfigMap corev1.ConfigMap
-	if err := vs.k8sClient.Get(
-		vmCtx,
-		ctrlclient.ObjectKey{
-			Namespace: vmiCache.Namespace,
-			Name:      vmiCache.Status.OVF.ConfigMapName,
-		},
-		&ovfConfigMap); err != nil {
-
-		return vimtypes.VirtualMachineConfigSpec{}, fmt.Errorf(
-			"failed to get ovf configmap: %w, %w",
-			err,
-			pkgerr.VMICacheNotReadyError{Name: vmiCache.Name})
-	}
-
-	var ovfEnvelope ovf.Envelope
-	if err := yaml.Unmarshal(
-		[]byte(ovfConfigMap.Data["value"]), &ovfEnvelope); err != nil {
-
-		return vimtypes.VirtualMachineConfigSpec{},
-			fmt.Errorf("failed to unmarshal ovf yaml into envelope: %w", err)
-	}
-
-	configSpec, err := ovfEnvelope.ToConfigSpec()
-	if err != nil {
-		return vimtypes.VirtualMachineConfigSpec{},
-			fmt.Errorf("failed to transform ovf to config spec: %w", err)
-	}
-
-	return configSpec, nil
-}
-
 func (vs *vSphereVMProvider) getConfigSpecFromVM(
 	vmCtx pkgctx.VirtualMachineContext,
 	vmiCache vmopv1.VirtualMachineImageCache,

pkg/providers/vsphere/vmprovider_vm_utils.go

@@ -14,18 +14,23 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/vmware/govmomi/object"
+	"github.com/vmware/govmomi/ovf"
 	vimtypes "github.com/vmware/govmomi/vim25/types"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 
+	"sigs.k8s.io/yaml"
+
 	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha5"
 	"github.com/vmware-tanzu/vm-operator/pkg/conditions"
 	pkgcfg "github.com/vmware-tanzu/vm-operator/pkg/config"
 	pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context"
+	pkgerr "github.com/vmware-tanzu/vm-operator/pkg/errors"
 	"github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/constants"
+	"github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/virtualmachine"
 	"github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/vmlifecycle"
 	"github.com/vmware-tanzu/vm-operator/pkg/util"
 	"github.com/vmware-tanzu/vm-operator/pkg/util/cloudinit"
@@ -602,6 +607,139 @@ func GetVMClassConfigSpec(
 	return configSpec, nil
 }
 
+// GetVMClassConfigSpecFromClassName retrieves the VM Class config spec from a
+// VM Class identified by className and namespace. It returns the config spec
+// derived from either the VM Class's ConfigSpec field or from its hardware
+// devices.
+func GetVMClassConfigSpecFromClassName(
+	ctx context.Context,
+	k8sClient ctrlclient.Client,
+	className, namespace string,
+) (vimtypes.VirtualMachineConfigSpec, error) {
+
+	// For classless VMs, return empty config spec.
+	if className == "" {
+		return vimtypes.VirtualMachineConfigSpec{}, nil
+	}
+
+	vmClass := &vmopv1.VirtualMachineClass{}
+	vmClassKey := ctrlclient.ObjectKey{
+		Name:      className,
+		Namespace: namespace,
+	}
+
+	if err := k8sClient.Get(ctx, vmClassKey, vmClass); err != nil {
+		return vimtypes.VirtualMachineConfigSpec{}, fmt.Errorf(
+			"failed to get VM Class %s: %w", className, err)
+	}
+
+	// If the VM Class has a ConfigSpec, use the existing function to parse it.
+	if len(vmClass.Spec.ConfigSpec) > 0 {
+		return GetVMClassConfigSpec(ctx, vmClass.Spec.ConfigSpec)
+	}
+
+	// Otherwise, create config spec from VM Class devices.
+	return virtualmachine.ConfigSpecFromVMClassDevices(&vmClass.Spec), nil
+}
+
+// GetConfigSpecFromOVFCache retrieves the VM config spec from a
+// VirtualMachineImageCache for OVF images.
+func GetConfigSpecFromOVFCache(
+	ctx context.Context,
+	k8sClient ctrlclient.Client,
+	vmiCache vmopv1.VirtualMachineImageCache) (
+	vimtypes.VirtualMachineConfigSpec, error) {
+
+	var ovfConfigMap corev1.ConfigMap
+	if err := k8sClient.Get(
+		ctx,
+		ctrlclient.ObjectKey{
+			Namespace: vmiCache.Namespace,
+			Name:      vmiCache.Status.OVF.ConfigMapName,
+		},
+		&ovfConfigMap); err != nil {
+
+		return vimtypes.VirtualMachineConfigSpec{}, fmt.Errorf(
+			"failed to get ovf configmap: %w, %w",
+			err,
+			pkgerr.VMICacheNotReadyError{Name: vmiCache.Name})
+	}
+
+	var ovfEnvelope ovf.Envelope
+	if err := yaml.Unmarshal(
+		[]byte(ovfConfigMap.Data["value"]), &ovfEnvelope); err != nil {
+
+		return vimtypes.VirtualMachineConfigSpec{},
+			fmt.Errorf("failed to unmarshal ovf yaml into envelope: %w", err)
+	}
+
+	configSpec, err := ovfEnvelope.ToConfigSpec()
+	if err != nil {
+		return vimtypes.VirtualMachineConfigSpec{},
+			fmt.Errorf("failed to transform ovf to config spec: %w", err)
+	}
+
+	return configSpec, nil
+}
+
+// GetVirtualMachineImageCache retrieves and validates a
+// VirtualMachineImageCache resource. It fetches the cache object using the
+// provided itemID and podNamespace, then validates that the hardware is ready
+// by checking the HardwareReady condition. For OVF image types, it also
+// ensures the OVF status exists and the provider version matches the expected
+// itemVersion. Returns the validated cache object or an error if the cache is
+// not found or not ready.
+func GetVirtualMachineImageCache(
+	ctx context.Context,
+	k8sClient ctrlclient.Client,
+	itemID, itemVersion, imageType, podNamespace string,
+) (vmopv1.VirtualMachineImageCache, error) {
+
+	var (
+		vmiCache    vmopv1.VirtualMachineImageCache
+		vmiCacheKey = ctrlclient.ObjectKey{
+			Namespace: podNamespace,
+			Name:      util.VMIName(itemID),
+		}
+	)
+
+	// Get the VirtualMachineImageCache resource.
+	if err := k8sClient.Get(ctx, vmiCacheKey, &vmiCache); err != nil {
+		return vmopv1.VirtualMachineImageCache{},
+			fmt.Errorf("failed to get vmi cache object: %w, %w",
+				err,
+				pkgerr.VMICacheNotReadyError{Name: vmiCacheKey.Name})
+	}
+
+	// Check if the hardware is ready.
+	hardwareReadyCondition := conditions.Get(
+		vmiCache,
+		vmopv1.VirtualMachineImageCacheConditionHardwareReady)
+
+	switch {
+	case hardwareReadyCondition != nil &&
+		hardwareReadyCondition.Status == metav1.ConditionFalse:
+
+		return vmopv1.VirtualMachineImageCache{},
+			fmt.Errorf("failed to get hardware: %s: %w",
+				hardwareReadyCondition.Message,
+				pkgerr.VMICacheNotReadyError{Name: vmiCache.Name})
+
+	case hardwareReadyCondition == nil,
+		hardwareReadyCondition.Status != metav1.ConditionTrue,
+		imageType == "ovf" && vmiCache.Status.OVF == nil,
+		imageType == "ovf" && vmiCache.Status.OVF.ProviderVersion != itemVersion:
+
+		return vmopv1.VirtualMachineImageCache{},
+			pkgerr.VMICacheNotReadyError{
+				Message: "hardware not ready",
+				Name:    vmiCacheKey.Name,
+			}
+	}
+
+	return vmiCache, nil
+}
+
 // GetAttachedDiskUUIDToPVC returns a map of disk UUID to PVC object for all
 // attached disks by checking for volumes that have a PersistentVolumeClaim.
 func GetAttachedDiskUUIDToPVC(