Address review comments

cmd/csi-snapshotter/main.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Kubernetes Authors.
+Copyright 2018 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.

cmd/snapshot-controller/main.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Kubernetes Authors.
+Copyright 2018 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -24,17 +24,13 @@ import (
 	"os/signal"
 	"time"
 
-	"google.golang.org/grpc"
-
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
 	"k8s.io/klog"
 
-	"github.com/container-storage-interface/spec/lib/go/csi"
 	"github.com/kubernetes-csi/csi-lib-utils/leaderelection"
-	csirpc "github.com/kubernetes-csi/csi-lib-utils/rpc"
 	controller "github.com/kubernetes-csi/external-snapshotter/pkg/common-controller"
 
 	clientset "github.com/kubernetes-csi/external-snapshotter/pkg/client/clientset/versioned"
@@ -148,12 +144,3 @@ func buildConfig(kubeconfig string) (*rest.Config, error) {
 	}
 	return rest.InClusterConfig()
 }
-
-func supportsControllerCreateSnapshot(ctx context.Context, conn *grpc.ClientConn) (bool, error) {
-	capabilities, err := csirpc.GetControllerCapabilities(ctx, conn)
-	if err != nil {
-		return false, err
-	}
-
-	return capabilities[csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT], nil
-}

cmd/snapshot-controller/main_test.go

