Pass VolumeAttributesClass Parameters as MutableParameters to CreateVolume

Signed-off-by: Connor Catlett <conncatl@amazon.com>

Author: ConnorJC3

pkg/controller/controller.go

@@ -217,8 +217,9 @@ type ProvisionerCSITranslator interface {
 
 // requiredCapabilities provides a set of extra capabilities required for special/optional features provided by a plugin
 type requiredCapabilities struct {
-	snapshot bool
-	clone    bool
+	snapshot     bool
+	clone        bool
+	modifyVolume bool
 }
 
 // NodeDeployment contains additional parameters for running external-provisioner alongside a
@@ -440,6 +441,13 @@ func (p *csiProvisioner) checkDriverCapabilities(rc *requiredCapabilities) error
 			return fmt.Errorf("CSI driver does not support clone operations: controller CLONE_VOLUME capability is not reported")
 		}
 	}
+	if rc.modifyVolume {
+		// Check whether plugin supports modifying volumes
+		// If not, PVCs with an associated VolumeAttributesClass cannot be created
+		if !p.controllerCapabilities[csi.ControllerServiceCapability_RPC_MODIFY_VOLUME] {
+			return fmt.Errorf("CSI driver does not support VolumeAttributesClass: controller MODIFY_VOLUME capability is not reported")
+		}
+	}
 
 	return nil
 }
@@ -596,6 +604,14 @@ func (p *csiProvisioner) prepareProvision(ctx context.Context, claim *v1.Persist
 		}
 	}
 
+	vacName := claim.Spec.VolumeAttributesClassName
+	// TODO: For reviewers - Should we reject volumes that have a VAC specified if the VAC feature gate is disabled?
+	// This will be a change in behavior (existing versions of external-provisioner out in the wild will just let those PVCs get provisioned),
+	// but it could prevent provisioning a PVC in a misleading way
+	if utilfeature.DefaultFeatureGate.Enabled(features.VolumeAttributesClass) && vacName != nil {
+		rc.modifyVolume = true
+	}
+
 	if err := p.checkDriverCapabilities(rc); err != nil {
 		return nil, controller.ProvisioningFinished, err
 	}
@@ -742,6 +758,19 @@ func (p *csiProvisioner) prepareProvision(ctx context.Context, claim *v1.Persist
 		deletionAnnSecrets.namespace = provisionerSecretRef.Namespace
 	}
 
