diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index d2eb080b06..88ae3ffecf 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -444,8 +444,12 @@ func (p *csiProvisioner) Provision(options controller.ProvisionOptions) (*v1.Per rep := &csi.CreateVolumeResponse{} // Resolve provision secret credentials. - // No PVC is provided when resolving provision/delete secret names, since the PVC may or may not exist at delete time. - provisionerSecretRef, err := getSecretReference(provisionerSecretParams, options.StorageClass.Parameters, pvName, nil) + provisionerSecretRef, err := getSecretReference(provisionerSecretParams, options.StorageClass.Parameters, pvName, &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: options.PVC.Name, + Namespace: options.PVC.Namespace, + }, + }) if err != nil { return nil, err } @@ -684,18 +688,23 @@ func (p *csiProvisioner) Delete(volume *v1.PersistentVolume) error { if len(storageClassName) != 0 { if storageClass, err := p.client.StorageV1().StorageClasses().Get(storageClassName, metav1.GetOptions{}); err == nil { // Resolve provision secret credentials. - // No PVC is provided when resolving provision/delete secret names, since the PVC may or may not exist at delete time. - provisionerSecretRef, err := getSecretReference(provisionerSecretParams, storageClass.Parameters, volume.Name, nil) + provisionerSecretRef, err := getSecretReference(provisionerSecretParams, storageClass.Parameters, volume.Name, &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: volume.Spec.ClaimRef.Name, + Namespace: volume.Spec.ClaimRef.Namespace, + }, + }) if err != nil { return err } + credentials, err := getCredentials(p.client, provisionerSecretRef) if err != nil { - return err + // Continue with deletion, as the secret may have already been deleted. + klog.Errorf("Failed to get credentials for volume %s: %s", volume.Name, err.Error()) } req.Secrets = credentials } - } ctx, cancel := context.WithTimeout(context.Background(), p.timeout) defer cancel() diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 4dc42beef3..9b26a3b1f3 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -558,6 +558,20 @@ func TestGetSecretReference(t *testing.T) { pvc: nil, expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"}, }, + "simple - valid, pvc name and namespace": { + secretParams: provisionerSecretParams, + params: map[string]string{ + provisionerSecretNameKey: "param-name", + provisionerSecretNamespaceKey: "param-ns", + }, + pvc: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "ns", + }, + }, + expectRef: &v1.SecretReference{Name: "param-name", Namespace: "param-ns"}, + }, "simple - invalid name": { secretParams: nodePublishSecretParams, params: map[string]string{nodePublishSecretNameKey: "bad name", nodePublishSecretNamespaceKey: "ns"}, @@ -572,7 +586,20 @@ func TestGetSecretReference(t *testing.T) { expectRef: nil, expectErr: true, }, - "template - valid": { + "template - PVC name annotations not supported for Provision and Delete": { + secretParams: provisionerSecretParams, + params: map[string]string{ + prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}-${pvc.annotations['akey']}", + }, + pvc: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "ns", + }, + }, + expectErr: true, + }, + "template - valid nodepublish secret ref": { secretParams: nodePublishSecretParams, params: map[string]string{ nodePublishSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}-${pvc.annotations['akey']}", @@ -588,6 +615,63 @@ func TestGetSecretReference(t *testing.T) { }, expectRef: &v1.SecretReference{Name: "static-pvname-pvcnamespace-pvcname-avalue", Namespace: "static-pvname-pvcnamespace"}, }, + "template - valid provisioner secret ref": { + secretParams: provisionerSecretParams, + params: map[string]string{ + provisionerSecretNameKey: "static-provisioner-${pv.name}-${pvc.namespace}-${pvc.name}", + provisionerSecretNamespaceKey: "static-provisioner-${pv.name}-${pvc.namespace}", + }, + pvName: "pvname", + pvc: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvcname", + Namespace: "pvcnamespace", + }, + }, + expectRef: &v1.SecretReference{Name: "static-provisioner-pvname-pvcnamespace-pvcname", Namespace: "static-provisioner-pvname-pvcnamespace"}, + }, + "template - valid, with pvc.name": { + secretParams: provisionerSecretParams, + params: map[string]string{ + provisionerSecretNameKey: "${pvc.name}", + provisionerSecretNamespaceKey: "ns", + }, + pvc: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvcname", + Namespace: "pvcns", + }, + }, + expectRef: &v1.SecretReference{Name: "pvcname", Namespace: "ns"}, + }, + "template - valid, provisioner with pvc name and namepsace": { + secretParams: provisionerSecretParams, + params: map[string]string{ + provisionerSecretNameKey: "${pvc.name}", + provisionerSecretNamespaceKey: "${pvc.namespace}", + }, + pvc: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvcname", + Namespace: "pvcns", + }, + }, + expectRef: &v1.SecretReference{Name: "pvcname", Namespace: "pvcns"}, + }, + "template - valid, static pvc name and templated namespace": { + secretParams: provisionerSecretParams, + params: map[string]string{ + provisionerSecretNameKey: "static-name-1", + provisionerSecretNamespaceKey: "${pvc.namespace}", + }, + pvc: &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "ns", + }, + }, + expectRef: &v1.SecretReference{Name: "static-name-1", Namespace: "ns"}, + }, "template - invalid namespace tokens": { secretParams: nodePublishSecretParams, params: map[string]string{ @@ -1843,3 +1927,162 @@ func TestProvisionWithMountOptions(t *testing.T) { t.Errorf("expected mount options %v; got: %v", expectedOptions, pv.Spec.MountOptions) } } + +type deleteTestcase struct { + persistentVolume *v1.PersistentVolume + storageClass *storagev1.StorageClass + mockDelete bool + expectErr bool +} + +// TestDelete is a test of the delete operation +func TestDelete(t *testing.T) { + tt := map[string]deleteTestcase{ + "fail - nil PV": deleteTestcase{ + persistentVolume: nil, + expectErr: true, + }, + "fail - nil volume.Spec.CSI": deleteTestcase{ + persistentVolume: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pv", + Namespace: "ns", + Annotations: map[string]string{ + prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}-${pvc.annotations['akey']}", + }, + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{}, + }, + }, + expectErr: true, + }, + "fail - pvc.annotations not supported": deleteTestcase{ + persistentVolume: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pv", + Namespace: "ns", + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + VolumeHandle: "vol-id-1", + }, + }, + ClaimRef: &v1.ObjectReference{ + Name: "sc-name", + Namespace: "ns", + }, + StorageClassName: "sc-name", + }, + }, + storageClass: &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sc-name", + Namespace: "ns", + }, + Parameters: map[string]string{ + prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}-${pvc.annotations['akey']}", + }, + }, + expectErr: true, + }, + "simple - valid case": deleteTestcase{ + persistentVolume: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pv", + Namespace: "ns", + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + VolumeHandle: "vol-id-1", + }, + }, + }, + }, + storageClass: &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sc-name", + Namespace: "ns", + }, + Parameters: map[string]string{ + prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}", + }, + }, + expectErr: false, + mockDelete: true, + }, + "simple - valid case with ClaimRef set": deleteTestcase{ + persistentVolume: &v1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pv", + Namespace: "ns", + }, + Spec: v1.PersistentVolumeSpec{ + PersistentVolumeSource: v1.PersistentVolumeSource{ + CSI: &v1.CSIPersistentVolumeSource{ + VolumeHandle: "vol-id-1", + }, + }, + ClaimRef: &v1.ObjectReference{ + Name: "pvc-name", + Namespace: "ns", + }, + }, + }, + storageClass: &storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sc-name", + Namespace: "ns", + }, + Parameters: map[string]string{ + prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}", + }, + }, + expectErr: false, + mockDelete: true, + }, + } + + for k, tc := range tt { + runDeleteTest(t, k, tc) + } +} + +func runDeleteTest(t *testing.T, k string, tc deleteTestcase) { + t.Logf("Running test: %v", k) + + tmpdir := tempDir(t) + + defer os.RemoveAll(tmpdir) + mockController, driver, _, controllerServer, csiConn, err := createMockServer(t, tmpdir) + if err != nil { + t.Fatal(err) + } + defer mockController.Finish() + defer driver.Stop() + + var clientSet *fakeclientset.Clientset + + if tc.storageClass != nil { + clientSet = fakeclientset.NewSimpleClientset(tc.storageClass) + } else { + clientSet = fakeclientset.NewSimpleClientset() + } + + if tc.mockDelete { + controllerServer.EXPECT().DeleteVolume(gomock.Any(), gomock.Any()).Return(&csi.DeleteVolumeResponse{}, nil).Times(1) + } + + pluginCaps, controllerCaps := provisionCapabilities() + csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "") + + err = csiProvisioner.Delete(tc.persistentVolume) + if tc.expectErr && err == nil { + t.Errorf("test %q: Expected error, got none", k) + } + if !tc.expectErr && err != nil { + t.Errorf("test %q: got error: %v", k, err) + } +}