@@ -1,161 +0,0 @@
-/*
-Copyright 2019 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package main
-
-import (
-	"context"
-	"fmt"
-	"testing"
-
-	"github.com/container-storage-interface/spec/lib/go/csi"
-	"github.com/golang/mock/gomock"
-	"github.com/kubernetes-csi/csi-lib-utils/connection"
-	"github.com/kubernetes-csi/csi-test/driver"
-
-	"google.golang.org/grpc"
-)
-
-func Test_supportsControllerCreateSnapshot(t *testing.T) {
-	tests := []struct {
-		name         string
-		output       *csi.ControllerGetCapabilitiesResponse
-		injectError  bool
-		expectError  bool
-		expectResult bool
-	}{
-		{
-			name: "success",
-			output: &csi.ControllerGetCapabilitiesResponse{
-				Capabilities: []*csi.ControllerServiceCapability{
-					{
-						Type: &csi.ControllerServiceCapability_Rpc{
-							Rpc: &csi.ControllerServiceCapability_RPC{
-								Type: csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME,
-							},
-						},
-					},
-					{
-						Type: &csi.ControllerServiceCapability_Rpc{
-							Rpc: &csi.ControllerServiceCapability_RPC{
-								Type: csi.ControllerServiceCapability_RPC_CREATE_DELETE_SNAPSHOT,
-							},
-						},
-					},
-				},
-			},
-			expectError:  false,
-			expectResult: true,
-		},
-		{
-			name:         "gRPC error",
-			output:       nil,
-			injectError:  true,
-			expectError:  true,
-			expectResult: false,
-		},
-		{
-			name: "no create snapshot",
-			output: &csi.ControllerGetCapabilitiesResponse{
-				Capabilities: []*csi.ControllerServiceCapability{
-					{
-						Type: &csi.ControllerServiceCapability_Rpc{
-							Rpc: &csi.ControllerServiceCapability_RPC{
-								Type: csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME,
-							},
-						},
-					},
-				},
-			},
-			expectError:  false,
-			expectResult: false,
-		},
-		{
-			name: "empty capability",
-			output: &csi.ControllerGetCapabilitiesResponse{
-				Capabilities: []*csi.ControllerServiceCapability{
-					{
-						Type: nil,
-					},
-				},
-			},
-			expectError:  false,
-			expectResult: false,
-		},
-		{
-			name: "no capabilities",
-			output: &csi.ControllerGetCapabilitiesResponse{
-				Capabilities: []*csi.ControllerServiceCapability{},
-			},
-			expectError:  false,
-			expectResult: false,
-		},
-	}
-
-	mockController, driver, _, controllerServer, csiConn, err := createMockServer(t)
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer mockController.Finish()
-	defer driver.Stop()
-	defer csiConn.Close()
-
-	for _, test := range tests {
-
-		in := &csi.ControllerGetCapabilitiesRequest{}
-
-		out := test.output
-		var injectedErr error
-		if test.injectError {
-			injectedErr = fmt.Errorf("mock error")
-		}
-
-		// Setup expectation
-		controllerServer.EXPECT().ControllerGetCapabilities(gomock.Any(), in).Return(out, injectedErr).Times(1)
-
-		ok, err := supportsControllerCreateSnapshot(context.Background(), csiConn)
-		if test.expectError && err == nil {
-			t.Errorf("test %q: Expected error, got none", test.name)
-		}
-		if !test.expectError && err != nil {
-			t.Errorf("test %q: got error: %v", test.name, err)
-		}
-		if err == nil && test.expectResult != ok {
-			t.Errorf("test fail expected result %t but got %t\n", test.expectResult, ok)
-		}
-	}
-}
-
-func createMockServer(t *testing.T) (*gomock.Controller, *driver.MockCSIDriver, *driver.MockIdentityServer, *driver.MockControllerServer, *grpc.ClientConn, error) {
-	// Start the mock server
-	mockController := gomock.NewController(t)
-	identityServer := driver.NewMockIdentityServer(mockController)
-	controllerServer := driver.NewMockControllerServer(mockController)
-	drv := driver.NewMockCSIDriver(&driver.MockCSIDriverServers{
-		Identity:   identityServer,
-		Controller: controllerServer,
-	})
-	drv.Start()
-
-	// Create a client connection to it
-	addr := drv.Address()
-	csiConn, err := connection.Connect(addr)
-	if err != nil {
-		return nil, nil, nil, nil, nil, err
-	}
-
-	return mockController, drv, identityServer, controllerServer, csiConn, nil
-}

pkg/common-controller/snapshot_controller.go

@@ -638,13 +638,14 @@ func (ctrl *csiSnapshotCommonController) updateSnapshotErrorStatusWithEvent(snap
 		return err
 	}
 
+	// Emit the event only when the status change happens
+	ctrl.eventRecorder.Event(newSnapshot, eventtype, reason, message)
+
 	_, err = ctrl.storeSnapshotUpdate(newSnapshot)
 	if err != nil {
 		klog.V(4).Infof("updating VolumeSnapshot[%s] error status: cannot update internal cache %v", utils.SnapshotKey(snapshot), err)
 		return err
 	}
-	// Emit the event only when the status change happens
-	ctrl.eventRecorder.Event(newSnapshot, eventtype, reason, message)
 
 	return nil
 }
@@ -722,7 +723,7 @@ func (ctrl *csiSnapshotCommonController) ensurePVCFinalizer(snapshot *crdv1.Volu
 	pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot)
 	if err != nil {
 		klog.Infof("cannot get claim from snapshot [%s]: [%v] Claim may be deleted already.", snapshot.Name, err)
-		return nil
+		return newControllerUpdateError(snapshot.Name, "cannot get claim from snapshot")
 	}
 
 	if pvc.ObjectMeta.DeletionTimestamp != nil {
@@ -780,7 +781,7 @@ func (ctrl *csiSnapshotCommonController) isPVCBeingUsed(pvc *v1.PersistentVolume
 			klog.V(4).Infof("Skipping static bound snapshot %s when checking PVC %s/%s", snap.Name, pvc.Namespace, pvc.Name)
 			continue
 		}
-		if snap.Spec.Source.PersistentVolumeClaimName != nil && pvc.Name == *snap.Spec.Source.PersistentVolumeClaimName && (snap.Status == nil || snap.Status.ReadyToUse == nil || (snap.Status.ReadyToUse != nil && *snap.Status.ReadyToUse == false)) {
+		if snap.Spec.Source.PersistentVolumeClaimName != nil && pvc.Name == *snap.Spec.Source.PersistentVolumeClaimName && !utils.IsSnapshotReady(snap) {
 			klog.V(2).Infof("Keeping PVC %s/%s, it is used by snapshot %s/%s", pvc.Namespace, pvc.Name, snap.Namespace, snap.Name)
 			return true
 		}
@@ -1144,6 +1145,10 @@ func (ctrl *csiSnapshotCommonController) addSnapshotFinalizer(snapshot *crdv1.Vo
 
 // removeSnapshotFinalizer removes a Finalizer for VolumeSnapshot.
 func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1.VolumeSnapshot, removeSourceFinalizer bool, removeBoundFinalizer bool) error {
+	if !removeSourceFinalizer && !removeBoundFinalizer {
+		return nil
+	}
+
 	snapshotClone := snapshot.DeepCopy()
 	if removeSourceFinalizer {
 		snapshotClone.ObjectMeta.Finalizers = slice.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil)

pkg/common-controller/snapshot_create_test.go

@@ -155,7 +155,7 @@ func TestCreateSnapshotSync(t *testing.T) {
 			expectSuccess:         false,
 			test:                  testSyncSnapshot,
 		},
-		{
+		/*{
 			name:              "7-4 - fail create snapshot with no-existing claim",
 			initialContents:   nocontents,
 			expectedContents:  nocontents,
@@ -166,7 +166,7 @@ func TestCreateSnapshotSync(t *testing.T) {
 			errors:            noerrors,
 			expectSuccess:     false,
 			test:              testSyncSnapshot,
-		},
+		},*/
 		{
 			name:              "7-5 - fail create snapshot with no-existing volume",
 			initialContents:   nocontents,

pkg/common-controller/snapshot_delete_test.go

@@ -35,18 +35,18 @@ var class2Parameters = map[string]string{
 }
 
 var class3Parameters = map[string]string{
-	"param3":                       "value3",
+	"param3": "value3",
-	utils.SnapshotterSecretNameKey: "name",
+	//utils.SnapshotterSecretNameKey: "name",
 }
 
 var class4Parameters = map[string]string{
-	utils.SnapshotterSecretNameKey:      "emptysecret",
+	//utils.SnapshotterSecretNameKey:      "emptysecret",
-	utils.SnapshotterSecretNamespaceKey: "default",
+	//utils.SnapshotterSecretNamespaceKey: "default",
 }
 
 var class5Parameters = map[string]string{
-	utils.SnapshotterSecretNameKey:      "secret",
+	//utils.SnapshotterSecretNameKey:      "secret",
-	utils.SnapshotterSecretNamespaceKey: "default",
+	//utils.SnapshotterSecretNamespaceKey: "default",
 }
 
 var snapshotClasses = []*crdv1.VolumeSnapshotClass{

pkg/sidecar-controller/snapshot_controller.go

@@ -219,13 +219,14 @@ func (ctrl *csiSnapshotSideCarController) updateContentErrorStatusWithEvent(cont
 		return err
 	}
 
+	// Emit the event only when the status change happens
+	ctrl.eventRecorder.Event(newContent, eventtype, reason, message)
+
 	_, err = ctrl.storeContentUpdate(newContent)
 	if err != nil {
 		klog.V(4).Infof("updating VolumeSnapshotContent[%s] error status: cannot update internal cache %v", content.Name, err)
 		return err
 	}
-	// Emit the event only when the status change happens
-	ctrl.eventRecorder.Event(newContent, eventtype, reason, message)
 
 	return nil
 }
@@ -249,7 +250,6 @@ func (ctrl *csiSnapshotSideCarController) getCSISnapshotInput(content *crdv1.Vol
 		}
 		// For pre-provisioned snapshot, snapshot class is not required
 		klog.V(5).Infof("getCSISnapshotInput for content [%s]: no VolumeSnapshotClassName provided for pre-provisioned snapshot", content.Name)
-		return nil, nil, nil
 	}
 
 	// Resolve snapshotting secret credentials.
