From 2ef26b0c4f621bc24b2c67f86f914c81dab4f537 Mon Sep 17 00:00:00 2001 From: David Zhu Date: Fri, 30 Nov 2018 14:34:31 -0800 Subject: [PATCH] Add new reserved prefixed parameter keys which are stripped out of parameter list, add deprecation notice for old keys and keep their behavior the same --- pkg/controller/controller.go | 210 ++++++++++-- pkg/controller/controller_test.go | 542 ++++++++++++++++++++---------- 2 files changed, 543 insertions(+), 209 deletions(-) diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index bdf97ed076..cbf4c9e66a 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -57,7 +57,37 @@ import ( utilfeature "k8s.io/apiserver/pkg/util/feature" ) +type deprecatedSecretParamsMap struct { + name string + deprecatedSecretNameKey string + deprecatedSecretNamespaceKey string + secretNameKey string + secretNamespaceKey string +} + const ( + // CSI Parameters prefixed with csiParameterPrefix are not passed through + // to the driver on CreateVolumeRequest calls. Instead they are intended + // to used by the CSI external-provisioner and maybe used to populate + // fields in subsequent CSI calls or Kubernetes API objects. + csiParameterPrefix = "csi.storage.k8s.io/" + + prefixedFsTypeKey = csiParameterPrefix + "fstype" + + prefixedProvisionerSecretNameKey = csiParameterPrefix + "provisioner-secret-name" + prefixedProvisionerSecretNamespaceKey = csiParameterPrefix + "provisioner-secret-namespace" + + prefixedControllerPublishSecretNameKey = csiParameterPrefix + "controller-publish-secret-name" + prefixedControllerPublishSecretNamespaceKey = csiParameterPrefix + "controller-publish-secret-namespace" + + prefixedNodeStageSecretNameKey = csiParameterPrefix + "node-stage-secret-name" + prefixedNodeStageSecretNamespaceKey = csiParameterPrefix + "node-stage-secret-namespace" + + prefixedNodePublishSecretNameKey = csiParameterPrefix + "node-publish-secret-name" + prefixedNodePublishSecretNamespaceKey = csiParameterPrefix + "node-publish-secret-namespace" + + // [Deprecated] CSI Parameters that are put into fields but + // NOT stripped from the parameters passed to CreateVolume provisionerSecretNameKey = "csiProvisionerSecretName" provisionerSecretNamespaceKey = "csiProvisionerSecretNamespace" @@ -83,6 +113,40 @@ const ( snapshotAPIGroup = snapapi.GroupName // "snapshot.storage.k8s.io" ) +var ( + provisionerSecretParams = deprecatedSecretParamsMap{ + name: "Provisioner", + deprecatedSecretNameKey: provisionerSecretNameKey, + deprecatedSecretNamespaceKey: provisionerSecretNamespaceKey, + secretNameKey: prefixedProvisionerSecretNameKey, + secretNamespaceKey: prefixedProvisionerSecretNamespaceKey, + } + + nodePublishSecretParams = deprecatedSecretParamsMap{ + name: "NodePublish", + deprecatedSecretNameKey: nodePublishSecretNameKey, + deprecatedSecretNamespaceKey: nodePublishSecretNamespaceKey, + secretNameKey: prefixedNodePublishSecretNameKey, + secretNamespaceKey: prefixedNodePublishSecretNamespaceKey, + } + + controllerPublishSecretParams = deprecatedSecretParamsMap{ + name: "ControllerPublish", + deprecatedSecretNameKey: controllerPublishSecretNameKey, + deprecatedSecretNamespaceKey: controllerPublishSecretNamespaceKey, + secretNameKey: prefixedControllerPublishSecretNameKey, + secretNamespaceKey: prefixedControllerPublishSecretNamespaceKey, + } + + nodeStageSecretParams = deprecatedSecretParamsMap{ + name: "NodeStage", + deprecatedSecretNameKey: nodeStageSecretNameKey, + deprecatedSecretNamespaceKey: nodeStageSecretNamespaceKey, + secretNameKey: prefixedNodeStageSecretNameKey, + secretNamespaceKey: prefixedNodeStageSecretNamespaceKey, + } +) + // CSIProvisioner struct type csiProvisioner struct { client kubernetes.Interface @@ -416,13 +480,21 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis return nil, err } + fsTypesFound := 0 fsType := "" for k, v := range options.Parameters { - switch strings.ToLower(k) { - case "fstype": + if strings.ToLower(k) == "fstype" { + fsType = v + fsTypesFound += 1 + glog.Warningf(deprecationWarning("fstype", prefixedFsTypeKey, "")) + } else if k == prefixedFsTypeKey { fsType = v + fsTypesFound += 1 } } + if fsTypesFound > 1 { + return nil, fmt.Errorf("fstype specified in parameters with both \"fstype\" and \"%s\" keys", prefixedFsTypeKey) + } if len(fsType) == 0 { fsType = defaultFSType } @@ -475,7 +547,7 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis // 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(provisionerSecretNameKey, provisionerSecretNamespaceKey, options.Parameters, pvName, nil) + provisionerSecretRef, err := getSecretReference(provisionerSecretParams, options.Parameters, pvName, nil) if err != nil { return nil, err } @@ -486,19 +558,24 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis req.Secrets = provisionerCredentials // Resolve controller publish, node stage, node publish secret references - controllerPublishSecretRef, err := getSecretReference(controllerPublishSecretNameKey, controllerPublishSecretNamespaceKey, options.Parameters, pvName, options.PVC) + controllerPublishSecretRef, err := getSecretReference(controllerPublishSecretParams, options.Parameters, pvName, options.PVC) if err != nil { return nil, err } - nodeStageSecretRef, err := getSecretReference(nodeStageSecretNameKey, nodeStageSecretNamespaceKey, options.Parameters, pvName, options.PVC) + nodeStageSecretRef, err := getSecretReference(nodeStageSecretParams, options.Parameters, pvName, options.PVC) if err != nil { return nil, err } - nodePublishSecretRef, err := getSecretReference(nodePublishSecretNameKey, nodePublishSecretNamespaceKey, options.Parameters, pvName, options.PVC) + nodePublishSecretRef, err := getSecretReference(nodePublishSecretParams, options.Parameters, pvName, options.PVC) if err != nil { return nil, err } + req.Parameters, err = removePrefixedParameters(options.Parameters) + if err != nil { + return nil, fmt.Errorf("failed to strip CSI Parameters of prefixed keys: %v", err) + } + opts := wait.Backoff{Duration: backoffDuration, Factor: backoffFactor, Steps: backoffSteps} err = wait.ExponentialBackoff(opts, func() (bool, error) { ctx, cancel := context.WithTimeout(context.Background(), p.timeout) @@ -591,6 +668,33 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis return pv, nil } +func removePrefixedParameters(param map[string]string) (map[string]string, error) { + newParam := map[string]string{} + for k, v := range param { + if strings.HasPrefix(k, csiParameterPrefix) { + // Check if its well known + switch k { + case prefixedFsTypeKey: + case prefixedProvisionerSecretNameKey: + case prefixedProvisionerSecretNamespaceKey: + case prefixedControllerPublishSecretNameKey: + case prefixedControllerPublishSecretNamespaceKey: + case prefixedNodeStageSecretNameKey: + case prefixedNodeStageSecretNamespaceKey: + case prefixedNodePublishSecretNameKey: + case prefixedNodePublishSecretNamespaceKey: + default: + return map[string]string{}, fmt.Errorf("found unknown parameter key \"%s\" with reserved namespace %s", k, csiParameterPrefix) + } + } else { + // Don't strip, add this key-value to new map + // Deprecated parameters prefixed with "csi" are not stripped to preserve backwards compatibility + newParam[k] = v + } + } + return newParam, nil +} + func (p *csiProvisioner) getVolumeContentSource(options controller.VolumeOptions) (*csi.VolumeContentSource, error) { snapshotObj, err := p.snapshotClient.VolumesnapshotV1alpha1().VolumeSnapshots(options.PVC.Namespace).Get(options.PVC.Spec.DataSource.Name, metav1.GetOptions{}) if err != nil { @@ -666,7 +770,7 @@ func (p *csiProvisioner) Delete(volume *v1.PersistentVolume) error { 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(provisionerSecretNameKey, provisionerSecretNamespaceKey, storageClass.Parameters, volume.Name, nil) + provisionerSecretRef, err := getSecretReference(provisionerSecretParams, storageClass.Parameters, volume.Name, nil) if err != nil { return err } @@ -703,8 +807,54 @@ func (p *csiProvisioner) volumeHandleToId(handle string) string { return handle } -// getSecretReference returns a reference to the secret specified in the given nameKey and namespaceKey parameters, or an error if the parameters are not specified correctly. -// if neither the name or namespace parameter are set, a nil reference and no error is returned. +// verifyAndGetSecretNameAndNamespaceTemplate gets the values (templates) associated +// with the parameters specified in "secret" and verifies that they are specified correctly. +func verifyAndGetSecretNameAndNamespaceTemplate(secret deprecatedSecretParamsMap, storageClassParams map[string]string) (nameTemplate, namespaceTemplate string, err error) { + numName := 0 + numNamespace := 0 + + if t, ok := storageClassParams[secret.deprecatedSecretNameKey]; ok { + nameTemplate = t + numName += 1 + glog.Warning(deprecationWarning(secret.deprecatedSecretNameKey, secret.secretNameKey, "")) + } + if t, ok := storageClassParams[secret.deprecatedSecretNamespaceKey]; ok { + namespaceTemplate = t + numNamespace += 1 + glog.Warning(deprecationWarning(secret.deprecatedSecretNamespaceKey, secret.secretNamespaceKey, "")) + } + if t, ok := storageClassParams[secret.secretNameKey]; ok { + nameTemplate = t + numName += 1 + } + if t, ok := storageClassParams[secret.secretNamespaceKey]; ok { + namespaceTemplate = t + numNamespace += 1 + } + + if numName > 1 || numNamespace > 1 { + // Double specified error + return "", "", fmt.Errorf("%s secrets specified in parameters with both \"csi\" and \"%s\" keys", secret.name, csiParameterPrefix) + } else if numName != numNamespace { + // Not both 0 or both 1 + return "", "", fmt.Errorf("either name and namespace for %s secrets specified, Both must be specified", secret.name) + } else if numName == 1 { + // Case where we've found a name and a namespace template + if nameTemplate == "" || namespaceTemplate == "" { + return "", "", fmt.Errorf("%s secrets specified in parameters but value of either namespace or name is empty", secret.name) + } + return nameTemplate, namespaceTemplate, nil + } else if numName == 0 { + // No secrets specified + return "", "", nil + } else { + // THIS IS NOT A VALID CASE + return "", "", fmt.Errorf("unknown error with getting secret name and namespace templates") + } +} + +// 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. // // supported tokens for name resolution: @@ -718,24 +868,19 @@ func (p *csiProvisioner) volumeHandleToId(handle string) string { // - ${pvc.namespace} // // an error is returned in the following situations: -// - only one of name or namespace is provided -// - the name or namespace parameter contains a token that cannot be resolved +// - the nameTemplate or namespaceTemplate contains a token that cannot be resolved // - the resolved name is not a valid secret name // - the resolved namespace is not a valid namespace name -func getSecretReference(nameKey, namespaceKey string, storageClassParams map[string]string, pvName string, pvc *v1.PersistentVolumeClaim) (*v1.SecretReference, error) { - nameTemplate, hasName := storageClassParams[nameKey] - namespaceTemplate, hasNamespace := storageClassParams[namespaceKey] - - if !hasName && !hasNamespace { - return nil, nil +func getSecretReference(secretParams deprecatedSecretParamsMap, storageClassParams map[string]string, pvName string, pvc *v1.PersistentVolumeClaim) (*v1.SecretReference, error) { + nameTemplate, namespaceTemplate, err := verifyAndGetSecretNameAndNamespaceTemplate(secretParams, storageClassParams) + if err != nil { + return nil, fmt.Errorf("failed to get name and namespace template from params: %v", err) } - - if len(nameTemplate) == 0 || len(namespaceTemplate) == 0 { - return nil, fmt.Errorf("%s and %s parameters must be specified together", nameKey, namespaceKey) + if nameTemplate == "" && namespaceTemplate == "" { + return nil, nil } ref := &v1.SecretReference{} - { // Secret namespace template can make use of the PV name or the PVC namespace. // Note that neither of those things are under the control of the PVC user. @@ -746,13 +891,13 @@ func getSecretReference(nameKey, namespaceKey string, storageClassParams map[str resolvedNamespace, err := resolveTemplate(namespaceTemplate, namespaceParams) if err != nil { - return nil, fmt.Errorf("error resolving %s value %q: %v", namespaceKey, namespaceTemplate, err) + return nil, fmt.Errorf("error resolving value %q: %v", namespaceTemplate, err) } if len(validation.IsDNS1123Label(resolvedNamespace)) > 0 { if namespaceTemplate != resolvedNamespace { - return nil, fmt.Errorf("%s parameter %q resolved to %q which is not a valid namespace name", namespaceKey, namespaceTemplate, resolvedNamespace) + return nil, fmt.Errorf("%q resolved to %q which is not a valid namespace name", namespaceTemplate, resolvedNamespace) } - return nil, fmt.Errorf("%s parameter %q is not a valid namespace name", namespaceKey, namespaceTemplate) + return nil, fmt.Errorf("%q is not a valid namespace name", namespaceTemplate) } ref.Namespace = resolvedNamespace } @@ -770,13 +915,13 @@ func getSecretReference(nameKey, namespaceKey string, storageClassParams map[str } resolvedName, err := resolveTemplate(nameTemplate, nameParams) if err != nil { - return nil, fmt.Errorf("error resolving %s value %q: %v", nameKey, nameTemplate, err) + return nil, fmt.Errorf("error resolving value %q: %v", nameTemplate, err) } if len(validation.IsDNS1123Subdomain(resolvedName)) > 0 { if nameTemplate != resolvedName { - return nil, fmt.Errorf("%s parameter %q resolved to %q which is not a valid secret name", nameKey, nameTemplate, resolvedName) + return nil, fmt.Errorf("%q resolved to %q which is not a valid secret name", nameTemplate, resolvedName) } - return nil, fmt.Errorf("%s parameter %q is not a valid secret name", nameKey, nameTemplate) + return nil, fmt.Errorf("%q is not a valid secret name", nameTemplate) } ref.Name = resolvedName } @@ -835,3 +980,14 @@ func bytesToGiQuantity(bytes int64) resource.Quantity { stringQuantity := fmt.Sprintf("%v%s", num, suffix) return resource.MustParse(stringQuantity) } + +func deprecationWarning(deprecatedParam, newParam, removalVersion string) string { + if removalVersion == "" { + removalVersion = "a future release" + } + newParamPhrase := "" + if len(newParam) != 0 { + newParamPhrase = fmt.Sprintf(", please use \"%s\" instead", newParam) + } + return fmt.Sprintf("\"%s\" is deprecated and will be removed in %s%s", deprecatedParam, removalVersion, newParamPhrase) +} diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 3bf7e386a2..997496db86 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -52,6 +52,11 @@ const ( timeout = 10 * time.Second ) +var ( + volumeModeFileSystem = v1.PersistentVolumeFilesystem + volumeModeBlock = v1.PersistentVolumeBlock +) + type csiConnection struct { conn *grpc.ClientConn } @@ -149,6 +154,104 @@ func TestGetPluginName(t *testing.T) { } } +func TestStripPrefixedCSIParams(t *testing.T) { + testcases := []struct { + name string + params map[string]string + expectedParams map[string]string + expectErr bool + }{ + { + name: "no prefix", + params: map[string]string{"csiFoo": "bar", "bim": "baz"}, + expectedParams: map[string]string{"csiFoo": "bar", "bim": "baz"}, + }, + { + name: "one prefixed", + params: map[string]string{prefixedControllerPublishSecretNameKey: "bar", "bim": "baz"}, + expectedParams: map[string]string{"bim": "baz"}, + }, + { + name: "prefix in value", + params: map[string]string{"foo": prefixedFsTypeKey, "bim": "baz"}, + expectedParams: map[string]string{"foo": prefixedFsTypeKey, "bim": "baz"}, + }, + { + name: "all known prefixed", + params: map[string]string{ + prefixedFsTypeKey: "csiBar", + prefixedProvisionerSecretNameKey: "csiBar", + prefixedProvisionerSecretNamespaceKey: "csiBar", + prefixedControllerPublishSecretNameKey: "csiBar", + prefixedControllerPublishSecretNamespaceKey: "csiBar", + prefixedNodeStageSecretNameKey: "csiBar", + prefixedNodeStageSecretNamespaceKey: "csiBar", + prefixedNodePublishSecretNameKey: "csiBar", + prefixedNodePublishSecretNamespaceKey: "csiBar", + }, + expectedParams: map[string]string{}, + }, + { + name: "all known deprecated params not stripped", + params: map[string]string{ + "fstype": "csiBar", + provisionerSecretNameKey: "csiBar", + provisionerSecretNamespaceKey: "csiBar", + controllerPublishSecretNameKey: "csiBar", + controllerPublishSecretNamespaceKey: "csiBar", + nodeStageSecretNameKey: "csiBar", + nodeStageSecretNamespaceKey: "csiBar", + nodePublishSecretNameKey: "csiBar", + nodePublishSecretNamespaceKey: "csiBar", + }, + expectedParams: map[string]string{ + "fstype": "csiBar", + provisionerSecretNameKey: "csiBar", + provisionerSecretNamespaceKey: "csiBar", + controllerPublishSecretNameKey: "csiBar", + controllerPublishSecretNamespaceKey: "csiBar", + nodeStageSecretNameKey: "csiBar", + nodeStageSecretNamespaceKey: "csiBar", + nodePublishSecretNameKey: "csiBar", + nodePublishSecretNamespaceKey: "csiBar", + }, + }, + + { + name: "unknown prefixed var", + params: map[string]string{csiParameterPrefix + "bim": "baz"}, + expectErr: true, + }, + { + name: "empty", + params: map[string]string{}, + expectedParams: map[string]string{}, + }, + } + + for _, tc := range testcases { + t.Logf("test: %v", tc.name) + + newParams, err := removePrefixedParameters(tc.params) + if err != nil { + if tc.expectErr { + continue + } else { + t.Fatalf("Encountered unexpected error: %v", err) + } + } else { + if tc.expectErr { + t.Fatalf("Did not get error when one was expected") + } + } + + eq := reflect.DeepEqual(newParams, tc.expectedParams) + if !eq { + t.Fatalf("Stripped parameters: %v not equal to expected parameters: %v", newParams, tc.expectedParams) + } + } +} + func TestGetDriverCapabilities(t *testing.T) { type testcase struct { name string @@ -594,75 +697,99 @@ func createFakePVCWithVolumeMode(requestBytes int64, volumeMode v1.PersistentVol func TestGetSecretReference(t *testing.T) { testcases := map[string]struct { - nameKey string - namespaceKey string + secretParams deprecatedSecretParamsMap params map[string]string pvName string pvc *v1.PersistentVolumeClaim expectRef *v1.SecretReference - expectErr error + expectErr bool }{ "no params": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + secretParams: nodePublishSecretParams, params: nil, expectRef: nil, - expectErr: nil, }, "empty err": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + secretParams: nodePublishSecretParams, params: map[string]string{nodePublishSecretNameKey: "", nodePublishSecretNamespaceKey: ""}, - expectErr: fmt.Errorf("csiNodePublishSecretName and csiNodePublishSecretNamespace parameters must be specified together"), + expectErr: true, }, - "name, no namespace": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + "[deprecated] name, no namespace": { + secretParams: nodePublishSecretParams, params: map[string]string{nodePublishSecretNameKey: "foo"}, - expectErr: fmt.Errorf("csiNodePublishSecretName and csiNodePublishSecretNamespace parameters must be specified together"), + expectErr: true, }, - "namespace, no name": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + "name, no namespace": { + secretParams: nodePublishSecretParams, + params: map[string]string{prefixedNodePublishSecretNameKey: "foo"}, + expectErr: true, + }, + "[deprecated] namespace, no name": { + secretParams: nodePublishSecretParams, params: map[string]string{nodePublishSecretNamespaceKey: "foo"}, - expectErr: fmt.Errorf("csiNodePublishSecretName and csiNodePublishSecretNamespace parameters must be specified together"), + expectErr: true, }, - "simple - valid": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + "namespace, no name": { + secretParams: nodePublishSecretParams, + params: map[string]string{prefixedNodePublishSecretNamespaceKey: "foo"}, + expectErr: true, + }, + "[deprecated] simple - valid": { + secretParams: nodePublishSecretParams, params: map[string]string{nodePublishSecretNameKey: "name", nodePublishSecretNamespaceKey: "ns"}, pvc: &v1.PersistentVolumeClaim{}, expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"}, - expectErr: nil, + }, + "deprecated and new both": { + secretParams: nodePublishSecretParams, + params: map[string]string{nodePublishSecretNameKey: "name", nodePublishSecretNamespaceKey: "ns", prefixedNodePublishSecretNameKey: "name", prefixedNodePublishSecretNamespaceKey: "ns"}, + expectErr: true, + }, + "deprecated and new names": { + secretParams: nodePublishSecretParams, + params: map[string]string{nodePublishSecretNameKey: "name", nodePublishSecretNamespaceKey: "ns", prefixedNodePublishSecretNameKey: "name"}, + expectErr: true, + }, + "deprecated and new namespace": { + secretParams: nodePublishSecretParams, + params: map[string]string{nodePublishSecretNameKey: "name", nodePublishSecretNamespaceKey: "ns", prefixedNodePublishSecretNamespaceKey: "ns"}, + expectErr: true, + }, + "deprecated and new mixed": { + secretParams: nodePublishSecretParams, + params: map[string]string{nodePublishSecretNameKey: "name", prefixedNodePublishSecretNamespaceKey: "ns"}, + pvc: &v1.PersistentVolumeClaim{}, + expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"}, + }, + "simple - valid": { + secretParams: nodePublishSecretParams, + params: map[string]string{prefixedNodePublishSecretNameKey: "name", prefixedNodePublishSecretNamespaceKey: "ns"}, + pvc: &v1.PersistentVolumeClaim{}, + expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"}, }, "simple - valid, no pvc": { - nameKey: provisionerSecretNameKey, - namespaceKey: provisionerSecretNamespaceKey, + secretParams: provisionerSecretParams, params: map[string]string{provisionerSecretNameKey: "name", provisionerSecretNamespaceKey: "ns"}, pvc: nil, expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"}, - expectErr: nil, }, "simple - invalid name": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + secretParams: nodePublishSecretParams, params: map[string]string{nodePublishSecretNameKey: "bad name", nodePublishSecretNamespaceKey: "ns"}, pvc: &v1.PersistentVolumeClaim{}, expectRef: nil, - expectErr: fmt.Errorf(`csiNodePublishSecretName parameter "bad name" is not a valid secret name`), + expectErr: true, }, "simple - invalid namespace": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + secretParams: nodePublishSecretParams, params: map[string]string{nodePublishSecretNameKey: "name", nodePublishSecretNamespaceKey: "bad ns"}, pvc: &v1.PersistentVolumeClaim{}, expectRef: nil, - expectErr: fmt.Errorf(`csiNodePublishSecretNamespace parameter "bad ns" is not a valid namespace name`), + expectErr: true, }, "template - valid": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + secretParams: nodePublishSecretParams, params: map[string]string{ nodePublishSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}-${pvc.annotations['akey']}", nodePublishSecretNamespaceKey: "static-${pv.name}-${pvc.namespace}", @@ -676,37 +803,42 @@ func TestGetSecretReference(t *testing.T) { }, }, expectRef: &v1.SecretReference{Name: "static-pvname-pvcnamespace-pvcname-avalue", Namespace: "static-pvname-pvcnamespace"}, - expectErr: nil, }, "template - invalid namespace tokens": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + secretParams: nodePublishSecretParams, params: map[string]string{ nodePublishSecretNameKey: "myname", nodePublishSecretNamespaceKey: "mynamespace${bar}", }, pvc: &v1.PersistentVolumeClaim{}, expectRef: nil, - expectErr: fmt.Errorf(`error resolving csiNodePublishSecretNamespace value "mynamespace${bar}": invalid tokens: ["bar"]`), + expectErr: true, }, "template - invalid name tokens": { - nameKey: nodePublishSecretNameKey, - namespaceKey: nodePublishSecretNamespaceKey, + secretParams: nodePublishSecretParams, params: map[string]string{ nodePublishSecretNameKey: "myname${foo}", nodePublishSecretNamespaceKey: "mynamespace", }, pvc: &v1.PersistentVolumeClaim{}, expectRef: nil, - expectErr: fmt.Errorf(`error resolving csiNodePublishSecretName value "myname${foo}": invalid tokens: ["foo"]`), + expectErr: true, }, } for k, tc := range testcases { t.Run(k, func(t *testing.T) { - ref, err := getSecretReference(tc.nameKey, tc.namespaceKey, tc.params, tc.pvName, tc.pvc) - if !reflect.DeepEqual(err, tc.expectErr) { - t.Errorf("Expected %v, got %v", tc.expectErr, err) + ref, err := getSecretReference(tc.secretParams, tc.params, tc.pvName, tc.pvc) + if err != nil { + if tc.expectErr { + return + } else { + 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) @@ -715,36 +847,32 @@ func TestGetSecretReference(t *testing.T) { } } -func TestProvision(t *testing.T) { - - var ( - requestedBytes int64 = 100 - volumeModeFileSystem = v1.PersistentVolumeFilesystem - volumeModeBlock = v1.PersistentVolumeBlock - ) +type provisioningTestcase struct { + volOpts controller.VolumeOptions + notNilSelector bool + driverNotReady bool + makeVolumeNameErr bool + getSecretRefErr bool + getCredentialsErr bool + volWithLessCap bool + expectedPVSpec *pvSpec + withSecretRefs bool + expectErr bool + expectCreateVolDo interface{} +} - type pvSpec struct { - Name string - ReclaimPolicy v1.PersistentVolumeReclaimPolicy - AccessModes []v1.PersistentVolumeAccessMode - VolumeMode *v1.PersistentVolumeMode - Capacity v1.ResourceList - CSIPVS *v1.CSIPersistentVolumeSource - } +type pvSpec struct { + Name string + ReclaimPolicy v1.PersistentVolumeReclaimPolicy + AccessModes []v1.PersistentVolumeAccessMode + VolumeMode *v1.PersistentVolumeMode + Capacity v1.ResourceList + CSIPVS *v1.CSIPersistentVolumeSource +} - testcases := map[string]struct { - volOpts controller.VolumeOptions - notNilSelector bool - driverNotReady bool - makeVolumeNameErr bool - getSecretRefErr bool - getCredentialsErr bool - volWithLessCap bool - expectedPVSpec *pvSpec - withSecretRefs bool - expectErr bool - expectCreateVolDo interface{} - }{ +func TestProvision(t *testing.T) { + var requestedBytes int64 = 100 + testcases := map[string]provisioningTestcase{ "normal provision": { volOpts: controller.VolumeOptions{ PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, @@ -770,6 +898,48 @@ func TestProvision(t *testing.T) { }, }, }, + "multiple fsType provision": { + volOpts: controller.VolumeOptions{ + PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, + PVName: "test-name", + PVC: createFakePVC(requestedBytes), + Parameters: map[string]string{ + "fstype": "ext3", + prefixedFsTypeKey: "ext4", + }, + }, + expectErr: true, + }, + "provision with prefixed FS Type key": { + volOpts: controller.VolumeOptions{ + PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, + PVName: "test-name", + PVC: createFakePVC(requestedBytes), + Parameters: map[string]string{ + prefixedFsTypeKey: "ext3", + }, + }, + expectedPVSpec: &pvSpec{ + Name: "test-testi", + ReclaimPolicy: v1.PersistentVolumeReclaimDelete, + Capacity: v1.ResourceList{ + v1.ResourceName(v1.ResourceStorage): bytesToGiQuantity(requestedBytes), + }, + CSIPVS: &v1.CSIPersistentVolumeSource{ + Driver: "test-driver", + VolumeHandle: "test-volume-id", + FSType: "ext3", + VolumeAttributes: map[string]string{ + "storage.kubernetes.io/csiProvisionerIdentity": "test-provisioner", + }, + }, + }, + expectCreateVolDo: func(ctx context.Context, req *csi.CreateVolumeRequest) { + if len(req.Parameters) != 0 { + t.Errorf("Parameters should have been stripped") + } + }, + }, "provision with access mode multi node multi writer": { volOpts: controller.VolumeOptions{ PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, @@ -1154,119 +1324,8 @@ func TestProvision(t *testing.T) { }, } - mockController, driver, identityServer, controllerServer, csiConn, err := createMockServer(t) - if err != nil { - t.Fatal(err) - } - defer mockController.Finish() - defer driver.Stop() - for k, tc := range testcases { - var clientSet kubernetes.Interface - - if tc.withSecretRefs { - clientSet = fakeclientset.NewSimpleClientset(&v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "ctrlpublishsecret", - Namespace: "default", - }, - }, &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nodestagesecret", - Namespace: "default", - }, - }, &v1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nodepublishsecret", - Namespace: "default", - }, - }) - } else { - clientSet = fakeclientset.NewSimpleClientset() - } - - csiProvisioner := NewCSIProvisioner(clientSet, nil, driver.Address(), 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, nil) - - out := &csi.CreateVolumeResponse{ - Volume: &csi.Volume{ - CapacityBytes: requestedBytes, - VolumeId: "test-volume-id", - }, - } - - if tc.withSecretRefs { - tc.volOpts.Parameters[controllerPublishSecretNameKey] = "ctrlpublishsecret" - tc.volOpts.Parameters[controllerPublishSecretNamespaceKey] = "default" - tc.volOpts.Parameters[nodeStageSecretNameKey] = "nodestagesecret" - tc.volOpts.Parameters[nodeStageSecretNamespaceKey] = "default" - tc.volOpts.Parameters[nodePublishSecretNameKey] = "nodepublishsecret" - tc.volOpts.Parameters[nodePublishSecretNamespaceKey] = "default" - } - - if tc.notNilSelector { - tc.volOpts.PVC.Spec.Selector = &metav1.LabelSelector{} - } else if tc.driverNotReady { - identityServer.EXPECT().GetPluginCapabilities(gomock.Any(), gomock.Any()).Return(nil, errors.New("driver not ready")).Times(1) - } else if tc.makeVolumeNameErr { - tc.volOpts.PVC.ObjectMeta.UID = "" - provisionMockServerSetupExpectations(identityServer, controllerServer) - } else if tc.getSecretRefErr { - tc.volOpts.Parameters[provisionerSecretNameKey] = "" - provisionMockServerSetupExpectations(identityServer, controllerServer) - } else if tc.getCredentialsErr { - tc.volOpts.Parameters[provisionerSecretNameKey] = "secretx" - tc.volOpts.Parameters[provisionerSecretNamespaceKey] = "default" - provisionMockServerSetupExpectations(identityServer, controllerServer) - } else if tc.volWithLessCap { - out.Volume.CapacityBytes = int64(80) - provisionMockServerSetupExpectations(identityServer, controllerServer) - controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) - controllerServer.EXPECT().DeleteVolume(gomock.Any(), gomock.Any()).Return(&csi.DeleteVolumeResponse{}, nil).Times(1) - } else if tc.expectCreateVolDo != nil { - provisionMockServerSetupExpectations(identityServer, controllerServer) - controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Do(tc.expectCreateVolDo).Return(out, nil).Times(1) - } else { - // Setup regular mock call expectations. - provisionMockServerSetupExpectations(identityServer, controllerServer) - controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) - } - - pv, err := csiProvisioner.Provision(tc.volOpts) - 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) - } - - if tc.expectedPVSpec != nil { - if pv.Name != tc.expectedPVSpec.Name { - t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name) - } - - if pv.Spec.PersistentVolumeReclaimPolicy != tc.expectedPVSpec.ReclaimPolicy { - t.Errorf("test %q: expected reclaim policy: %v, got: %v", k, tc.expectedPVSpec.ReclaimPolicy, pv.Spec.PersistentVolumeReclaimPolicy) - } - - if !reflect.DeepEqual(pv.Spec.AccessModes, tc.expectedPVSpec.AccessModes) { - t.Errorf("test %q: expected access modes: %v, got: %v", k, tc.expectedPVSpec.AccessModes, pv.Spec.AccessModes) - } - - if !reflect.DeepEqual(pv.Spec.VolumeMode, tc.expectedPVSpec.VolumeMode) { - t.Errorf("test %q: expected volumeMode: %v, got: %v", k, tc.expectedPVSpec.VolumeMode, pv.Spec.VolumeMode) - } - - if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) { - t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity) - } - - if tc.expectedPVSpec.CSIPVS != nil { - if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) { - t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI) - } - } - - } + runProvisionTest(t, k, tc, requestedBytes) } } @@ -1299,6 +1358,125 @@ func newSnapshot(name, className, boundToContent, snapshotUID, claimName string, return &snapshot } +func runProvisionTest(t *testing.T, k string, tc provisioningTestcase, requestedBytes int64) { + t.Logf("Running test: %v", k) + + mockController, driver, identityServer, controllerServer, csiConn, err := createMockServer(t) + if err != nil { + t.Fatal(err) + } + defer mockController.Finish() + defer driver.Stop() + + var clientSet kubernetes.Interface + + if tc.withSecretRefs { + clientSet = fakeclientset.NewSimpleClientset(&v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ctrlpublishsecret", + Namespace: "default", + }, + }, &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nodestagesecret", + Namespace: "default", + }, + }, &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nodepublishsecret", + Namespace: "default", + }, + }) + } else { + clientSet = fakeclientset.NewSimpleClientset() + } + + csiProvisioner := NewCSIProvisioner(clientSet, nil, driver.Address(), 5*time.Second, "test-provisioner", "test", 5, csiConn.conn, nil) + + out := &csi.CreateVolumeResponse{ + Volume: &csi.Volume{ + CapacityBytes: requestedBytes, + VolumeId: "test-volume-id", + }, + } + + if tc.withSecretRefs { + tc.volOpts.Parameters[controllerPublishSecretNameKey] = "ctrlpublishsecret" + tc.volOpts.Parameters[controllerPublishSecretNamespaceKey] = "default" + tc.volOpts.Parameters[nodeStageSecretNameKey] = "nodestagesecret" + tc.volOpts.Parameters[nodeStageSecretNamespaceKey] = "default" + tc.volOpts.Parameters[nodePublishSecretNameKey] = "nodepublishsecret" + tc.volOpts.Parameters[nodePublishSecretNamespaceKey] = "default" + } + + if tc.notNilSelector { + tc.volOpts.PVC.Spec.Selector = &metav1.LabelSelector{} + } else if tc.driverNotReady { + identityServer.EXPECT().GetPluginCapabilities(gomock.Any(), gomock.Any()).Return(nil, errors.New("driver not ready")).Times(1) + } else if tc.makeVolumeNameErr { + tc.volOpts.PVC.ObjectMeta.UID = "" + provisionMockServerSetupExpectations(identityServer, controllerServer) + } else if tc.getSecretRefErr { + tc.volOpts.Parameters[provisionerSecretNameKey] = "" + provisionMockServerSetupExpectations(identityServer, controllerServer) + } else if tc.getCredentialsErr { + tc.volOpts.Parameters[provisionerSecretNameKey] = "secretx" + tc.volOpts.Parameters[provisionerSecretNamespaceKey] = "default" + provisionMockServerSetupExpectations(identityServer, controllerServer) + } else if tc.volWithLessCap { + out.Volume.CapacityBytes = int64(80) + provisionMockServerSetupExpectations(identityServer, controllerServer) + controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) + controllerServer.EXPECT().DeleteVolume(gomock.Any(), gomock.Any()).Return(&csi.DeleteVolumeResponse{}, nil).Times(1) + } else if tc.expectCreateVolDo != nil { + provisionMockServerSetupExpectations(identityServer, controllerServer) + controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Do(tc.expectCreateVolDo).Return(out, nil).Times(1) + } else { + // Setup regular mock call expectations. + provisionMockServerSetupExpectations(identityServer, controllerServer) + if !tc.expectErr { + controllerServer.EXPECT().CreateVolume(gomock.Any(), gomock.Any()).Return(out, nil).Times(1) + } + } + + pv, err := csiProvisioner.Provision(tc.volOpts) + 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) + } + + if tc.expectedPVSpec != nil { + if pv.Name != tc.expectedPVSpec.Name { + t.Errorf("test %q: expected PV name: %q, got: %q", k, tc.expectedPVSpec.Name, pv.Name) + } + + if pv.Spec.PersistentVolumeReclaimPolicy != tc.expectedPVSpec.ReclaimPolicy { + t.Errorf("test %q: expected reclaim policy: %v, got: %v", k, tc.expectedPVSpec.ReclaimPolicy, pv.Spec.PersistentVolumeReclaimPolicy) + } + + if !reflect.DeepEqual(pv.Spec.AccessModes, tc.expectedPVSpec.AccessModes) { + t.Errorf("test %q: expected access modes: %v, got: %v", k, tc.expectedPVSpec.AccessModes, pv.Spec.AccessModes) + } + + if !reflect.DeepEqual(pv.Spec.VolumeMode, tc.expectedPVSpec.VolumeMode) { + t.Errorf("test %q: expected volumeMode: %v, got: %v", k, tc.expectedPVSpec.VolumeMode, pv.Spec.VolumeMode) + } + + if !reflect.DeepEqual(pv.Spec.Capacity, tc.expectedPVSpec.Capacity) { + t.Errorf("test %q: expected capacity: %v, got: %v", k, tc.expectedPVSpec.Capacity, pv.Spec.Capacity) + } + + if tc.expectedPVSpec.CSIPVS != nil { + if !reflect.DeepEqual(pv.Spec.PersistentVolumeSource.CSI, tc.expectedPVSpec.CSIPVS) { + t.Errorf("test %q: expected PV: %v, got: %v", k, tc.expectedPVSpec.CSIPVS, pv.Spec.PersistentVolumeSource.CSI) + } + } + + } +} + // newContent returns a new content with given attributes func newContent(name, className, snapshotHandle, volumeUID, volumeName, boundToSnapshotUID, boundToSnapshotName string, size *int64, creationTime *int64) *crdv1.VolumeSnapshotContent { content := crdv1.VolumeSnapshotContent{