+	if utilfeature.DefaultFeatureGate.Enabled(features.VolumeAttributesClass) && vacName != nil {
+		vac, err := p.client.StorageV1alpha1().VolumeAttributesClasses().Get(ctx, *vacName, metav1.GetOptions{})
+		if err != nil {
+			return nil, controller.ProvisioningNoChange, err
+		}
+
+		if vac.DriverName != p.driverName {
+			return nil, controller.ProvisioningFinished, fmt.Errorf("VAC %s referenced in PVC is for driver %s which does not match driver name %s", *vacName, vac.DriverName, p.driverName)
+		}
+
+		req.MutableParameters = vac.Parameters
+	}
+
 	return &prepareProvisionResult{
 		fsType:              fsType,
 		migratedVolume:      migratedVolume,

pkg/controller/controller_test.go

@@ -33,6 +33,7 @@ import (
 	"google.golang.org/grpc/status"
 	v1 "k8s.io/api/core/v1"
 	storagev1 "k8s.io/api/storage/v1"
+	storagev1alpha1 "k8s.io/api/storage/v1alpha1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -42,6 +43,7 @@ import (
 	"k8s.io/client-go/kubernetes"
 	fakeclientset "k8s.io/client-go/kubernetes/fake"
 	k8stesting "k8s.io/client-go/testing"
+	"k8s.io/component-base/featuregate"
 	utilfeaturetesting "k8s.io/component-base/featuregate/testing"
 	csitrans "k8s.io/csi-translation-lib"
 	"k8s.io/klog/v2"
@@ -504,6 +506,15 @@ func provisionFromPVCCapabilities() (rpc.PluginCapabilitySet, rpc.ControllerCapa
 		}
 }
 
+func provisionWithVACCapabilities() (rpc.PluginCapabilitySet, rpc.ControllerCapabilitySet) {
+	return rpc.PluginCapabilitySet{
+			csi.PluginCapability_Service_CONTROLLER_SERVICE: true,
+		}, rpc.ControllerCapabilitySet{
+			csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME: true,
+			csi.ControllerServiceCapability_RPC_MODIFY_VOLUME:        true,
+		}
+}
+
 var fakeSCName = "fake-test-sc"
 
 func createFakeNamedPVC(requestBytes int64, name string, userAnnotations map[string]string) *v1.PersistentVolumeClaim {
@@ -543,6 +554,13 @@ func createFakePVCWithVolumeMode(requestBytes int64, volumeMode v1.PersistentVol
 	return claim
 }
 
+// createFakePVCWithVAC returns PVC with VolumeAttributesClassName
+func createFakePVCWithVAC(requestBytes int64, vacName string) *v1.PersistentVolumeClaim {
+	claim := createFakePVC(requestBytes)
+	claim.Spec.VolumeAttributesClassName = &vacName
+	return claim
+}
+
 // fakeClaim returns a valid PVC with the requested settings
 func fakeClaim(name, namespace, claimUID string, capacity int64, boundToVolume string, phase v1.PersistentVolumeClaimPhase, class *string, mode string) *v1.PersistentVolumeClaim {
 	claim := v1.PersistentVolumeClaim{
@@ -820,28 +838,29 @@ func TestGetSecretReference(t *testing.T) {
 }
 
 type provisioningTestcase struct {
-	capacity                      int64 // if zero, default capacity, otherwise available bytes
-	volOpts                       controller.ProvisionOptions
-	notNilSelector                bool
-	makeVolumeNameErr             bool
-	getSecretRefErr               bool
-	getCredentialsErr             bool
-	volWithLessCap                bool
-	volWithZeroCap                bool
-	expectedPVSpec                *pvSpec
-	clientSetObjects              []runtime.Object
-	createVolumeError             error
-	expectErr                     bool
-	expectState                   controller.ProvisioningState
-	expectCreateVolDo             func(t *testing.T, ctx context.Context, req *csi.CreateVolumeRequest)
-	withExtraMetadata             bool
-	skipCreateVolume              bool
-	deploymentNode                string // fake distributed provisioning with this node as host
-	immediateBinding              bool   // enable immediate binding support for distributed provisioning
-	expectSelectedNode            string // a specific selected-node of the PVC in the apiserver after the test, same as before if empty
-	expectNoProvision             bool   // if true, then ShouldProvision should return false
-	supportsSingleNodeMultiWriter bool   // if true, then provision with single node multi writer capabilities
-	controllerPublishReadOnly     bool
+	capacity                  int64 // if zero, default capacity, otherwise available bytes
+	volOpts                   controller.ProvisionOptions
+	notNilSelector            bool
+	makeVolumeNameErr         bool
+	getSecretRefErr           bool
+	getCredentialsErr         bool
+	volWithLessCap            bool
+	volWithZeroCap            bool
+	expectedPVSpec            *pvSpec
+	clientSetObjects          []runtime.Object
+	createVolumeError         error
+	expectErr                 bool
+	expectState               controller.ProvisioningState
+	expectCreateVolDo         func(t *testing.T, ctx context.Context, req *csi.CreateVolumeRequest)
+	withExtraMetadata         bool
+	skipCreateVolume          bool
+	deploymentNode            string // fake distributed provisioning with this node as host
+	immediateBinding          bool   // enable immediate binding support for distributed provisioning
+	expectSelectedNode        string // a specific selected-node of the PVC in the apiserver after the test, same as before if empty
+	expectNoProvision         bool   // if true, then ShouldProvision should return false
+	controllerPublishReadOnly bool
+	featureGates              map[featuregate.Feature]bool
+	pluginCapabilities        func() (rpc.PluginCapabilitySet, rpc.ControllerCapabilitySet)
 }
 
 type provisioningFSTypeTestcase struct {
@@ -1403,7 +1422,7 @@ func provisionTestcases() (int64, map[string]provisioningTestcase) {
 			expectState: controller.ProvisioningFinished,
 		},
 		"provision with access mode single node writer and single node multi writer capability": {
-			supportsSingleNodeMultiWriter: true,
+			pluginCapabilities: provisionWithSingleNodeMultiWriterCapabilities,
 			volOpts: controller.ProvisionOptions{
 				StorageClass: &storagev1.StorageClass{
 					ReclaimPolicy: &deletePolicy,
@@ -1456,7 +1475,7 @@ func provisionTestcases() (int64, map[string]provisioningTestcase) {
 			expectState: controller.ProvisioningFinished,
 		},
 		"provision with access mode single node single writer and single node multi writer capability": {
-			supportsSingleNodeMultiWriter: true,
+			pluginCapabilities: provisionWithSingleNodeMultiWriterCapabilities,
 			volOpts: controller.ProvisionOptions{
 				StorageClass: &storagev1.StorageClass{
 					ReclaimPolicy: &deletePolicy,
@@ -2282,6 +2301,110 @@ func provisionTestcases() (int64, map[string]provisioningTestcase) {
 			expectNoProvision:  true, // not owner and will not change that either
 			expectSelectedNode: "",   // not changed by ShouldProvision
 		},
+		"normal provision with VolumeAttributesClass": {
+			featureGates: map[featuregate.Feature]bool{
+				features.VolumeAttributesClass: true,
+			},
+			pluginCapabilities: provisionWithVACCapabilities,
+			clientSetObjects: []runtime.Object{&storagev1alpha1.VolumeAttributesClass{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-vac",
+				},
+				DriverName: driverName,
+				Parameters: map[string]string{
+					"test-param": "from-vac",
+				},
+			}},
+			expectCreateVolDo: func(t *testing.T, ctx context.Context, req *csi.CreateVolumeRequest) {
+				// TODO: define a constant for this? kind of ugly
+				if !reflect.DeepEqual(req.MutableParameters, map[string]string{"test-param": "from-vac"}) {
+					t.Errorf("Missing or incorrect VolumeAttributeClass (mutable) parameters")
+				}
+			},
+			volOpts: controller.ProvisionOptions{
+				StorageClass: &storagev1.StorageClass{
+					ReclaimPolicy: &deletePolicy,
+					Parameters: map[string]string{
+						"fstype":     "ext3",
+						"test-param": "from-sc",
+					},
+				},
+				PVName: "test-name",
+				PVC:    createFakePVCWithVAC(requestedBytes, "test-vac"),
+			},
+			expectState: controller.ProvisioningFinished,
+		},
+		"fail with VolumeAttributesClass but driver does not support MODIFY_VOLUME": {
+			featureGates: map[featuregate.Feature]bool{
+				features.VolumeAttributesClass: true,
+			},
+			volOpts: controller.ProvisionOptions{
+				StorageClass: &storagev1.StorageClass{
+					ReclaimPolicy: &deletePolicy,
+					Parameters: map[string]string{
+						"fstype": "ext3",
+					},
+				},
+				PVName: "test-name",
+				PVC:    createFakePVCWithVAC(requestedBytes, "test-vac"),
+			},
+			expectErr:   true,
+			expectState: controller.ProvisioningFinished,
+		},
+		"fail with VolumeAttributesClass but VAC does not exist": {
+			featureGates: map[featuregate.Feature]bool{
+				features.VolumeAttributesClass: true,
+			},
+			pluginCapabilities: provisionWithVACCapabilities,
+			clientSetObjects: []runtime.Object{&storagev1alpha1.VolumeAttributesClass{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-vac",
+				},
+				DriverName: driverName,
+				Parameters: map[string]string{
+					"test-param": "from-vac",
+				},
+			}},
+			volOpts: controller.ProvisionOptions{
+				StorageClass: &storagev1.StorageClass{
+					ReclaimPolicy: &deletePolicy,
+					Parameters: map[string]string{
+						"fstype": "ext3",
+					},
+				},
+				PVName: "test-name",
+				PVC:    createFakePVCWithVAC(requestedBytes, "test-vac-does-not-exist"),
+			},
+			expectErr:   true,
+			expectState: controller.ProvisioningNoChange,
+		},
+		"fail with VolumeAttributesClass but driver name does not match": {
+			featureGates: map[featuregate.Feature]bool{
+				features.VolumeAttributesClass: true,
+			},
+			pluginCapabilities: provisionWithVACCapabilities,
+			clientSetObjects: []runtime.Object{&storagev1alpha1.VolumeAttributesClass{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-vac",
+				},
+				DriverName: "not-" + driverName,
+				Parameters: map[string]string{
+					"test-param": "from-vac",
+				},
+			}},
+			volOpts: controller.ProvisionOptions{
+				StorageClass: &storagev1.StorageClass{
+					ReclaimPolicy: &deletePolicy,
+					Parameters: map[string]string{
+						"fstype": "ext3",
+					},
+				},
+				PVName: "test-name",
+				PVC:    createFakePVCWithVAC(requestedBytes, "test-vac"),
+			},
+			expectErr:   true,
+			expectState: controller.ProvisioningFinished,
+		},
 	}
 }
 
@@ -2414,6 +2537,10 @@ func runFSTypeProvisionTest(t *testing.T, k string, tc provisioningFSTypeTestcas
 }
 
 func runProvisionTest(t *testing.T, tc provisioningTestcase, requestedBytes int64, provisionDriverName, supportsMigrationFromInTreePluginName string, testProvision bool) {
+	for featureName, featureValue := range tc.featureGates {
+		defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, featureName, featureValue)()
+	}
+
 	tmpdir := tempDir(t)
 	defer os.RemoveAll(tmpdir)
 	mockController, driver, _, controllerServer, csiConn, err := createMockServer(t, tmpdir)
@@ -2524,8 +2651,8 @@ func runProvisionTest(t *testing.T, tc provisioningTestcase, requestedBytes int6
 
 	var pluginCaps rpc.PluginCapabilitySet
 	var controllerCaps rpc.ControllerCapabilitySet
-	if tc.supportsSingleNodeMultiWriter {
-		pluginCaps, controllerCaps = provisionWithSingleNodeMultiWriterCapabilities()
+	if tc.pluginCapabilities != nil {
+		pluginCaps, controllerCaps = tc.pluginCapabilities()
 	} else {
 		pluginCaps, controllerCaps = provisionCapabilities()
 	}