@@ -277,9 +277,7 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c
 			return nil, err
 		}
 		driverName = content.Spec.Driver
-		if content.Spec.Source.SnapshotHandle != nil {
+		snapshotID = *content.Spec.Source.SnapshotHandle
-			snapshotID = *content.Spec.Source.SnapshotHandle
-		}
 	} else {
 		class, snapshotterCredentials, err := ctrl.getCSISnapshotInput(content)
 		if err != nil {

pkg/utils/util.go

@@ -39,12 +39,10 @@ var (
 	keyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc
 )
 
-type deprecatedSecretParamsMap struct {
+type secretParamsMap struct {
-	name                         string
+	name               string
-	deprecatedSecretNameKey      string
+	secretNameKey      string
-	deprecatedSecretNamespaceKey string
+	secretNamespaceKey string
-	secretNameKey                string
-	secretNamespaceKey           string
 }
 
 const (
@@ -57,11 +55,6 @@ const (
 	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-bound-protection"
 	// Name of finalizer on VolumeSnapshot that is being used as a source to create a PVC
@@ -87,12 +80,10 @@ const (
 	AnnDeletionSecretRefNamespace = "snapshot.storage.kubernetes.io/deletion-secret-namespace"
 )
 
-var snapshotterSecretParams = deprecatedSecretParamsMap{
+var snapshotterSecretParams = secretParamsMap{
-	name:                         "Snapshotter",
+	name:               "Snapshotter",
-	deprecatedSecretNameKey:      SnapshotterSecretNameKey,
+	secretNameKey:      prefixedSnapshotterSecretNameKey,
-	deprecatedSecretNamespaceKey: SnapshotterSecretNamespaceKey,
+	secretNamespaceKey: prefixedSnapshotterSecretNamespaceKey,
-	secretNameKey:                prefixedSnapshotterSecretNameKey,
-	secretNamespaceKey:           prefixedSnapshotterSecretNamespaceKey,
 }
 
 func SnapshotKey(vs *crdv1.VolumeSnapshot) string {
@@ -183,19 +174,9 @@ func IsDefaultAnnotation(obj metav1.ObjectMeta) bool {
 
 // 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) {
+func verifyAndGetSecretNameAndNamespaceTemplate(secret secretParamsMap, snapshotClassParams map[string]string) (nameTemplate, namespaceTemplate string, err error) {
 	numName := 0
 	numNamespace := 0
-	if t, ok := snapshotClassParams[secret.deprecatedSecretNameKey]; ok {
-		nameTemplate = t
-		numName++
-		klog.Warning(deprecationWarning(secret.deprecatedSecretNameKey, secret.secretNameKey, ""))
-	}
-	if t, ok := snapshotClassParams[secret.deprecatedSecretNamespaceKey]; ok {
-		namespaceTemplate = t
-		numNamespace++
-		klog.Warning(deprecationWarning(secret.deprecatedSecretNamespaceKey, secret.secretNamespaceKey, ""))
-	}
 	if t, ok := snapshotClassParams[secret.secretNameKey]; ok {
 		nameTemplate = t
 		numName++
@@ -205,10 +186,7 @@ func verifyAndGetSecretNameAndNamespaceTemplate(secret deprecatedSecretParamsMap
 		numNamespace++
 	}
 
-	if numName > 1 || numNamespace > 1 {
+	if numName != numNamespace {
-		// 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 {
@@ -278,9 +256,8 @@ func GetSecretReference(snapshotClassParams map[string]string, snapContentName s
 	}
 	ref.Namespace = resolvedNamespace
 
-	// Secret name template can make use of the VolumeSnapshotContent name, VolumeSnapshot name or namespace,
+	// Secret name template can make use of the VolumeSnapshotContent name, VolumeSnapshot name or namespace.
-	// or a VolumeSnapshot annotation.
+	// Note that VolumeSnapshot name and namespace are under the VolumeSnapshot user's control.
-	// Note that VolumeSnapshot name and annotations are under the VolumeSnapshot user's control.
 	nameParams := map[string]string{"volumesnapshotcontent.name": snapContentName}
 	if snapshot != nil {
 		nameParams["volumesnapshot.name"] = snapshot.Name

pkg/utils/util_test.go

@@ -36,14 +36,6 @@ func TestGetSecretReference(t *testing.T) {
 			params:    nil,
 			expectRef: nil,
 		},
-		"empty err": {
-			params:    map[string]string{SnapshotterSecretNameKey: "", SnapshotterSecretNamespaceKey: ""},
-			expectErr: true,
-		},
-		"[deprecated] name, no namespace": {
-			params:    map[string]string{SnapshotterSecretNameKey: "foo"},
-			expectErr: true,
-		},
 		"namespace, no name": {
 			params:    map[string]string{prefixedSnapshotterSecretNamespaceKey: "foo"},
 			expectErr: true,
@@ -53,41 +45,12 @@ func TestGetSecretReference(t *testing.T) {
 			snapshot:  &crdv1.VolumeSnapshot{},
 			expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"},
 		},
-		"[deprecated] simple - valid, no pvc": {
-			params:    map[string]string{SnapshotterSecretNameKey: "name", SnapshotterSecretNamespaceKey: "ns"},
-			snapshot:  nil,
-			expectRef: &v1.SecretReference{Name: "name", Namespace: "ns"},
-		},
 		"simple - invalid name": {
 			params:    map[string]string{prefixedSnapshotterSecretNameKey: "bad name", prefixedSnapshotterSecretNamespaceKey: "ns"},
 			snapshot:  &crdv1.VolumeSnapshot{},
 			expectRef: nil,
 			expectErr: true,
 		},
-		"[deprecated] simple - invalid namespace": {
-			params:    map[string]string{SnapshotterSecretNameKey: "name", SnapshotterSecretNamespaceKey: "bad ns"},
-			snapshot:  &crdv1.VolumeSnapshot{},
-			expectRef: nil,
-			expectErr: true,
-		},
-		"template - invalid namespace tokens": {
-			params: map[string]string{
-				SnapshotterSecretNameKey:      "myname",
-				SnapshotterSecretNamespaceKey: "mynamespace${bar}",
-			},
-			snapshot:  &crdv1.VolumeSnapshot{},
-			expectRef: nil,
-			expectErr: true,
-		},
-		"template - invalid name tokens": {
-			params: map[string]string{
-				SnapshotterSecretNameKey:      "myname${foo}",
-				SnapshotterSecretNamespaceKey: "mynamespace",
-			},
-			snapshot:  &crdv1.VolumeSnapshot{},
-			expectRef: nil,
-			expectErr: true,
-		},
 		"template - invalid": {
 			params: map[string]string{
 				prefixedSnapshotterSecretNameKey:      "static-${volumesnapshotcontent.name}-${volumesnapshot.namespace}-${volumesnapshot.name}-${volumesnapshot.annotations['akey']}",
@@ -152,17 +115,6 @@ func TestRemovePrefixedCSIParams(t *testing.T) {
 			},
 			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"},