diff --git a/pkg/controller/csi_handler.go b/pkg/controller/csi_handler.go index 9cb065d4..3a00e76e 100644 --- a/pkg/controller/csi_handler.go +++ b/pkg/controller/csi_handler.go @@ -65,7 +65,11 @@ func (handler *csiHandler) CreateSnapshot(snapshot *crdv1.VolumeSnapshot, volume if err != nil { return "", "", 0, 0, false, err } - return handler.csiConnection.CreateSnapshot(ctx, snapshotName, volume, parameters, snapshotterCredentials) + newParameters, err := removePrefixedParameters(parameters) + if err != nil { + return "", "", 0, 0, false, fmt.Errorf("failed to remove CSI Parameters of prefixed keys: %v", err) + } + return handler.csiConnection.CreateSnapshot(ctx, snapshotName, volume, newParameters, snapshotterCredentials) } func (handler *csiHandler) DeleteSnapshot(content *crdv1.VolumeSnapshotContent, snapshotterCredentials map[string]string) error { diff --git a/pkg/controller/snapshot_controller.go b/pkg/controller/snapshot_controller.go index 3418b28c..22a57cd5 100644 --- a/pkg/controller/snapshot_controller.go +++ b/pkg/controller/snapshot_controller.go @@ -542,11 +542,11 @@ func (ctrl *csiSnapshotController) getCreateSnapshotInput(snapshot *crdv1.Volume contentName := GetSnapshotContentNameForSnapshot(snapshot) // Resolve snapshotting secret credentials. - snapshotterSecretRef, err := GetSecretReference(class.Parameters, contentName, snapshot) + snapshotterSecretRef, err := getSecretReference(class.Parameters, contentName, snapshot) if err != nil { return nil, nil, "", nil, err } - snapshotterCredentials, err := GetCredentials(ctrl.client, snapshotterSecretRef) + snapshotterCredentials, err := getCredentials(ctrl.client, snapshotterSecretRef) if err != nil { return nil, nil, "", nil, err } @@ -710,11 +710,11 @@ func (ctrl *csiSnapshotController) deleteSnapshotContentOperation(content *crdv1 if snapshotClass, err := ctrl.classLister.Get(*snapshotClassName); err == nil { // Resolve snapshotting secret credentials. // No VolumeSnapshot is provided when resolving delete secret names, since the VolumeSnapshot may or may not exist at delete time. - snapshotterSecretRef, err := GetSecretReference(snapshotClass.Parameters, content.Name, nil) + snapshotterSecretRef, err := getSecretReference(snapshotClass.Parameters, content.Name, nil) if err != nil { return err } - snapshotterCredentials, err = GetCredentials(ctrl.client, snapshotterSecretRef) + snapshotterCredentials, err = getCredentials(ctrl.client, snapshotterSecretRef) if err != nil { return err } diff --git a/pkg/controller/snapshot_create_test.go b/pkg/controller/snapshot_create_test.go index a8861f15..7ae31041 100644 --- a/pkg/controller/snapshot_create_test.go +++ b/pkg/controller/snapshot_create_test.go @@ -230,7 +230,7 @@ func TestCreateSnapshotSync(t *testing.T) { initialContents: nocontents, expectedContents: nocontents, initialSnapshots: newSnapshotArray("snap7-2", invalidSecretClass, "", "snapuid7-2", "claim7-2", false, nil, nil, nil), - expectedSnapshots: newSnapshotArray("snap7-2", invalidSecretClass, "", "snapuid7-2", "claim7-2", false, newVolumeError("Failed to create snapshot: failed to get input parameters to create snapshot snap7-2: \"csiSnapshotterSecretName and csiSnapshotterSecretNamespace parameters must be specified together\""), nil, nil), + expectedSnapshots: newSnapshotArray("snap7-2", invalidSecretClass, "", "snapuid7-2", "claim7-2", false, newVolumeError("Failed to create snapshot: failed to get input parameters to create snapshot snap7-2: \"failed to get name and namespace template from params: either name and namespace for Snapshotter secrets specified, Both must be specified\""), nil, nil), initialClaims: newClaimArray("claim7-2", "pvc-uid7-2", "1Gi", "volume7-2", v1.ClaimBound, &classEmpty), initialVolumes: newVolumeArray("volume7-2", "pv-uid7-2", "pv-handle7-2", "1Gi", "pvc-uid7-2", "claim7-2", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty), expectedEvents: []string{"Warning SnapshotCreationFailed"}, diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 498d0dba..570099c4 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -18,6 +18,8 @@ package controller import ( "fmt" + "strings" + "github.com/golang/glog" crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1" "k8s.io/api/core/v1" @@ -37,12 +39,41 @@ var ( keyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc ) -const snapshotterSecretNameKey = "csiSnapshotterSecretName" -const snapshotterSecretNamespaceKey = "csiSnapshotterSecretNamespace" +type deprecatedSecretParamsMap struct { + name string + deprecatedSecretNameKey string + deprecatedSecretNamespaceKey string + secretNameKey string + secretNamespaceKey string +} -// Name of finalizer on VolumeSnapshotContents that are bound by VolumeSnapshots -const VolumeSnapshotContentFinalizer = "snapshot.storage.kubernetes.io/volumesnapshotcontent-protection" -const VolumeSnapshotFinalizer = "snapshot.storage.kubernetes.io/volumesnapshot-protection" +const ( + // CSI Parameters prefixed with csiParameterPrefix are not passed through + // to the driver on CreateSnapshotRequest calls. Instead they are intended + // to be used by the CSI external-snapshotter and maybe used to populate + // fields in subsequent CSI calls or Kubernetes API objects. + csiParameterPrefix = "csi.storage.k8s.io/" + + prefixedSnapshotterSecretNameKey = csiParameterPrefix + "snapshotter-secret-name" + prefixedSnapshotterSecretNamespaceKey = csiParameterPrefix + "snapshotter-secret-namespace" + + // [Deprecated] CSI Parameters that are put into fields but + // NOT stripped from the parameters passed to CreateSnapshot + snapshotterSecretNameKey = "csiSnapshotterSecretName" + snapshotterSecretNamespaceKey = "csiSnapshotterSecretNamespace" + + // Name of finalizer on VolumeSnapshotContents that are bound by VolumeSnapshots + VolumeSnapshotContentFinalizer = "snapshot.storage.kubernetes.io/volumesnapshotcontent-protection" + VolumeSnapshotFinalizer = "snapshot.storage.kubernetes.io/volumesnapshot-protection" +) + +var snapshotterSecretParams = deprecatedSecretParamsMap{ + name: "Snapshotter", + deprecatedSecretNameKey: snapshotterSecretNameKey, + deprecatedSecretNamespaceKey: snapshotterSecretNamespaceKey, + secretNameKey: prefixedSnapshotterSecretNameKey, + secretNamespaceKey: prefixedSnapshotterSecretNamespaceKey, +} func snapshotKey(vs *crdv1.VolumeSnapshot) string { return fmt.Sprintf("%s/%s", vs.Namespace, vs.Name) @@ -130,9 +161,54 @@ func IsDefaultAnnotation(obj metav1.ObjectMeta) bool { return false } -// 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. -// no lookup of the referenced secret is performed, and the secret may or may not exist. +// verifyAndGetSecretNameAndNamespaceTemplate gets the values (templates) associated +// with the parameters specified in "secret" and verifies that they are specified correctly. +func verifyAndGetSecretNameAndNamespaceTemplate(secret deprecatedSecretParamsMap, snapshotClassParams map[string]string) (nameTemplate, namespaceTemplate string, err error) { + numName := 0 + numNamespace := 0 + if t, ok := snapshotClassParams[secret.deprecatedSecretNameKey]; ok { + nameTemplate = t + numName += 1 + glog.Warning(deprecationWarning(secret.deprecatedSecretNameKey, secret.secretNameKey, "")) + } + if t, ok := snapshotClassParams[secret.deprecatedSecretNamespaceKey]; ok { + namespaceTemplate = t + numNamespace += 1 + glog.Warning(deprecationWarning(secret.deprecatedSecretNamespaceKey, secret.secretNamespaceKey, "")) + } + if t, ok := snapshotClassParams[secret.secretNameKey]; ok { + nameTemplate = t + numName += 1 + } + if t, ok := snapshotClassParams[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("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: // - ${volumesnapshotcontent.name} @@ -145,20 +221,17 @@ func IsDefaultAnnotation(obj metav1.ObjectMeta) bool { // - ${volumesnapshot.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(snapshotClassParams map[string]string, snapContentName string, snapshot *crdv1.VolumeSnapshot) (*v1.SecretReference, error) { - nameTemplate, hasName := snapshotClassParams[snapshotterSecretNameKey] - namespaceTemplate, hasNamespace := snapshotClassParams[snapshotterSecretNamespaceKey] - - if !hasName && !hasNamespace { - return nil, nil +func getSecretReference(snapshotClassParams map[string]string, snapContentName string, snapshot *crdv1.VolumeSnapshot) (*v1.SecretReference, error) { + nameTemplate, namespaceTemplate, err := verifyAndGetSecretNameAndNamespaceTemplate(snapshotterSecretParams, snapshotClassParams) + 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", snapshotterSecretNameKey, snapshotterSecretNamespaceKey) + if nameTemplate == "" && namespaceTemplate == "" { + return nil, nil } ref := &v1.SecretReference{} @@ -174,15 +247,15 @@ func GetSecretReference(snapshotClassParams map[string]string, snapContentName s resolvedNamespace, err := resolveTemplate(namespaceTemplate, namespaceParams) if err != nil { - return nil, fmt.Errorf("error resolving %s value %q: %v", snapshotterSecretNamespaceKey, namespaceTemplate, err) + return nil, fmt.Errorf("error resolving value %q: %v", namespaceTemplate, err) } glog.V(4).Infof("GetSecretReference namespaceTemplate %s, namespaceParams: %+v, resolved %s", namespaceTemplate, namespaceParams, resolvedNamespace) 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", snapshotterSecretNamespaceKey, 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", snapshotterSecretNamespaceKey, namespaceTemplate) + return nil, fmt.Errorf("%q is not a valid namespace name", namespaceTemplate) } ref.Namespace = resolvedNamespace @@ -199,13 +272,13 @@ func GetSecretReference(snapshotClassParams map[string]string, snapContentName s } resolvedName, err := resolveTemplate(nameTemplate, nameParams) if err != nil { - return nil, fmt.Errorf("error resolving %s value %q: %v", snapshotterSecretNameKey, 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", snapshotterSecretNameKey, 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", snapshotterSecretNameKey, nameTemplate) + return nil, fmt.Errorf("%q is not a valid secret name", nameTemplate) } ref.Name = resolvedName @@ -229,8 +302,8 @@ func resolveTemplate(template string, params map[string]string) (string, error) return resolved, nil } -// GetCredentials retrieves credentials stored in v1.SecretReference -func GetCredentials(k8s kubernetes.Interface, ref *v1.SecretReference) (map[string]string, error) { +// getCredentials retrieves credentials stored in v1.SecretReference +func getCredentials(k8s kubernetes.Interface, ref *v1.SecretReference) (map[string]string, error) { if ref == nil { return nil, nil } @@ -271,3 +344,34 @@ func isSnapshotDeletionCandidate(snapshot *crdv1.VolumeSnapshot) bool { func needToAddSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot) bool { return snapshot.ObjectMeta.DeletionTimestamp == nil && !slice.ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotFinalizer, nil) } + +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 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 prefixedSnapshotterSecretNameKey: + case prefixedSnapshotterSecretNamespaceKey: + 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 +} diff --git a/pkg/controller/util_test.go b/pkg/controller/util_test.go index dd280905..bfb64e6b 100644 --- a/pkg/controller/util_test.go +++ b/pkg/controller/util_test.go @@ -17,7 +17,6 @@ limitations under the License. package controller import ( - "fmt" crdv1 "github.com/kubernetes-csi/external-snapshotter/pkg/apis/volumesnapshot/v1alpha1" "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -31,53 +30,50 @@ func TestGetSecretReference(t *testing.T) { snapContentName string snapshot *crdv1.VolumeSnapshot expectRef *v1.SecretReference - expectErr error + expectErr bool }{ "no params": { params: nil, expectRef: nil, - expectErr: nil, }, "empty err": { params: map[string]string{snapshotterSecretNameKey: "", snapshotterSecretNamespaceKey: ""}, - expectErr: fmt.Errorf("csiSnapshotterSecretName and csiSnapshotterSecretNamespace parameters must be specified together"), + expectErr: true, }, - "name, no namespace": { + "[deprecated] name, no namespace": { params: map[string]string{snapshotterSecretNameKey: "foo"}, - expectErr: fmt.Errorf("csiSnapshotterSecretName and csiSnapshotterSecretNamespace parameters must be specified together"), + expectErr: true, }, "namespace, no name": { - params: map[string]string{snapshotterSecretNamespaceKey: "foo"}, - expectErr: fmt.Errorf("csiSnapshotterSecretName and csiSnapshotterSecretNamespace parameters must be specified together"), + params: map[string]string{prefixedSnapshotterSecretNamespaceKey: "foo"}, + expectErr: true, }, "simple - valid": { - params: map[string]string{snapshotterSecretNameKey: "name", snapshotterSecretNamespaceKey: "ns"}, + params: map[string]string{prefixedSnapshotterSecretNameKey: "name", prefixedSnapshotterSecretNamespaceKey: "ns"}, snapshot: &crdv1.VolumeSnapshot{}, expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"}, - expectErr: nil, }, - "simple - valid, no pvc": { + "[deprecated] simple - valid, no pvc": { params: map[string]string{snapshotterSecretNameKey: "name", snapshotterSecretNamespaceKey: "ns"}, snapshot: nil, expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"}, - expectErr: nil, }, "simple - invalid name": { - params: map[string]string{snapshotterSecretNameKey: "bad name", snapshotterSecretNamespaceKey: "ns"}, + params: map[string]string{prefixedSnapshotterSecretNameKey: "bad name", prefixedSnapshotterSecretNamespaceKey: "ns"}, snapshot: &crdv1.VolumeSnapshot{}, expectRef: nil, - expectErr: fmt.Errorf(`csiSnapshotterSecretName parameter "bad name" is not a valid secret name`), + expectErr: true, }, - "simple - invalid namespace": { + "[deprecated] simple - invalid namespace": { params: map[string]string{snapshotterSecretNameKey: "name", snapshotterSecretNamespaceKey: "bad ns"}, snapshot: &crdv1.VolumeSnapshot{}, expectRef: nil, - expectErr: fmt.Errorf(`csiSnapshotterSecretNamespace parameter "bad ns" is not a valid namespace name`), + expectErr: true, }, "template - valid": { params: map[string]string{ - snapshotterSecretNameKey: "static-${volumesnapshotcontent.name}-${volumesnapshot.namespace}-${volumesnapshot.name}-${volumesnapshot.annotations['akey']}", - snapshotterSecretNamespaceKey: "static-${volumesnapshotcontent.name}-${volumesnapshot.namespace}", + prefixedSnapshotterSecretNameKey: "static-${volumesnapshotcontent.name}-${volumesnapshot.namespace}-${volumesnapshot.name}-${volumesnapshot.annotations['akey']}", + prefixedSnapshotterSecretNamespaceKey: "static-${volumesnapshotcontent.name}-${volumesnapshot.namespace}", }, snapContentName: "snapcontentname", snapshot: &crdv1.VolumeSnapshot{ @@ -88,7 +84,6 @@ func TestGetSecretReference(t *testing.T) { }, }, expectRef: &v1.SecretReference{Name: "static-snapcontentname-snapshotnamespace-snapshotname-avalue", Namespace: "static-snapcontentname-snapshotnamespace"}, - expectErr: nil, }, "template - invalid namespace tokens": { params: map[string]string{ @@ -97,7 +92,7 @@ func TestGetSecretReference(t *testing.T) { }, snapshot: &crdv1.VolumeSnapshot{}, expectRef: nil, - expectErr: fmt.Errorf(`error resolving csiSnapshotterSecretNamespace value "mynamespace${bar}": invalid tokens: ["bar"]`), + expectErr: true, }, "template - invalid name tokens": { params: map[string]string{ @@ -106,15 +101,23 @@ func TestGetSecretReference(t *testing.T) { }, snapshot: &crdv1.VolumeSnapshot{}, expectRef: nil, - expectErr: fmt.Errorf(`error resolving csiSnapshotterSecretName value "myname${foo}": invalid tokens: ["foo"]`), + expectErr: true, }, } for k, tc := range testcases { t.Run(k, func(t *testing.T) { - ref, err := GetSecretReference(tc.params, tc.snapContentName, tc.snapshot) - if !reflect.DeepEqual(err, tc.expectErr) { - t.Errorf("Expected %v, got %v", tc.expectErr, err) + ref, err := getSecretReference(tc.params, tc.snapContentName, tc.snapshot) + 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) @@ -122,3 +125,71 @@ func TestGetSecretReference(t *testing.T) { }) } } + +func TestRemovePrefixedCSIParams(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{prefixedSnapshotterSecretNameKey: "bar", "bim": "baz"}, + expectedParams: map[string]string{"bim": "baz"}, + }, + { + name: "all known prefixed", + params: map[string]string{ + prefixedSnapshotterSecretNameKey: "csiBar", + prefixedSnapshotterSecretNamespaceKey: "csiBar", + }, + expectedParams: map[string]string{}, + }, + { + name: "all known deprecated params not stripped", + params: map[string]string{ + snapshotterSecretNameKey: "csiBar", + snapshotterSecretNamespaceKey: "csiBar", + }, + expectedParams: map[string]string{ + snapshotterSecretNameKey: "csiBar", + snapshotterSecretNamespaceKey: "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 paramaters: %v not equal to expected paramaters: %v", newParams, tc.expectedParams) + } + } +}