Added the feature to use secrets of ListSnapshots #252

bells17 · 2020-02-10T03:04:27Z

I modified to be able to use secrets in ListSnapshotRequest.

What type of PR is this?
/kind feature

What this PR does / why we need it:
I want to use secrets in ListSnapshots.

Which issue(s) this PR fixes:
Fixes #236
This is a new PR. old PR is #237.
(Because I misunderstood how to support VolumeSnapshotSecret in ListVolumes, I recreated a new PR...)

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Adds support for ListSnapshots secrets

k8s-ci-robot · 2020-02-10T03:04:35Z

Hi @bells17. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bells17 · 2020-02-10T03:21:27Z

@xing-yang I created a new PR. Could you review this PR?

xing-yang · 2020-02-10T14:13:17Z

/ok-to-test

xing-yang · 2020-02-11T19:23:51Z

You need to add "release-note" right after ``` above the release note text like this:
```release-note
This is the release note
```

bells17 · 2020-02-12T04:10:36Z

Thank you, I added "release-note".

xing-yang · 2020-02-13T21:57:31Z

pkg/sidecar-controller/snapshot_controller.go

+		if content.Spec.VolumeSnapshotClassName != nil {
+			class, err := ctrl.getSnapshotClass(*content.Spec.VolumeSnapshotClassName)
+			if err != nil {
+				klog.Errorf("failed to getSnapshotClass %s", err)


s/failed/Failed
Can you also print out the snapshot class name in the error message.

xing-yang · 2020-02-13T21:59:32Z

pkg/sidecar-controller/snapshot_controller.go

+			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 credentials for snapshot %s: %s", content.Name, err.Error())


Missing "get":
Failed to get credentials for snapshot ...

xing-yang · 2020-02-13T22:01:16Z

pkg/sidecar-controller/snapshot_controller.go

+			if err != nil {
+				// Continue with deletion, as the secret may have already been deleted.
+				klog.Errorf("Failed to credentials for snapshot %s: %s", content.Name, err.Error())
+				return nil, fmt.Errorf("cannot get credentials for snapshot content %#v", content.Name)


It's good to have the "err" here as well. You can format a message and have it used in both logging and the return message.

@xing-yang Sorry,

You can format a message and have it used in both logging and the return message.

This means should I create a variable that store formatted error message, and hand over the variable to klog.Errorf argument and return value?
And should I apply the same to other parts as well?

Yes. So that means the message needs to start with lower case.

Um...Sorry I can't understand yet.
In this part, should I fix as below?

// Continue with deletion, as the secret may have already been deleted. klog.Errorf("Failed to credentials for snapshot %s: %s", content.Name, err.Error()) return nil, fmt.Errorf("failed to credentials for snapshot %s: %s", content.Name, err.Error())

Yes, that is fine too.

s/to credentials/to get credentials

Thank you. I understand.

xing-yang · 2020-02-14T02:11:11Z

pkg/sidecar-controller/snapshot_controller.go

+			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.Error())
+				return nil, fmt.Errorf("cannot get secret reference for snapshot content %#v", content.Name)


print err here as well.

xing-yang · 2020-02-14T02:12:04Z

pkg/sidecar-controller/snapshot_controller.go

+			class, err := ctrl.getSnapshotClass(*content.Spec.VolumeSnapshotClassName)
+			if err != nil {
+				klog.Errorf("failed to getSnapshotClass %s", err)
+				return nil, fmt.Errorf("cannot get snapshot class for snapshot content %s", err)


add snapshot class name here too.

xing-yang · 2020-02-15T16:22:32Z

pkg/sidecar-controller/framework_test.go

@@ -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 Delete Snapshot call")


/Delete/List

xing-yang · 2020-02-15T19:10:47Z

I tested this PR to create a snapshot dynamically with secrets for delete and list snapshots. It failed with the following error:

Failed to create snapshot: failed to take snapshot of the volume, 4cbb0970-5024-11ea-9bdb-0242ac110003: "failed to remove CSI Parameters of prefixed keys: found unknown parameter key \"csi.storage.k8s.io/snapshotter-list-secret-name\" with reserved namespace csi.storage.k8s.io/"

In function RemovePrefixedParameters() in util.go, those new prefixed keys for list snapshots secret need to be added. Please add a unit test for create snapshot with unknown keys.

bells17 · 2020-02-19T15:51:59Z

@xing-yang
#252 (comment)

In function RemovePrefixedParameters() in util.go, those new prefixed keys for list snapshots secret need to be added.

Sorry, it went over my head. I'll fix it.

Please add a unit test for create snapshot with unknown keys.

a unit test of checking unknown keys is already exists as below:

external-snapshotter/pkg/utils/util_test.go

Lines 125 to 129 in 3b76de4

    
           { 
        
           	name:      "unknown prefixed var", 
        
           	params:    map[string]string{csiParameterPrefix + "bim": "baz"}, 
        
           	expectErr: true, 
        
           },

So I think it's maybe better that append parameters that having "csi.storage.k8s.io/snapshotter-list-secret-namespace" and "csi.storage.k8s.io/snapshotter-list-secret-name" to the "default-class" for unit tests is better.

external-snapshotter/pkg/sidecar-controller/snapshot_delete_test.go

Lines 120 to 130 in 3b76de4

    
           { 
        
           	TypeMeta: metav1.TypeMeta{ 
        
           		Kind: "VolumeSnapshotClass", 
        
           	}, 
        
           	ObjectMeta: metav1.ObjectMeta{ 
        
           		Name:        defaultClass, 
        
           		Annotations: map[string]string{utils.IsDefaultSnapshotClassAnnotation: "true"}, 
        
           	}, 
        
           	Driver:         mockDriverName, 
        
           	DeletionPolicy: crdv1.VolumeSnapshotContentDelete, 
        
           },

What do you think about this idea?

xing-yang · 2020-02-19T15:53:38Z

Yes, your suggestion on unit test sounds good to me.

bells17 · 2020-02-19T16:24:42Z

@xing-yang I fixed RemovePrefixedParameters() and improved log and error messages.
(Also I appended parameters to "default-class" for unit tests)

But I don't test below cases manually yet:

create a snapshot dynamically with secrets for delete and list snapshots.
import an existing snapshot with secrets for list.

I'll test above cases tomorrow.
(Because currently Japan now midnight 😅 I'm living in Japan.)

xing-yang · 2020-02-19T16:28:04Z

@bells17 no problem. Thanks for the heads up!

bells17 · 2020-02-20T15:05:49Z

@xing-yang Hi, I tested below cases:

create a snapshot dynamically with secrets for delete and list snapshots.
import an existing snapshot with secrets for list.

I wonder if you could review this PR again.

xing-yang · 2020-02-20T15:08:53Z

Thanks @bells17 ! I'll take a look.

xing-yang · 2020-02-20T15:10:55Z

pkg/sidecar-controller/snapshot_controller.go

+
+			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)


s/for snapshot/for snapshot content

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

xing-yang · 2020-02-20T15:11:03Z

pkg/sidecar-controller/snapshot_controller.go

+			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)
+				return nil, fmt.Errorf("failed to get secret reference for snapshot %s: %s", content.Name, err)


Same as above

xing-yang · 2020-02-20T15:12:12Z

pkg/sidecar-controller/snapshot_controller.go

+		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)


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

xing-yang · 2020-02-20T15:12:21Z

pkg/sidecar-controller/snapshot_controller.go

+			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)
+				return nil, fmt.Errorf("failed to get snapshot class %s for snapshot content %s", *content.Spec.VolumeSnapshotClassName, err)


Same as above

xing-yang · 2020-02-20T15:13:26Z

pkg/sidecar-controller/snapshot_controller.go

+			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)


s/for snapshot/for snapshot content

use %v to print err

xing-yang · 2020-02-20T15:13:40Z

pkg/sidecar-controller/snapshot_controller.go

+			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)
+				return nil, fmt.Errorf("failed to get credentials for snapshot %s: %s", content.Name, err)


same as above

xing-yang · 2020-02-20T15:16:46Z

pkg/utils/util.go

+	PrefixedSnapshotterSecretNameKey      = csiParameterPrefix + "snapshotter-secret-name"
+	PrefixedSnapshotterSecretNamespaceKey = csiParameterPrefix + "snapshotter-secret-namespace"
+
+	PrefixedSnapshotterListSecretNameKey      = csiParameterPrefix + "snapshotter-list-secret-name"


Add a comment: // Prefixed name key for ListSnapshots secret

xing-yang · 2020-02-20T15:17:01Z

pkg/utils/util.go

+	PrefixedSnapshotterSecretNamespaceKey = csiParameterPrefix + "snapshotter-secret-namespace"
+
+	PrefixedSnapshotterListSecretNameKey      = csiParameterPrefix + "snapshotter-list-secret-name"
+	PrefixedSnapshotterListSecretNamespaceKey = csiParameterPrefix + "snapshotter-list-secret-namespace"


Add a comment: // Prefixed namespace key for ListSnapshots secret

xing-yang · 2020-02-20T15:17:23Z

pkg/utils/util.go

@@ -53,8 +53,11 @@ const (
 	// fields in subsequent CSI calls or Kubernetes API objects.
 	csiParameterPrefix = "csi.storage.k8s.io/"

-	prefixedSnapshotterSecretNameKey      = csiParameterPrefix + "snapshotter-secret-name"
-	prefixedSnapshotterSecretNamespaceKey = csiParameterPrefix + "snapshotter-secret-namespace"
+	PrefixedSnapshotterSecretNameKey      = csiParameterPrefix + "snapshotter-secret-name"


Add a comment: // Prefixed name key for DeleteSnapshot secret

xing-yang · 2020-02-20T15:17:34Z

pkg/utils/util.go

-	prefixedSnapshotterSecretNameKey      = csiParameterPrefix + "snapshotter-secret-name"
-	prefixedSnapshotterSecretNamespaceKey = csiParameterPrefix + "snapshotter-secret-namespace"
+	PrefixedSnapshotterSecretNameKey      = csiParameterPrefix + "snapshotter-secret-name"
+	PrefixedSnapshotterSecretNamespaceKey = csiParameterPrefix + "snapshotter-secret-namespace"


Add a comment: // Prefixed namespace key for DeleteSnapshot secret

bells17 · 2020-02-20T16:02:03Z

@xing-yang Thank you for your review quickly and I'm sorry about that I have your commented log messages and error messages many times to improve these messages 😢
I reflected your comments.

xing-yang · 2020-02-21T02:27:33Z

Thanks for your work!
/lgtm
/approve

k8s-ci-robot · 2020-02-21T02:27:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bells17, xing-yang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xing-yang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adb3af9 Merge pull request kubernetes-csi#252 from bells17/update-go-version b82ee38 Merge pull request kubernetes-csi#253 from bells17/fix-typo c317456 Fix typo 0a78505 Bump to Go 1.22.3 edd89ad Merge pull request kubernetes-csi#251 from jsafrane/add-logcheck 043fd09 Add test-logcheck target d7535ae Merge pull request kubernetes-csi#250 from jsafrane/go-1.22 b52e7ad Update go to 1.22.2 14fdb6f Merge pull request kubernetes-csi#247 from msau42/prow 9b4352e Update release playbook c7bb972 Fix release notes script to use fixed tags 463a0e9 Add script to update specific go modules git-subtree-dir: release-tools git-subtree-split: adb3af9

adb3af9 Merge pull request kubernetes-csi#252 from bells17/update-go-version b82ee38 Merge pull request kubernetes-csi#253 from bells17/fix-typo c317456 Fix typo 0a78505 Bump to Go 1.22.3 git-subtree-dir: release-tools git-subtree-split: adb3af9

Update go to 1.22.3

Added the feature to use secrets of ListSnapshots

3b76de4

k8s-ci-robot requested review from saad-ali and xing-yang February 10, 2020 03:04

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 10, 2020

bells17 mentioned this pull request Feb 10, 2020

Add feature listsnapshots secrets #237

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 10, 2020

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 12, 2020

xing-yang reviewed Feb 14, 2020

View reviewed changes

xing-yang reviewed Feb 15, 2020

View reviewed changes

bells17 added 2 commits February 19, 2020 20:19

Improve error messages

4017329

Fix error message s/Delete/List/

2045ef1

bells17 added 2 commits February 20, 2020 01:00

Add support for list secret parameters to RemovePrefixedParameters

0a1579d

Improve error messages

dd390a3

Modify to upper case variable name from lower case for unit tests

8e9680a

xing-yang reviewed Feb 20, 2020

View reviewed changes

bells17 added 2 commits February 21, 2020 00:42

Fix log messages and error messages

7cbcf6a

Add comments

3c2baf4

xing-yang mentioned this pull request Feb 20, 2020

Add secret for ListSnapshot call #162

Closed

k8s-ci-robot assigned xing-yang Feb 21, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2020

k8s-ci-robot merged commit 883842f into kubernetes-csi:master Feb 21, 2020

xing-yang mentioned this pull request Mar 9, 2020

Add a secret key for ListSnapshot to SnapshotClass #152

Closed

bells17 deleted the add-feature-listsnapshots-secrets2 branch July 16, 2020 23:25

xing-yang mentioned this pull request Oct 28, 2020

add e2e tests for create/delete volume snapshot with secrets kubernetes/kubernetes#95848

Merged

andyzhangx pushed a commit to andyzhangx/external-snapshotter that referenced this pull request Jun 5, 2024

Merge pull request kubernetes-csi#252 from bells17/update-go-version

adb3af9

Update go to 1.22.3

Added the feature to use secrets of ListSnapshots #252

Added the feature to use secrets of ListSnapshots #252

Conversation

bells17 commented Feb 10, 2020 • edited Loading

k8s-ci-robot commented Feb 10, 2020

bells17 commented Feb 10, 2020

xing-yang commented Feb 10, 2020

xing-yang commented Feb 11, 2020

bells17 commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented Feb 15, 2020

bells17 commented Feb 19, 2020

xing-yang commented Feb 19, 2020

bells17 commented Feb 19, 2020

xing-yang commented Feb 19, 2020

bells17 commented Feb 20, 2020 • edited Loading

xing-yang commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bells17 commented Feb 20, 2020 • edited Loading

xing-yang commented Feb 21, 2020

k8s-ci-robot commented Feb 21, 2020

bells17 commented Feb 10, 2020 •

edited

Loading

bells17 commented Feb 20, 2020 •

edited

Loading

bells17 commented Feb 20, 2020 •

edited

Loading