feat(ListSnapshotsSecret): csi-sanity test suite now passes secrets to ListSnapshots

[jskazinski]

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds ListSnapshotsSecret handling to the csi-sanity test suite. This feature is needed by CSI Drivers that require secrets in order to execute the Controller ListSnapshots function. All csi-sanity calls to ListSnapshot now pass in ListSnapshotsSecret.

Which issue(s) this PR fixes:
Fixes #356

Special notes for your reviewer:

• Small change to correct link in CONTRIBUTING.md
• CSICreds now includes ListSnapshotsSecret
• Adjusted ListSnapshotRequest objects to include sc.Secrets.ListSnapshotsSecret
• Also passing sc.Secrets.DeleteSnapshotSecret to the 'deleting the snapshot' call (separate issue)

Does this PR introduce a user-facing change?:

Users will need to add ListSnapshotsSecret to a YAML file to pass secrets to the CSI Driver in from the csi-sanity -csi.secrets ${secrets} ... call.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Details

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. I understand the commands that are listed here.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Joe Skazinski (d7989f4)

[k8s-ci-robot]

Welcome @jskazinski!

It looks like this is your first PR to kubernetes-csi/csi-test 🎉. 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/csi-test 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]

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

Details

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.

[pohly]

/ok-to-test

[bswartz]

I think it's only okay to pass the secret value when also passing in a snapshot ID. The secret would be interpreted as the secret that was used for creating that snapshot originally. In situations where no snapshot ID is passed in, I don't know how a CO is supposed to know which secret to supply, assuming that each snapshot might have a different secret and the list operation is working with more than one.

[jskazinski]

@bswartz - My take was that the driver handling the request needs the secret data to perform the request, even if the test does not pass in a snapshot ID. The request handler may require secret data (credentials) in order to process any request. I see your point about the possibility of a unique secret per snapshot, but for these test cases I just followed the prior examples, which may have a similar issue. For testing purposes, I only tested using one secret. Is there an example I can follow to make such a change?
Are you thinking the change needs to pass in a table of snapshot id/secret values relations? So the CO can pass in multiple secret structs per snapshot ID?

[bswartz]

@bswartz - My take was that the driver handling the request needs the secret data to perform the request, even if the test does not pass in a snapshot ID. The request handler may require secret data (credentials) in order to process any request. I see your point about the possibility of a unique secret per snapshot, but for these test cases I just followed the prior examples, which may have a similar issue. For testing purposes, I only tested using one secret. Is there an example I can follow to make such a change? Are you thinking the change needs to pass in a table of snapshot id/secret values relations? So the CO can pass in multiple secret structs per snapshot ID?

I'm saying that the desired behavior is underspecified in the CSI spec. If you look at the existing Kubernetes behavior as a de-facto specification of how a CO should behave, then ListSnapshots never gets called without a snapshot ID. For the cases where it does get called (with a snapshot ID) the supplied secret is the one for that specific snapshot. I find it hard to imagine a clean way to extend the current behavior to the no-snapshot-ID list case in Kubernetes, because if there are two or more snapshots with distinct secrets, it's unclear which secret to provide.

Most likely the spec itself should be clarified about what's supposed to happen. Just because Kubernetes implements things a certain way doesn't mean it's right. However I think that nobody is motivated to solve the problem here because nearly all the people who work on the spec are only interested in addressing Kubernetes use cases, and this isn't something Kubernetes cares about.

[jskazinski]

@bswartz - My take was that the driver handling the request needs the secret data to perform the request, even if the test does not pass in a snapshot ID. The request handler may require secret data (credentials) in order to process any request. I see your point about the possibility of a unique secret per snapshot, but for these test cases I just followed the prior examples, which may have a similar issue. For testing purposes, I only tested using one secret. Is there an example I can follow to make such a change? Are you thinking the change needs to pass in a table of snapshot id/secret values relations? So the CO can pass in multiple secret structs per snapshot ID?

I'm saying that the desired behavior is underspecified in the CSI spec. If you look at the existing Kubernetes behavior as a de-facto specification of how a CO should behave, then ListSnapshots never gets called without a snapshot ID. For the cases where it does get called (with a snapshot ID) the supplied secret is the one for that specific snapshot. I find it hard to imagine a clean way to extend the current behavior to the no-snapshot-ID list case in Kubernetes, because if there are two or more snapshots with distinct secrets, it's unclear which secret to provide.

