diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index d17036f36d..5db21cc6d8 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -56,7 +56,35 @@ import ( utilfeature "k8s.io/apiserver/pkg/util/feature" ) +type secretParamsType struct { + name string + deprecatedSecretNameKey string + deprecatedSecretNamespaceKey string + secretNameKey string + secretNamespaceKey string +} + const ( + // CSI Parameters that are put into fields and are stripped + // from Parameters passed to CreateVolume + csiParameterPrefix = "csi.storage.k8s.io/" + + prefixedFsTypeKey = csiParameterPrefix + "FSType" + + prefixedProvisionerSecretNameKey = csiParameterPrefix + "ProvisionerSecretName" + prefixedProvisionerSecretNamespaceKey = csiParameterPrefix + "ProvisionerSecretNamespace" + + prefixedControllerPublishSecretNameKey = csiParameterPrefix + "ControllerPublishSecretName" + prefixedControllerPublishSecretNamespaceKey = csiParameterPrefix + "ControllerPublishSecretNamespace" + + prefixedNodeStageSecretNameKey = csiParameterPrefix + "NodeStageSecretName" + prefixedNodeStageSecretNamespaceKey = csiParameterPrefix + "NodeStageSecretNamespace" + + prefixedNodePublishSecretNameKey = csiParameterPrefix + "NodePublishSecretName" + prefixedNodePublishSecretNamespaceKey = csiParameterPrefix + "NodePublishSecretNamespace" + + // [Deprecated] CSI Parameters that are put into fields but + // NOT stripped from the parameters passed to CreateVolume provisionerSecretNameKey = "csiProvisionerSecretName" provisionerSecretNamespaceKey = "csiProvisionerSecretNamespace" @@ -82,6 +110,40 @@ const ( snapshotAPIGroup = snapapi.GroupName // "snapshot.storage.k8s.io" ) +var ( + provisionerSecretParams = secretParamsType{ + name: "Provisioner", + deprecatedSecretNameKey: provisionerSecretNameKey, + deprecatedSecretNamespaceKey: provisionerSecretNamespaceKey, + secretNameKey: prefixedProvisionerSecretNameKey, + secretNamespaceKey: prefixedProvisionerSecretNamespaceKey, + } + + nodePublishSecretParams = secretParamsType{ + name: "NodePublish", + deprecatedSecretNameKey: nodePublishSecretNameKey, + deprecatedSecretNamespaceKey: nodePublishSecretNamespaceKey, + secretNameKey: prefixedNodePublishSecretNameKey, + secretNamespaceKey: prefixedNodePublishSecretNamespaceKey, + } + + controllerPublishSecretParams = secretParamsType{ + name: "ControllerPublish", + deprecatedSecretNameKey: controllerPublishSecretNameKey, + deprecatedSecretNamespaceKey: controllerPublishSecretNamespaceKey, + secretNameKey: prefixedControllerPublishSecretNameKey, + secretNamespaceKey: prefixedControllerPublishSecretNamespaceKey, + } + + nodeStageSecretParams = secretParamsType{ + name: "NodeStage", + deprecatedSecretNameKey: nodeStageSecretNameKey, + deprecatedSecretNamespaceKey: nodeStageSecretNamespaceKey, + secretNameKey: prefixedNodeStageSecretNameKey, + secretNamespaceKey: prefixedNodeStageSecretNamespaceKey, + } +) + // CSIProvisioner struct type csiProvisioner struct { client kubernetes.Interface @@ -386,6 +448,17 @@ func getVolumeCapability( } } +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) +} + func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.PersistentVolume, error) { if options.PVC.Spec.Selector != nil { return nil, fmt.Errorf("claim Selector is not supported") @@ -415,13 +488,22 @@ 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": fsType = v + fsTypesFound += 1 + glog.Warningf(deprecationWarning("fstype", prefixedFsTypeKey, "")) + case prefixedFsTypeKey: + fsType = v + fsTypesFound += 1 } } + if fsTypesFound > 1 { + return nil, fmt.Errorf("fstype specified in paramaters with both \"fstype\" and \"%s\" keys", prefixedFsTypeKey) + } if len(fsType) == 0 { fsType = defaultFSType } @@ -474,7 +556,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 } @@ -485,19 +567,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 = stripPrefixedParameters(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) @@ -590,6 +677,32 @@ func (p *csiProvisioner) Provision(options controller.VolumeOptions) (*v1.Persis return pv, nil } +func stripPrefixedParameters(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 + 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 { @@ -665,7 +778,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 } @@ -702,8 +815,52 @@ 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. +func getSecretNameAndNamespaceTemplate(secret secretParamsType, 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 paramaters with both \"csi\" and \"%s\" keys", secret.name, csiParameterPrefix) + } else if numName != numNamespace { + // Not both 0 or both 1 + return "", "", fmt.Errorf("unequal number of name and namespace %s secrets 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 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: @@ -717,24 +874,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 secretParamsType, storageClassParams map[string]string, pvName string, pvc *v1.PersistentVolumeClaim) (*v1.SecretReference, error) { + nameTemplate, namespaceTemplate, err := getSecretNameAndNamespaceTemplate(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. @@ -745,13 +897,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 } @@ -769,13 +921,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 } diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 3bf7e386a2..4d04fce1b9 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,78 @@ 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{"foo": "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: "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 := stripPrefixedParameters(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 paramaters: %v not equal to expected paramaters: %v", newParams, tc.expectedParams) + } + } +} + func TestGetDriverCapabilities(t *testing.T) { type testcase struct { name string @@ -435,9 +512,9 @@ func TestCreateDriverReturnsInvalidCapacityDuringProvision(t *testing.T) { // Requested PVC with requestedBytes storage opts := controller.VolumeOptions{ PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, - PVName: "test-name", - PVC: createFakePVC(requestedBytes), - Parameters: map[string]string{}, + PVName: "test-name", + PVC: createFakePVC(requestedBytes), + Parameters: map[string]string{}, } // Drivers CreateVolume response with lower capacity bytes than request @@ -594,75 +671,99 @@ func createFakePVCWithVolumeMode(requestBytes int64, volumeMode v1.PersistentVol func TestGetSecretReference(t *testing.T) { testcases := map[string]struct { - nameKey string - namespaceKey string + secretParams secretParamsType 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 +777,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,41 +821,37 @@ 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, - PVName: "test-name", - PVC: createFakePVC(requestedBytes), + PVName: "test-name", + PVC: createFakePVC(requestedBytes), Parameters: map[string]string{ "fstype": "ext3", }, @@ -770,10 +872,52 @@ 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, - PVName: "test-name", + PVName: "test-name", PVC: &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ UID: "testid", @@ -821,7 +965,7 @@ func TestProvision(t *testing.T) { "provision with access mode multi node multi readonly": { volOpts: controller.VolumeOptions{ PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, - PVName: "test-name", + PVName: "test-name", PVC: &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ UID: "testid", @@ -869,7 +1013,7 @@ func TestProvision(t *testing.T) { "provision with access mode single writer": { volOpts: controller.VolumeOptions{ PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, - PVName: "test-name", + PVName: "test-name", PVC: &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ UID: "testid", @@ -917,7 +1061,7 @@ func TestProvision(t *testing.T) { "provision with multiple access modes": { volOpts: controller.VolumeOptions{ PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, - PVName: "test-name", + PVName: "test-name", PVC: &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ UID: "testid", @@ -1099,7 +1243,7 @@ func TestProvision(t *testing.T) { "provision with mount options": { volOpts: controller.VolumeOptions{ PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, - PVName: "test-name", + PVName: "test-name", PVC: &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ UID: "testid", @@ -1154,119 +1298,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 +1332,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{ @@ -1368,7 +1520,7 @@ func TestProvisionFromSnapshot(t *testing.T) { "provision with volume snapshot data source": { volOpts: controller.VolumeOptions{ PersistentVolumeReclaimPolicy: v1.PersistentVolumeReclaimDelete, - PVName: "test-name", + PVName: "test-name", PVC: &v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ UID: "testid",