Validate storage class parameters in getSecretReference

Signed-off-by: Grant Griffiths <ggp493@gmail.com>
kubernetes-csi · May 14, 2019 · 067e474 · 067e474
1 parent 82727cc
commit 067e474
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 88 deletions.
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -445,7 +445,12 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis
 	rep := &csi.CreateVolumeResponse{}
 
 	// Resolve provision secret credentials.
-	provisionerSecretRef, err := getProvisionSecretReference(provisionerSecretParams, createVolumeRequestParameters, pvName, options.PVC)
+	provisionerSecretRef, err := getSecretReference(provisionerSecretParams, createVolumeRequestParameters, pvName, &v1.PersistentVolumeClaim{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      options.PVC.Name,
+			Namespace: options.PVC.Namespace,
+		},
+	})
 	if err != nil {
 		return nil, err
 	}
@@ -557,6 +562,19 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis
 	return pv, nil
 }
 
+func validateStorageClassParameters(params map[string]string) error {
+	for k, v := range params {
+		switch {
+		case strings.Contains(k, prefixedProvisionerSecretNameKey) && strings.Contains(v, "pvc.annotations"):
+			return fmt.Errorf("Secret name stored in storage class parameters is not supported")
+		case strings.Contains(k, prefixedProvisionerSecretNamespaceKey) && strings.Contains(v, "pvc.annotations"):
+			return fmt.Errorf("Secret namespace stored in storage class parameters is not supported")
+		}
+	}
+
+	return nil
+}
+
 func (p *csiProvisioner) supportsTopology() bool {
 	return p.pluginCapabilities[csi.PluginCapability_Service_VOLUME_ACCESSIBILITY_CONSTRAINTS] &&
 		utilfeature.DefaultFeatureGate.Enabled(features.Topology)
@@ -677,7 +695,7 @@ func (p *csiProvisioner) Delete(volume *v1.PersistentVolume) error {
 			credentials, err := getCredentials(p.client, provisionerSecretRef)
 			if err != nil {
 				// Continue with deletion, as the secret may have already been deleted.
-				klog.Warningf("Failed to get credentials for volume %s: %s", volume.Name, err.Error())
+				klog.Errorf("Failed to get credentials for volume %s: %s", volume.Name, err.Error())
 			}
 			req.Secrets = credentials
 		}
@@ -754,20 +772,6 @@ func verifyAndGetSecretNameAndNamespaceTemplate(secret secretParamsMap, storageC
 	}
 }
 
-// getProvisionSecretReference returns a reference to the provisioning secret,
-// with an additional check for unsupported PVC annotations.
-func getProvisionSecretReference(secretParams secretParamsMap, storageClassParams map[string]string, pvName string, pvc *v1.PersistentVolumeClaim) (*v1.SecretReference, error) {
-	// validate that no annotations are being passed in as secrets
-	for k, v := range pvc.Annotations {
-		if (strings.Contains(k, prefixedProvisionerSecretNameKey) && strings.Contains(v, "pvc.annotations")) ||
-			(strings.Contains(k, prefixedProvisionerSecretNamespaceKey) && strings.Contains(v, "pvc.annotations")) {
-			return nil, fmt.Errorf("Secrets stored in PVC annotations is not supported for Provision and Delete")
-		}
-	}
-
-	return getSecretReference(secretParams, storageClassParams, pvName, pvc)
-}
-
 // getSecretReference returns a reference to the secret specified in the given nameTemplate
 //  and namespaceTemplate, or an error if the templates are not specified correctly.
 // no lookup of the referenced secret is performed, and the secret may or may not exist.
@@ -787,6 +791,11 @@ func getProvisionSecretReference(secretParams secretParamsMap, storageClassParam
 // - the resolved name is not a valid secret name
 // - the resolved namespace is not a valid namespace name
 func getSecretReference(secretParams secretParamsMap, storageClassParams map[string]string, pvName string, pvc *v1.PersistentVolumeClaim) (*v1.SecretReference, error) {
+	// Ensure that no unsupported SC parameters are passed in.
+	if err := validateStorageClassParameters(storageClassParams); err != nil {
+		return nil, err
+	}
+
 	nameTemplate, namespaceTemplate, err := verifyAndGetSecretNameAndNamespaceTemplate(secretParams, storageClassParams)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get name and namespace template from params: %v", err)

diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
@@ -580,6 +580,14 @@ func TestGetSecretReference(t *testing.T) {
 			expectRef:    nil,
 			expectErr:    true,
 		},
+		"simple - 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{},
+			expectErr: true,
+		},
 		"template - valid": {
 			secretParams: nodePublishSecretParams,
 			params: map[string]string{
@@ -679,78 +687,6 @@ func TestGetSecretReference(t *testing.T) {
 	}
 }
 
-func TestGetProvisionSecretReference(t *testing.T) {
-	testcases := map[string]struct {
-		secretParams secretParamsMap
-		params       map[string]string
-		pvName       string
-		pvc          *v1.PersistentVolumeClaim
-
-		expectRef *v1.SecretReference
-		expectErr bool
-	}{
-		"simple - valid name and namespace": {
-			secretParams: provisionerSecretParams,
-			params: map[string]string{
-				provisionerSecretNameKey:      "${pvc.name}",
-				provisionerSecretNamespaceKey: "${pvc.namespace}",
-			},
-			pvc: &v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "name",
-					Namespace: "ns",
-				},
-			},
-			expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"},
-		},
-		"simple - PVC name annotations not supported for Provision and Delete": {
-			secretParams: provisionerSecretParams,
-			pvc: &v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "name",
-					Namespace: "ns",
-					Annotations: map[string]string{
-						prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}-${pvc.annotations['akey']}",
-					},
-				},
-			},
-			expectErr: true,
-		},
-		"simple - PVC namespace annotations not supported for Provision and Delete": {
-			secretParams: provisionerSecretParams,
-			pvc: &v1.PersistentVolumeClaim{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "name",
-					Namespace: "ns",
-					Annotations: map[string]string{
-						prefixedProvisionerSecretNamespaceKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}-${pvc.annotations['akey']}",
-					},
-				},
-			},
-			expectErr: true,
-		},
-	}
-
-	for k, tc := range testcases {
-		t.Run(k, func(t *testing.T) {
-			ref, err := getProvisionSecretReference(tc.secretParams, tc.params, tc.pvName, tc.pvc)
-			if err != nil {
-				if tc.expectErr {
-					return
-				}
-				t.Fatalf("Did not expect error but got: %v", err)
-			} else {
-				if tc.expectErr {
-					t.Fatalf("Expected error but got none")
-				}
-			}
-			if !reflect.DeepEqual(ref, tc.expectRef) {
-				t.Errorf("Expected %v, got %v", tc.expectRef, ref)
-			}
-		})
-	}
-}
-
 type provisioningTestcase struct {
 	volOpts           controller.VolumeOptions
 	notNilSelector    bool