Most likely the spec itself should be clarified about what's supposed to happen. Just because Kubernetes implements things a certain way doesn't mean it's right. However I think that nobody is motivated to solve the problem here because nearly all the people who work on the spec are only interested in addressing Kubernetes use cases, and this isn't something Kubernetes cares about.

Thank you for the clarification and additional information. Is there anything you would like me to do in regards to this specific pull request? If so, I will work on those updates.

[jskazinski]

@pohly I am unfamiliar with the process, is there anything I need to do to request reviews or perform next steps?

[pohly]

There's no specific process beyond getting reviewers to agree that merging the PR is useful.

As far as I am concerned, it is useful (otherwise you wouldn't have bothered with implementing this) and it fits into how other secrets are handled, so I have no conceptual issue with it. I would merge.

But as @bswartz had comments, I'd like to get is okay, too.

/assign @bswartz

Can you lgtm if you agree with merging it?

[bswartz]

Given that the correct behavior is unspecified, I think a good way to enable testing how drivers should work while not giving implementors any false impressions would be to allow two secrets: one for an actual list call (without the snapshot ID) and another for a get-style-list (with a snapshot ID). The one for the get-style-list call is absolutely essential because Kubernetes does rely on that working correctly and drivers should implement it and be able to test it. The one for the list-style-list maybe could be left as nil under normal circumstances, but changed to a real value if the spec every gets more clear or if the Kubernetes ever starts relying on the list-style-list working.

[jskazinski]

My current understanding is that the same secret is used by the storage system for both the get a list of snapshots (with no snapshot_id) and the get a single snapshot (with a snapshot_id). The current csi-tests do test both of these use cases. If the test case wants to use a nil secret when calling ListSnapshots, isn't that done by NOT passing in any secret value, so that is already possible if one wants to test with a nil secret. In regards to passing multiple secrets, perhaps that is possible based on passing in multiple key/value pairs via the secret map - if not, then the ListSnapshotsRequest type would have to be updated. For this pull request, I apologize for my lack of experience with the csi-tests, but I am not sure what changes you are suggesting.

[bswartz]

My current understanding is that the same secret is used by the storage system for both the get a list of snapshots (with no snapshot_id) and the get a single snapshot (with a snapshot_id).

This depends on the CSI driver and storage system implementation. The CSI spec merely provides a way to pass down secrets, and in every other context, the secret can be (but doesn't have to be) specific to the snapshot in question. List calls with no specific snapshot are the one place where it's not possible to use a snapshot-specific secret.

Kubernetes currently allows CSI driver developers to require that admins configure snapshot-specific secrets, because Kubernetes never encounters the situation where is has to call a CSI RPC without a specific snapshot, so the limitation above doesn't matter in practice.

My concern with this change is that, by using the same secrets in all cases, we could be leading CSI driver authors to assume that this is what clients will normally do, but because the CSI spec is vague, it could change in the future, especially if Kubernetes ever develops a need to list more than one snapshot.

After reflecting one what approach could be better, though, I really don't have any ideas. For drivers where the secrets should be the same for any snapshot, then using that secret for listing also makes sense. And if there's a driver that needs snapshot-specific secrets for some things, it can't expect such a secret for listing, so it either won't implement listing or will use some other more generic secret. For the purposes of testing, they can probably figure it out, and if not, we need to change the CSI spec anyways and we can fix things at that time. So I'll drop my objection.

[bswartz]

/lgtm

[jskazinski]

Good points, and thank you for the additional information. On first thought, I can see a linkage between storage system and snapshots, where a secret is needed for every unique storage system. This is the case based on my experience. I see snapshots, or even list of snapshots, all tied to a unique storage system, and each storage system could/should require a unique secrets. I could also see this decoupled from actual requests, and maintained separately as a storage system/secret mapping. But base don the current design of the csi spec, perhaps adding the concept of a unique storage system id, and binding that to secrets, would add what is needed by csi drivers. Of course, I can't speak for all storage systems.
Thanks for the approval!

[pohly]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bswartz, jskazinski, pohly

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [pohly]

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

[pohly]

/lgtm