issue-384, A single "default" storage class secret is added #393

taaraora · 2020-01-03T16:29:30Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #384

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Default StorageClass secrets are added:
 csi.storage.k8s.io/secret-name: ${pvc.name}
 csi.storage.k8s.io/secret-namespace: ${pvc.namespace}

Note: Default secrets for storage class feature does work only when both parameters are added.

k8s-ci-robot · 2020-01-03T16:29:39Z

Welcome @taaraora!

It looks like this is your first PR to kubernetes-csi/external-provisioner 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-provisioner has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-01-03T16:29:39Z

Hi @taaraora. 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.

msau42 · 2020-01-09T02:57:43Z

/ok-to-test

msau42

Can you add some unit tests that use the new default secret?

Another thing to keep in mind, allowable secret templates are different for provisioning compared to the other operations. We should make sure we can maintain that behavior for the default secret.

msau42 · 2020-01-09T03:02:31Z

/assign @misterikkit
to help review

misterikkit

Thanks for the enhancement! Looks like it will save us some API bloat in the future.

Functional change LGTM. Please add some test coverage.

misterikkit · 2020-01-10T00:36:14Z

pkg/controller/controller_test.go

+				prefixedDefaultSecretNameKey:                "csiBar",
+				prefixedDefaultSecretNamespaceKey:           "csiBar",


Does this change cover the new logic, or just fix an existing test? I think we should add a case that gets the default value.

This was just fix of existing test. I've added a few new tests for default secret.

taaraora · 2020-01-10T16:08:49Z

Can you add some unit tests that use the new default secret?

Another thing to keep in mind, allowable secret templates are different for provisioning compared to the other operations. We should make sure we can maintain that behavior for the default secret.

@msau42

I've added tests for default secrets.
Yep, default secrets support only those template variables which are common for all call templates. We definitely should describe this behavior in the documentation.

misterikkit

Thanks for adding tests. Do you mind squashing the commits?

misterikkit · 2020-01-14T19:34:30Z

pkg/controller/controller_test.go

