Skip to content

Commit

Permalink
Pass VolumeAttributesClass Parameters as MutableParameters to CreateV…
Browse files Browse the repository at this point in the history 
…olume

Signed-off-by: Connor Catlett <[email protected]>
  • Loading branch information
@ConnorJC3
ConnorJC3 committed Oct 26, 2023
1 parent e9897f5 commit e72843d
Show file tree
Hide file tree
Showing 2 changed files with 184 additions and 28 deletions.
33 changes: 31 additions & 2 deletions pkg/controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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,
Expand Down
179 changes: 153 additions & 26 deletions pkg/controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
},
}
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
}
Expand Down

0 comments on commit e72843d

Please sign in to comment.