Allow devops users to attach GC RWX file volume with VM service VMs (#3611)

Author: skogta

manifests/supervisorcluster/1.29/cns-csi.yaml

@@ -732,6 +732,9 @@ rules:
   - apiGroups: ["cns.vmware.com"]
     resources: ["cnsfileaccessconfigs"]
     verbs: ["get", "list", "update"]
+  - apiGroups: ["vmoperator.vmware.com"]
+    resources: ["virtualmachines"]
+    verbs: ["get", "list"]
 ---
 kind: ClusterRoleBinding
 apiVersion: rbac.authorization.k8s.io/v1

manifests/supervisorcluster/1.30/cns-csi.yaml

@@ -737,6 +737,9 @@ rules:
   - apiGroups: ["cns.vmware.com"]
     resources: ["cnsfileaccessconfigs"]
     verbs: ["get", "list", "update"]
+  - apiGroups: ["vmoperator.vmware.com"]
+    resources: ["virtualmachines"]
+    verbs: ["get", "list"]
 ---
 kind: ClusterRoleBinding
 apiVersion: rbac.authorization.k8s.io/v1

manifests/supervisorcluster/1.31/cns-csi.yaml

@@ -737,6 +737,9 @@ rules:
   - apiGroups: ["cns.vmware.com"]
     resources: ["cnsfileaccessconfigs"]
     verbs: ["get", "list", "update"]
+  - apiGroups: ["vmoperator.vmware.com"]
+    resources: ["virtualmachines"]
+    verbs: ["get", "list"]
 ---
 kind: ClusterRoleBinding
 apiVersion: rbac.authorization.k8s.io/v1

manifests/supervisorcluster/1.32/cns-csi.yaml

@@ -737,6 +737,9 @@ rules:
   - apiGroups: ["cns.vmware.com"]
     resources: ["cnsfileaccessconfigs"]
     verbs: ["get", "list", "update"]
+  - apiGroups: ["vmoperator.vmware.com"]
+    resources: ["virtualmachines"]
+    verbs: ["get", "list"]
 ---
 kind: ClusterRoleBinding
 apiVersion: rbac.authorization.k8s.io/v1

pkg/syncer/admissionhandler/cnscsi_admissionhandler.go

@@ -13,6 +13,7 @@ import (
 	"strconv"
 	"strings"
 
+	vmoperatortypes "github.com/vmware-tanzu/vm-operator/api/v1alpha5"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common/commonco/k8sorchestrator"
 
 	admissionv1 "k8s.io/api/admission/v1"
@@ -29,9 +30,12 @@ import (
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common/commonco"
 
+	apitypes "k8s.io/apimachinery/pkg/types"
 	cnsfileaccessconfigv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsfileaccessconfig/v1alpha1"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/crypto"
+	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/utils"
 	"sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/logger"
+	k8s "sigs.k8s.io/vsphere-csi-driver/v3/pkg/kubernetes"
 )
 
 const (
@@ -40,8 +44,8 @@ const (
 	DefaultWebhookPort               = 9883
 	DefaultWebhookMetricsBindAddress = "0"
 	devopsUserLabelKey               = "cns.vmware.com/user-created"
-	vmNameLabelKey                   = "cns.vmware.com/vm-name"
-	pvcNameLabelKey                  = "cns.vmware.com/pvc-name"
+	vmUIDLabelKey                    = "cns.vmware.com/vm-uid"
+	pvcUIDLabelKey                   = "cns.vmware.com/pvc-uid"
 )
 
 var (
@@ -248,6 +252,71 @@ func (h *CSISupervisorMutationWebhook) Handle(ctx context.Context, req admission
 	return admission.Allowed("")
 }
 
+// getVmUID returns the VM UID for the given VM
+func getVmUID(ctx context.Context,
+	vmName string, namespace string) (string, error) {
+	log := logger.GetLogger(ctx)
+
+	restClientConfig, err := k8s.GetKubeConfig(ctx)
+	if err != nil {
+		log.Errorf("failed to initialize rest clientconfig. Error: %s", err)
+		return "", err
+	}
+
+	vmOperatorClient, err := k8s.NewClientForGroup(ctx, restClientConfig, vmoperatortypes.GroupName)
+	if err != nil {
+		log.Error("failed to initialize vmOperatorClient. Error: %s", err)
+		return "", err
+	}
+
+	vm, _, err := getVirtualMachine(ctx, vmOperatorClient, vmName, namespace)
+	if err != nil {
+		log.Error("failed to get virtualmachine. Error: %s", err)
+		return "", err
+	}
+
+	log.Infof("Found UID %s for VM %s", string(vm.ObjectMeta.UID), vm.Name)
+	return string(vm.ObjectMeta.UID), nil
+}
+
+// getVirtualMachine returns the VM object for the given vmName and namespace.
+func getVirtualMachine(ctx context.Context, vmOperatorClient client.Client,
+	vmName string, namespace string) (*vmoperatortypes.VirtualMachine, string, error) {
+	log := logger.GetLogger(ctx)
+	vmKey := apitypes.NamespacedName{
+		Namespace: namespace,
+		Name:      vmName,
+	}
+	virtualMachine, apiVersion, err := utils.GetVirtualMachineAllApiVersions(ctx,
+		vmKey, vmOperatorClient)
+	if err != nil {
+		log.Error("failed to get virtualmachine instance for the VM with name: %q. Error: %+v", vmName, err)
+		return nil, apiVersion, err
+	}
+	return virtualMachine, apiVersion, nil
+}
+
+// getPVCUID retrieves the UID of a PersistentVolumeClaim by name and namespace.
+func getPVCUID(ctx context.Context, pvcName, namespace string) (string, error) {
+	log := logger.GetLogger(ctx)
+
+	k8sClient, err := k8s.NewClient(ctx)
+	if err != nil {
+		log.Errorf("failed to create k8s client. Errror: %s", err)
+		return "", err
+	}
+
+	pvc, err := k8sClient.CoreV1().PersistentVolumeClaims(namespace).Get(ctx, pvcName,
+		v1.GetOptions{})
+	if err != nil {
+		log.Errorf("failed to obtain PVC %s. Errror: %s", pvcName, err)
+		return "", err
+	}
+
+	log.Infof("Found UID %s for PVC %s", string(pvc.UID), pvcName)
+	return string(pvc.UID), nil
+}
+
 // mutateNewCnsFileAccessConfig adds devops label on a CnsFileAccessConfig CR
 // if it is being created by a devops user.
 func (h *CSISupervisorMutationWebhook) mutateNewCnsFileAccessConfig(ctx context.Context,
@@ -275,12 +344,26 @@ func (h *CSISupervisorMutationWebhook) mutateNewCnsFileAccessConfig(ctx context.
 		newCnsFileAccessConfig.Labels = make(map[string]string)
 	}
 
+	// Obtain VM's UID
+	vmUID, err := getVmUID(ctx, newCnsFileAccessConfig.Spec.VMName, newCnsFileAccessConfig.Namespace)
+	if err != nil {
+		log.Errorf("faield to get VM UID for VM %s. Err: %s", newCnsFileAccessConfig.Spec.VMName, err)
+		return admission.Errored(http.StatusInternalServerError, err)
+	}
+
+	// Obtain PVC's UID
+	pvcUID, err := getPVCUID(ctx, newCnsFileAccessConfig.Spec.PvcName, newCnsFileAccessConfig.Namespace)
+	if err != nil {
+		log.Errorf("faield to get PVC UID for PVC %s. Err: %s", newCnsFileAccessConfig.Spec.PvcName, err)
+		return admission.Errored(http.StatusInternalServerError, err)
+	}
+
 	// Add VM name and PVC name label.
 	// If someone created this CR with these labels already present, CSI will overrite on them
 	// with the correct values.
 	newCnsFileAccessConfig.Labels[devopsUserLabelKey] = "true"
-	newCnsFileAccessConfig.Labels[vmNameLabelKey] = newCnsFileAccessConfig.Spec.VMName
-	newCnsFileAccessConfig.Labels[pvcNameLabelKey] = newCnsFileAccessConfig.Spec.PvcName
+	newCnsFileAccessConfig.Labels[vmUIDLabelKey] = vmUID
+	newCnsFileAccessConfig.Labels[pvcUIDLabelKey] = pvcUID
 
 	newRawCnsFileAccessConfig, err := json.Marshal(newCnsFileAccessConfig)
 	if err != nil {

pkg/syncer/admissionhandler/validate_cnsfileaccessconfig.go

@@ -108,8 +108,23 @@ func cnsFileAccessConfigAlreadyExists(ctx context.Context, clientConfig *rest.Co
 		return "", err
 	}
 
+	// Obtain VM's UID.
+	vmUID, err := getVmUID(ctx, vm, namespace)
+	if err != nil {
+		log.Errorf("failed to get VM UID for VM %s. Err: %s", vm, err)
+		return "", err
+	}
+
+	// Obtain PVC's UID
+	pvcUID, err := getPVCUID(ctx, pvc, namespace)
+	if err != nil {
+		log.Errorf("failed to get PVC UID for PVC %s. Err: %s", pvc, err)
+		return "", err
+	}
+
 	// List only that CnsFileAccessConfig CRs which has the same VM name and PVC name labels.
-	labelSelector := labels.SelectorFromSet(labels.Set{vmNameLabelKey: vm, pvcNameLabelKey: pvc})
+	labelSelector := labels.SelectorFromSet(labels.Set{vmUIDLabelKey: vmUID, pvcUIDLabelKey: pvcUID})
+
 	// Get the list of all CnsFileAccessConfig CRs in the given namespace.
 	cnsFileAccessConfigList := &cnsfileaccessconfigv1alpha1.CnsFileAccessConfigList{}
 	err = cnsOperatorClient.List(ctx, cnsFileAccessConfigList, &client.ListOptions{

pkg/syncer/cnsoperator/controller/cnsfileaccessconfig/cnsfileaccessconfig_controller.go

@@ -62,10 +62,7 @@ import (
 
 const (
 	defaultMaxWorkerThreadsForFileAccessConfig = 10
-	vmNameLabelKey                             = "cns.vmware.com/vm-name"
-	pvcNameLabelKey                            = "cns.vmware.com/pvc-name"
 	capvVmLabelKey                             = "capv.vmware.com"
-	capvPvcLabelKey                            = "TKGService"
 	devopsUserLabelKey                         = "cns.vmware.com/user-created"
 )
 
@@ -822,12 +819,10 @@ func (r *ReconcileCnsFileAccessConfig) getVMExternalIP(ctx context.Context,
 }
 
 // validateVmAndPvc validates if the VM and PVC combination given in the instance is correct or not.
-// CnsFileAccessConfig CRs created by devpos users will have "cns.vmware.com/user-created", "cns.vmware.com/vm-name"
-// and "cns.vmware.com/pvc-name" labels.
-// When these labels are present, the PVC and VM must not belong to TKG cluster.
+// CnsFileAccessConfig CRs created by devpos users will have "cns.vmware.com/user-created" label.
+// When this labels are present, the VM must not belong to TKG cluster.
 // This is verified by ensuring that:
 // The VM does not have a label applied by CAPV - example capv.vmware.com/cluster.name.
-// The PVC does not have a label applied by CAPV - example <TKG cluster namespace>/TKGService
 func validateVmAndPvc(ctx context.Context, instanceLabels map[string]string, instanceName string, pvcName string,
 	namespace string, client client.Client, vm *vmoperatortypes.VirtualMachine) error {
 	log := logger.GetLogger(ctx)
@@ -860,23 +855,6 @@ func validateVmAndPvc(ctx context.Context, instanceLabels map[string]string, ins
 		}
 	}
 
-	pvc := &v1.PersistentVolumeClaim{}
-	err := client.Get(ctx, types.NamespacedName{Name: pvcName, Namespace: namespace}, pvc)
-	if err != nil {
-		log.Errorf("failed to get PVC with name %s in namespace %s", pvcName, namespace)
-		return err
-	}
-
-	for key := range pvc.Labels {
-		if strings.Contains(key, capvPvcLabelKey) {
-			msg := fmt.Sprintf("CnsFileAccessConfig is created by devops user and has "+
-				"TKG PVC %s. Invalid combination.", pvcName)
-			log.Errorf(msg)
-			err := errors.New(msg)
-			return err
-		}
-	}
-
 	log.Infof("Successfully verified instance %s for VM/PVC combination", instanceName)
 	return nil
 }

pkg/syncer/cnsoperator/controller/cnsfileaccessconfig/cnsfileaccessconfig_controller_test.go

@@ -4,90 +4,65 @@ import (
 	"context"
 	"testing"
 
-	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-
+	"github.com/stretchr/testify/assert"
 	vmoperatortypes "github.com/vmware-tanzu/vm-operator/api/v1alpha5"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func TestValidateVmAndPvc(t *testing.T) {
-	scheme := runtime.NewScheme()
-	_ = v1.AddToScheme(scheme)
-	_ = vmoperatortypes.AddToScheme(scheme)
+func TestValidateVmAndPvc_NoLabels(t *testing.T) {
+	ctx := context.Background()
 
-	ctx := context.TODO()
+	err := validateVmAndPvc(ctx, nil, "vm-1", "pvc-1", "ns-1", nil, &vmoperatortypes.VirtualMachine{})
+	assert.NoError(t, err)
+}
 
-	tests := []struct {
-		name           string
-		instanceLabels map[string]string
-		vmLabels       map[string]string
-		expectError    bool
-		setupPVC       bool
-	}{
-		{
-			name:           "no instance labels",
-			instanceLabels: nil,
-			expectError:    false,
-		},
-		{
-			name: "labels but not devops user",
-			instanceLabels: map[string]string{
-				"random": "value",
-			},
-			expectError: false,
-		},
-		{
-			name: "devops user with capv label on VM",
-			instanceLabels: map[string]string{
-				devopsUserLabelKey: "true",
-			},
-			vmLabels: map[string]string{
-				capvVmLabelKey + "/cluster": "my-cluster",
-			},
-			expectError: true,
-		},
-		{
-			name: "valid input - no errors",
-			instanceLabels: map[string]string{
-				devopsUserLabelKey: "true",
-			},
-			setupPVC:    true,
-			expectError: false,
-		},
+func TestValidateVmAndPvc_NotDevOpsUser(t *testing.T) {
+	ctx := context.Background()
+
+	labels := map[string]string{
+		"other.label": "value",
 	}
 
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			var objects []runtime.Object
+	err := validateVmAndPvc(ctx, labels, "vm-1", "pvc-1", "ns-1", nil, &vmoperatortypes.VirtualMachine{})
+	assert.NoError(t, err)
+}
+
+func TestValidateVmAndPvc_DevOpsUser_ValidVM(t *testing.T) {
+	ctx := context.Background()
 
-			vm := &vmoperatortypes.VirtualMachine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:   "my-vm",
-					Labels: test.vmLabels,
-				},
-			}
+	labels := map[string]string{
+		devopsUserLabelKey: "true",
+	}
 
-			if test.setupPVC {
-				pvc := &v1.PersistentVolumeClaim{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "my-pvc",
-						Namespace: "default",
-					},
-				}
-				objects = append(objects, pvc)
-			}
+	vm := &vmoperatortypes.VirtualMachine{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   "my-vm",
+			Labels: map[string]string{"custom.label": "value"},
+		},
+	}
 
-			client := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(objects...).Build()
+	err := validateVmAndPvc(ctx, labels, "vm-1", "pvc-1", "ns-1", nil, vm)
+	assert.NoError(t, err)
+}
 
-			err := validateVmAndPvc(ctx, test.instanceLabels, "my-instance", "my-pvc", "default", client, vm)
+func TestValidateVmAndPvc_DevOpsUser_InvalidVM(t *testing.T) {
+	ctx := context.Background()
 
-			if test.expectError && err == nil {
-				t.Errorf("expected error but got nil")
-			} else if !test.expectError && err != nil {
-				t.Errorf("unexpected error: %v", err)
-			}
-		})
+	labels := map[string]string{
+		devopsUserLabelKey: "true",
 	}
+
+	vm := &vmoperatortypes.VirtualMachine{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-vm",
+			Labels: map[string]string{
+				capvVmLabelKey + "/machine": "value", // Contains capvVmLabelKey
+			},
+		},
+	}
+
+	err := validateVmAndPvc(ctx, labels, "vm-1", "pvc-1", "ns-1", nil, vm)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "Invalid combination")
+	assert.Contains(t, err.Error(), "my-vm")
 }