-// Minimal PVC required for tests to function
-func createFakePVC(requestBytes int64) *v1.PersistentVolumeClaim {
+// createFakeNamedPVC creates PVC for tests, should be extended with new args when needed
+func createFakeNamedPVC(requestBytes int64, name string, userAnnotations ...struct{ Key, Value string }) *v1.PersistentVolumeClaim {


Any reason userAnnotations is a slice of struct rather than a map[string]string? The call site that uses this new arg is overly verbose.

Yes, the current signature makes userAnnotations argument optional. If we change it to map it will require to add an empty map to each place where this function is invoked now.

Seems like not too many call sites? Alternatively, since you only use userAnnotations in one place, you could make it a different helper function with a longer, more specific name.

The most comprehensive solution would be to adopt the standard "function options" pattern, but that may be overkill.

Hm, I agree with you, a different helper function is the better way to handle this.

Fixed, it is map[string]string now.

misterikkit · 2020-01-14T19:36:14Z

pkg/controller/controller_test.go

@@ -477,13 +477,18 @@ func provisionFromPVCCapabilities() (rpc.PluginCapabilitySet, rpc.ControllerCapa
 		}
 }

-// Minimal PVC required for tests to function
-func createFakePVC(requestBytes int64) *v1.PersistentVolumeClaim {
+// createFakeNamedPVC creates PVC for tests, should be extended with new args when needed


should be extended with new args when needed

I don't understand what you mean, since this could apply to any helper function. Could you elaborate?

Thank you, that makes sense, I can just delete it.

misterikkit · 2020-01-14T22:53:51Z

pkg/controller/controller_test.go

@@ -1229,6 +1382,31 @@ func TestProvision(t *testing.T) {
 			expectErr:         true,
 			expectState:       controller.ProvisioningNoChange,
 		},
+		"fail to get secret reference for default secret parameter template with annotation": {


Is the failure caused by mismatch between secret-from-annotation and default-secret-from-annotation? If so, could you make the names more distinct? Thanks.

As @msau42 pointed out the default secrets can support only that set of template variables which is common for all calls. In this test, I showed that the annotation variable is not supported by the default secret. Should I rename this test?

I think after that explanation, the test name makes sense. I just find the existing logic with secret references and template evaluation to be confusing. 😢

Now that I re-read the test, maybe we can update the name to mention invalid/illegal template variable.

taaraora · 2020-01-15T16:37:02Z

/retest

misterikkit

/lgtm

nice job refactoring the tests.

msau42 · 2020-01-28T02:23:41Z

/approve
Thank you!

k8s-ci-robot · 2020-01-28T02:23:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, taaraora

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~~ [msau42]

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

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 3, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 3, 2020

k8s-ci-robot requested review from lpabon and sbezverk January 3, 2020 16:29

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 3, 2020

taaraora force-pushed the issue-384 branch 2 times, most recently from 1a38101 to cb84e09 Compare January 3, 2020 17:46

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 Jan 9, 2020

msau42 reviewed Jan 9, 2020

View reviewed changes

k8s-ci-robot assigned misterikkit Jan 9, 2020

misterikkit reviewed Jan 10, 2020

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 10, 2020

taaraora requested review from misterikkit and msau42 January 14, 2020 17:32

misterikkit reviewed Jan 14, 2020

View reviewed changes

taaraora force-pushed the issue-384 branch from 2cc884a to cf62f2c Compare January 14, 2020 23:25

taaraora requested a review from misterikkit January 14, 2020 23:26

issue-384, A single "default" storage class secret is added

8d4cd7a

taaraora force-pushed the issue-384 branch from cf62f2c to 8d4cd7a Compare January 15, 2020 16:13

misterikkit reviewed Jan 24, 2020

View reviewed changes

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

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

k8s-ci-robot merged commit 8444597 into kubernetes-csi:master Jan 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue-384, A single "default" storage class secret is added #393

issue-384, A single "default" storage class secret is added #393

taaraora commented Jan 3, 2020 •

edited

Loading

k8s-ci-robot commented Jan 3, 2020

k8s-ci-robot commented Jan 3, 2020

msau42 commented Jan 9, 2020

msau42 left a comment

msau42 commented Jan 9, 2020

misterikkit left a comment

misterikkit Jan 10, 2020

taaraora Jan 10, 2020

taaraora commented Jan 10, 2020 •

edited

Loading

misterikkit left a comment

misterikkit Jan 14, 2020

taaraora Jan 14, 2020

misterikkit Jan 14, 2020

taaraora Jan 15, 2020

taaraora Jan 15, 2020

misterikkit Jan 14, 2020

taaraora Jan 14, 2020

misterikkit Jan 14, 2020

taaraora Jan 14, 2020

misterikkit Jan 14, 2020

misterikkit Jan 14, 2020

taaraora Jan 15, 2020

taaraora commented Jan 15, 2020

misterikkit left a comment

msau42 commented Jan 28, 2020

k8s-ci-robot commented Jan 28, 2020

		prefixedDefaultSecretNameKey: "csiBar",
		prefixedDefaultSecretNamespaceKey: "csiBar",

issue-384, A single "default" storage class secret is added #393

issue-384, A single "default" storage class secret is added #393

Conversation

taaraora commented Jan 3, 2020 • edited Loading

k8s-ci-robot commented Jan 3, 2020

k8s-ci-robot commented Jan 3, 2020

msau42 commented Jan 9, 2020

msau42 left a comment

Choose a reason for hiding this comment

msau42 commented Jan 9, 2020

misterikkit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taaraora commented Jan 10, 2020 • edited Loading

misterikkit left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taaraora commented Jan 15, 2020

misterikkit left a comment

Choose a reason for hiding this comment

msau42 commented Jan 28, 2020

k8s-ci-robot commented Jan 28, 2020

taaraora commented Jan 3, 2020 •

edited

Loading

taaraora commented Jan 10, 2020 •

edited

Loading