Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the feature to use secrets of ListSnapshots #252

Merged
merged 8 commits into from
Feb 21, 2020
Merged

Added the feature to use secrets of ListSnapshots #252

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3b76de4
Added the feature to use secrets of ListSnapshots
bells17 Feb 8, 2020
4017329
Improve error messages
bells17 Feb 19, 2020
2045ef1
Fix error message s/Delete/List/
bells17 Feb 19, 2020
0a1579d
Add support for list secret parameters to RemovePrefixedParameters
bells17 Feb 19, 2020
dd390a3
Improve error messages
bells17 Feb 19, 2020
8e9680a
Modify to upper case variable name from lower case for unit tests
bells17 Feb 20, 2020
7cbcf6a
Fix log messages and error messages
bells17 Feb 20, 2020
3c2baf4
Add comments
bells17 Feb 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ module github.com/kubernetes-csi/external-snapshotter/v2
go 1.12

require (
github.com/container-storage-interface/spec v1.1.0
github.com/container-storage-interface/spec v1.2.0
github.com/golang/mock v1.2.0
github.com/golang/protobuf v1.3.2
github.com/google/go-cmp v0.3.1 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ github.com/census-instrumentation/opencensus-proto v0.2.1/go.mod h1:f6KPmirojxKA
github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw=
github.com/container-storage-interface/spec v1.1.0 h1:qPsTqtR1VUPvMPeK0UnCZMtXaKGyyLPG8gj/wG6VqMs=
github.com/container-storage-interface/spec v1.1.0/go.mod h1:6URME8mwIBbpVyZV93Ce5St17xBiQJQY67NDsuohiy4=
github.com/container-storage-interface/spec v1.2.0 h1:bD9KIVgaVKKkQ/UbVUY9kCaH/CJbhNxe0eeB4JeJV2s=
github.com/container-storage-interface/spec v1.2.0/go.mod h1:6URME8mwIBbpVyZV93Ce5St17xBiQJQY67NDsuohiy4=
github.com/davecgh/go-spew v0.0.0-20151105211317-5215b55f46b2/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
Expand Down
2 changes: 1 addition & 1 deletion pkg/common-controller/snapshot_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ func (ctrl *csiSnapshotCommonController) getCreateSnapshotInput(snapshot *crdv1.
contentName := utils.GetSnapshotContentNameForSnapshot(snapshot)

// Resolve snapshotting secret credentials.
snapshotterSecretRef, err := utils.GetSecretReference(class.Parameters, contentName, snapshot)
snapshotterSecretRef, err := utils.GetSecretReference(utils.SnapshotterSecretParams, class.Parameters, contentName, snapshot)
if err != nil {
return nil, nil, "", nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/sidecar-controller/content_create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func TestSyncContent(t *testing.T) {
readyToUse: true,
},
},
expectedListCalls: []listCall{{"sid1-1", true, time.Now(), 1, nil}},
expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil}},
errors: noerrors,
test: testSyncContent,
})
Expand Down
6 changes: 3 additions & 3 deletions pkg/sidecar-controller/csi_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (
type Handler interface {
CreateSnapshot(content *crdv1.VolumeSnapshotContent, parameters map[string]string, snapshotterCredentials map[string]string) (string, string, time.Time, int64, bool, error)
DeleteSnapshot(content *crdv1.VolumeSnapshotContent, snapshotterCredentials map[string]string) error
GetSnapshotStatus(content *crdv1.VolumeSnapshotContent) (bool, time.Time, int64, error)
GetSnapshotStatus(content *crdv1.VolumeSnapshotContent, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error)
}

// csiHandler is a handler that calls CSI to create/delete volume snapshot.
Expand Down Expand Up @@ -103,7 +103,7 @@ func (handler *csiHandler) DeleteSnapshot(content *crdv1.VolumeSnapshotContent,
return nil
}

func (handler *csiHandler) GetSnapshotStatus(content *crdv1.VolumeSnapshotContent) (bool, time.Time, int64, error) {
func (handler *csiHandler) GetSnapshotStatus(content *crdv1.VolumeSnapshotContent, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error) {
ctx, cancel := context.WithTimeout(context.Background(), handler.timeout)
defer cancel()

Expand All @@ -117,7 +117,7 @@ func (handler *csiHandler) GetSnapshotStatus(content *crdv1.VolumeSnapshotConten
return false, time.Time{}, 0, fmt.Errorf("failed to list snapshot for content %s: snapshotHandle is missing", content.Name)
}

csiSnapshotStatus, timestamp, size, err := handler.snapshotter.GetSnapshotStatus(ctx, snapshotHandle)
csiSnapshotStatus, timestamp, size, err := handler.snapshotter.GetSnapshotStatus(ctx, snapshotHandle, snapshotterListCredentials)
if err != nil {
return false, time.Time{}, 0, fmt.Errorf("failed to list snapshot for content %s: %q", content.Name, err)
}
Expand Down
8 changes: 7 additions & 1 deletion pkg/sidecar-controller/framework_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,7 @@ func emptyDataSecretAnnotations() map[string]string {

type listCall struct {
snapshotID string
secrets map[string]string
// information to return
readyToUse bool
createTime time.Time
Expand Down Expand Up @@ -911,7 +912,7 @@ func (f *fakeSnapshotter) DeleteSnapshot(ctx context.Context, snapshotID string,
return call.err
}

func (f *fakeSnapshotter) GetSnapshotStatus(ctx context.Context, snapshotID string) (bool, time.Time, int64, error) {
func (f *fakeSnapshotter) GetSnapshotStatus(ctx context.Context, snapshotID string, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error) {
if f.listCallCounter >= len(f.listCalls) {
f.t.Errorf("Unexpected CSI list Snapshot call: snapshotID=%s, index: %d, calls: %+v", snapshotID, f.createCallCounter, f.createCalls)
return false, time.Time{}, 0, fmt.Errorf("unexpected call")
Expand All @@ -925,6 +926,11 @@ func (f *fakeSnapshotter) GetSnapshotStatus(ctx context.Context, snapshotID stri
err = fmt.Errorf("unexpected List snapshot call")
}

if !reflect.DeepEqual(call.secrets, snapshotterListCredentials) {
f.t.Errorf("Wrong CSI List Snapshot call: snapshotID=%s, expected secrets %+v, got %+v", snapshotID, call.secrets, snapshotterListCredentials)
err = fmt.Errorf("unexpected List Snapshot call")
}

if err != nil {
return false, time.Time{}, 0, fmt.Errorf("unexpected call")
}
Expand Down
25 changes: 24 additions & 1 deletion pkg/sidecar-controller/snapshot_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,33 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c
var readyToUse = false
var driverName string
var snapshotID string
var snapshotterListCredentials map[string]string

if content.Spec.Source.SnapshotHandle != nil {
klog.V(5).Infof("checkandUpdateContentStatusOperation: call GetSnapshotStatus for snapshot which is pre-bound to content [%s]", content.Name)
readyToUse, creationTime, size, err = ctrl.handler.GetSnapshotStatus(content)

if content.Spec.VolumeSnapshotClassName != nil {
class, err := ctrl.getSnapshotClass(*content.Spec.VolumeSnapshotClassName)
if err != nil {
klog.Errorf("Failed to get snapshot class %s for snapshot content %s", *content.Spec.VolumeSnapshotClassName, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

klog.Errorf("Failed to get snapshot class %s for snapshot content %s: %v", *content.Spec.VolumeSnapshotClassName, content.Name, err)

return nil, fmt.Errorf("failed to get snapshot class %s for snapshot content %s", *content.Spec.VolumeSnapshotClassName, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

}

snapshotterListSecretRef, err := utils.GetSecretReference(utils.SnapshotterListSecretParams, class.Parameters, content.GetObjectMeta().GetName(), nil)
if err != nil {
klog.Errorf("Failed to get secret reference for snapshot %s: %s", content.Name, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/for snapshot/for snapshot content

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also can you use "%v" to print err?

return nil, fmt.Errorf("failed to get secret reference for snapshot %s: %s", content.Name, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

}

snapshotterListCredentials, err = utils.GetCredentials(ctrl.client, snapshotterListSecretRef)
if err != nil {
// Continue with deletion, as the secret may have already been deleted.
klog.Errorf("Failed to get credentials for snapshot %s: %s", content.Name, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/for snapshot/for snapshot content

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use %v to print err

return nil, fmt.Errorf("failed to get credentials for snapshot %s: %s", content.Name, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

}
}

readyToUse, creationTime, size, err = ctrl.handler.GetSnapshotStatus(content, snapshotterListCredentials)
if err != nil {
klog.Errorf("checkandUpdateContentStatusOperation: failed to call get snapshot status to check whether snapshot is ready to use %q", err)
return nil, err
Expand Down
28 changes: 18 additions & 10 deletions pkg/sidecar-controller/snapshot_delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ var class5Parameters = map[string]string{
utils.AnnDeletionSecretRefNamespace: "default",
}

var class6Parameters = map[string]string{
utils.PrefixedSnapshotterSecretNameKey: "secret",
utils.PrefixedSnapshotterSecretNamespaceKey: "default",
utils.PrefixedSnapshotterListSecretNameKey: "secret",
utils.PrefixedSnapshotterListSecretNamespaceKey: "default",
}

var snapshotClasses = []*crdv1.VolumeSnapshotClass{
{
TypeMeta: metav1.TypeMeta{
Expand Down Expand Up @@ -126,6 +133,7 @@ var snapshotClasses = []*crdv1.VolumeSnapshotClass{
Annotations: map[string]string{utils.IsDefaultSnapshotClassAnnotation: "true"},
},
Driver: mockDriverName,
Parameters: class6Parameters,
DeletionPolicy: crdv1.VolumeSnapshotContentDelete,
},
}
Expand Down Expand Up @@ -156,7 +164,7 @@ func TestDeleteSync(t *testing.T) {
readyToUse: true,
},
},
expectedListCalls: []listCall{{"sid1-1", true, time.Now(), 1, nil}},
expectedListCalls: []listCall{{"sid1-1", map[string]string{}, true, time.Now(), 1, nil}},
expectedDeleteCalls: []deleteCall{{"sid1-1", nil, nil}},
test: testSyncContent,
},
Expand All @@ -178,7 +186,7 @@ func TestDeleteSync(t *testing.T) {
readyToUse: true,
},
},
expectedListCalls: []listCall{{"sid1-2", true, time.Now(), 1, nil}},
expectedListCalls: []listCall{{"sid1-2", map[string]string{}, true, time.Now(), 1, nil}},
expectedDeleteCalls: []deleteCall{{"sid1-2", nil, nil}},
test: testSyncContent,
},
Expand All @@ -201,7 +209,7 @@ func TestDeleteSync(t *testing.T) {
},
expectedDeleteCalls: []deleteCall{{"sid1-3", nil, fmt.Errorf("mock csi driver delete error")}},
expectedEvents: []string{"Warning SnapshotDeleteError"},
expectedListCalls: []listCall{{"sid1-3", true, time.Now(), 1, nil}},
expectedListCalls: []listCall{{"sid1-3", map[string]string{}, true, time.Now(), 1, nil}},
test: testSyncContent,
},
{
Expand All @@ -216,7 +224,7 @@ func TestDeleteSync(t *testing.T) {
name: "1-5 - csi driver delete snapshot returns error, bound finalizer should remain",
initialContents: newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "snap1-5-volumehandle", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-5", "sid1-5", "snap1-5", "sid1-5", validSecretClass, "", "snap1-5-volumehandle", deletionPolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedListCalls: []listCall{{"sid1-5", true, time.Now(), 1000, nil}},
expectedListCalls: []listCall{{"sid1-5", map[string]string{}, true, time.Now(), 1000, nil}},
expectedDeleteCalls: []deleteCall{{"sid1-5", nil, errors.New("mock csi driver delete error")}},
expectedEvents: []string{"Warning SnapshotDeleteError"},
errors: noerrors,
Expand All @@ -227,7 +235,7 @@ func TestDeleteSync(t *testing.T) {
name: "1-6 - content is deleted before deleting",
initialContents: newContentArray("content1-6", "sid1-6", "snap1-6", "sid1-6", classGold, "sid1-6", "", deletionPolicy, nil, nil, true),
expectedContents: nocontents,
expectedListCalls: []listCall{{"sid1-6", false, time.Now(), 0, nil}},
expectedListCalls: []listCall{{"sid1-6", nil, false, time.Now(), 0, nil}},
expectedDeleteCalls: []deleteCall{{"sid1-6", map[string]string{"foo": "bar"}, nil}},
expectedEvents: noevents,
errors: noerrors,
Expand All @@ -243,7 +251,7 @@ func TestDeleteSync(t *testing.T) {
initialContents: newContentArrayWithReadyToUse("content1-7", "", "snap1-7", "sid1-7", validSecretClass, "sid1-7", "", deletePolicy, nil, &defaultSize, &True, true),
expectedContents: newContentArrayWithReadyToUse("content1-7", "", "snap1-7", "sid1-7", validSecretClass, "sid1-7", "", deletePolicy, nil, &defaultSize, &True, true),
expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-7", true, time.Now(), 1000, nil}},
expectedListCalls: []listCall{{"sid1-7", map[string]string{}, true, time.Now(), 1000, nil}},
initialSecrets: []*v1.Secret{secret()},
errors: noerrors,
test: testSyncContent,
Expand All @@ -253,7 +261,7 @@ func TestDeleteSync(t *testing.T) {
initialContents: newContentArrayWithReadyToUse("content1-8", "sid1-8", "none-existed-snapshot", "sid1-8", validSecretClass, "sid1-8", "", retainPolicy, nil, &defaultSize, &True, true),
expectedContents: newContentArrayWithReadyToUse("content1-8", "sid1-8", "none-existed-snapshot", "sid1-8", validSecretClass, "sid1-8", "", retainPolicy, nil, &defaultSize, &True, true),
expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-8", true, time.Now(), 0, nil}},
expectedListCalls: []listCall{{"sid1-8", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors,
test: testSyncContent,
},
Expand All @@ -262,7 +270,7 @@ func TestDeleteSync(t *testing.T) {
initialContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "sid1-9", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-9", "sid1-9", "snap1-9", "", emptySecretClass, "", "snap1-9-volumehandle", deletePolicy, nil, &defaultSize, false, &timeNowMetav1),
expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-9", true, time.Now(), 0, nil}},
expectedListCalls: []listCall{{"sid1-9", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors,
initialSecrets: []*v1.Secret{}, // secret does not exist
expectedDeleteCalls: []deleteCall{{"sid1-9", nil, nil}},
Expand All @@ -273,7 +281,7 @@ func TestDeleteSync(t *testing.T) {
initialContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-10", "sid1-10", "snap1-10", "sid1-10", emptySecretClass, "", "snap1-10-volumehandle", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-10", true, time.Now(), 0, nil}},
expectedListCalls: []listCall{{"sid1-10", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors,
initialSecrets: []*v1.Secret{},
test: testSyncContent,
Expand All @@ -292,7 +300,7 @@ func TestDeleteSync(t *testing.T) {
initialContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, true, &timeNowMetav1),
expectedContents: newContentArrayWithDeletionTimestamp("content1-12", "sid1-12", "snap1-12", "sid1-12", emptySecretClass, "sid1-12", "", retainPolicy, nil, &defaultSize, false, &timeNowMetav1),
expectedEvents: noevents,
expectedListCalls: []listCall{{"sid1-12", true, time.Now(), 0, nil}},
expectedListCalls: []listCall{{"sid1-12", map[string]string{}, true, time.Now(), 0, nil}},
errors: noerrors,
initialSecrets: []*v1.Secret{},
test: testSyncContent,
Expand Down
5 changes: 3 additions & 2 deletions pkg/snapshotter/snapshotter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ type Snapshotter interface {
DeleteSnapshot(ctx context.Context, snapshotID string, snapshotterCredentials map[string]string) (err error)

// GetSnapshotStatus returns if a snapshot is ready to use, creation time, and restore size.
GetSnapshotStatus(ctx context.Context, snapshotID string) (bool, time.Time, int64, error)
GetSnapshotStatus(ctx context.Context, snapshotID string, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error)
}

type snapshot struct {
Expand Down Expand Up @@ -112,7 +112,7 @@ func (s *snapshot) isListSnapshotsSupported(ctx context.Context) (bool, error) {
return false, nil
}

func (s *snapshot) GetSnapshotStatus(ctx context.Context, snapshotID string) (bool, time.Time, int64, error) {
func (s *snapshot) GetSnapshotStatus(ctx context.Context, snapshotID string, snapshotterListCredentials map[string]string) (bool, time.Time, int64, error) {
klog.V(5).Infof("GetSnapshotStatus: %s", snapshotID)

client := csi.NewControllerClient(s.conn)
Expand All @@ -127,6 +127,7 @@ func (s *snapshot) GetSnapshotStatus(ctx context.Context, snapshotID string) (bo
}
req := csi.ListSnapshotsRequest{
SnapshotId: snapshotID,
Secrets: snapshotterListCredentials,
}
rsp, err := client.ListSnapshots(ctx, &req)
if err != nil {
Expand Down
41 changes: 30 additions & 11 deletions pkg/snapshotter/snapshotter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,17 +362,24 @@ func TestGetSnapshotStatus(t *testing.T) {
},
}

secret := map[string]string{"foo": "bar"}
secretRequest := &csi.ListSnapshotsRequest{
SnapshotId: defaultID,
Secrets: secret,
}

tests := []struct {
name string
snapshotID string
listSnapshotsSupported bool
input *csi.ListSnapshotsRequest
output *csi.ListSnapshotsResponse
injectError codes.Code
expectError bool
expectReady bool
expectCreateAt time.Time
expectSize int64
name string
snapshotID string
snapshotterListCredentials map[string]string
listSnapshotsSupported bool
input *csi.ListSnapshotsRequest
output *csi.ListSnapshotsResponse
injectError codes.Code
expectError bool
expectReady bool
expectCreateAt time.Time
expectSize int64
}{
{
name: "success",
Expand All @@ -385,6 +392,18 @@ func TestGetSnapshotStatus(t *testing.T) {
expectCreateAt: createTime,
expectSize: size,
},
{
name: "secret",
snapshotID: defaultID,
snapshotterListCredentials: secret,
listSnapshotsSupported: true,
input: secretRequest,
output: defaultResponse,
expectError: false,
expectReady: true,
expectCreateAt: createTime,
expectSize: size,
},
{
name: "ListSnapshots not supported",
snapshotID: defaultID,
Expand Down Expand Up @@ -455,7 +474,7 @@ func TestGetSnapshotStatus(t *testing.T) {
}

s := NewSnapshotter(csiConn)
ready, createTime, size, err := s.GetSnapshotStatus(context.Background(), test.snapshotID)
ready, createTime, size, err := s.GetSnapshotStatus(context.Background(), test.snapshotID, test.snapshotterListCredentials)
if test.expectError && err == nil {
t.Errorf("test %q: Expected error, got none", test.name)
}
Expand Down
Loading