diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 641e462408..5cd6de84dc 100644 --- a/pkg/controller/controller.go +++ b/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, diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 2d4c286678..3a332ee2b7 100644 --- a/pkg/controller/controller_test.go +++ b/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() }