Credentials difficult for COs to expose

[saad-ali]

PR #168 attempted to fix issues with credentials in the CSI spec by introducing a unique credential per call: controller_create_credentials, controller_delete_credentials, controller_publish_credentials, controller_unpublish_credentials, node_stage_credentials, node_unstage_credentials, node_publish_credentials, and node_unpublish_credentials. Furthermore it requires that COs SHALL permit passing through the required credentials.

A few issues:

1. This implies the credentials for each of these calls could be different (even if the calls are for the same volume), and that COs must allow users to set them. Update: on rethinking this should be possible to allow plugins to control where secrets are exposed.
2. This implies the credentials for request and response for the same call could be different, and that COs must allow users to set them.
3. Exposing this many credentials to end users is confusing (it requires them to understand the inner workings of CO to SP communications), and difficult for COs to do (where in the Kubernetes API would all of these make sense).
4. nit: The word credentials implies identity, but it could be used to pass, for example, a decryption key (which is not really a credential).

Proposal:

• Simplify by specifying a secret per use case instead of per operation (reducing 8 possibilities to 3):
  1. create_delete_volume_secrets
     • Grants ability to create and delete volume requests.
     • Passed to CreateVolume() and DeleteVolume().
  2. volume_secrets
     • Secrets required to access a volume, independent of any consumer.
     • Example: an encryption key for volume.
     • Passed to ControllerPublishVolume(), ControllerUnpublishVolume(), NodeStageVolume(), NodeUnstageVolume(), NodePublishVolume(), NodeUnpublishVolume().
  3. workload_secrets
     • Secrets required to auth a specific consumer of a volume.
     • Example: A single NFS share that permits multiple users access via different credentials.
     • Passed to NodePublishVolume(), NodeUnpublishVolume()

As we add additional functionality to the spec (e.g. snapshots) we can add more use case specific secrets.

Considering we want to minimize the breaking changes between 0.2 and 0.3, we should consider this for 0.2.

Update: see updated "Proposal 2" below.

[saad-ali]

Slept on this, and realized the original proposal could block out the ability for plugins to choose which calls to accept which secrets on (the same secret may not be required in all calls and requiring it everywhere may mean having to give access to the secret in components the plugin may not want to).

It may still be worth simplifying though.

Proposal 2:

• Simplify by having one secret per operation pair (reducing 8 possibilities to 4):
  1. create_delete_volume_secrets
     • Grants ability to create and delete volume requests.
     • Passed to CreateVolume() and DeleteVolume().
  2. controller_publish_unpublish_secrets
     • Secrets required to complete controller publish call.
     • Example: an access key for volume.
     • Passed to ControllerPublishVolume() and ControllerUnpublishVolume()
  3. node_stage_unstage_secrets
     • Secrets required to complete node stage call.
     • Example: a decryption key for volume.
     • Passed to NodeStageVolume() and NodeUnstageVolume()
  4. node_publish_unpublish_secrets
     • Secrets required to complete node publish call.
     • Example: end user credentials.
     • Passed to NodePublishVolume() and NodeUnpublishVolume()

[jdef]

I like proposal 2 better than the original. It's unclear to me whether secrets are really needed for symmetric RPCs across the range. For example, I don't see a use case for node "unpublish secrets" or "unstage secrets". controller-oriented secrets make a little more sense to include across symmetric RPCs.

can you provide interesting use cases for providing secrets to node-level teardown-oriented RPCs?

[saad-ali]

For example, I don't see a use case for node "unpublish secrets" or "unstage secrets". controller-oriented secrets make a little more sense to include across symmetric RPCs.

can you provide interesting use cases for providing secrets to node-level teardown-oriented RPCs?

I can't think of a good use case for node_unpublish and node_unstage secrets. In fact, we argued hard to not have volume_attributes be part of node unpublish calls, so I would be completely ok with dropping secrets altogether from the NodeUnstageVolume and NodeUnpublishVolume calls. We can always add them in the future if we find they are necessary (easier to add then remove).

So, proposal 3:

• Simplify by having one secret per operation pair (reducing 8 possibilities to 4):
  1. provisioner_secrets
     • Grants ability to create and delete volume requests.
     • Passed to CreateVolume() and DeleteVolume().
  2. controller_publish_secrets
     • Secrets required to complete controller publish call.
     • Example: an access key for volume.
     • Passed to ControllerPublishVolume() and ControllerUnpublishVolume()
  3. node_stage_secrets
     • Secrets required to complete node stage call.
     • Example: a decryption key for volume.
     • Passed to NodeStageVolume() only.
     • Not passed to NodeUnstageVolume() which we can consider adding in the future, if needed.
  4. node_publish_secrets
     • Secrets required to complete node publish call.
     • Example: end user credentials.
     • Passed to NodePublishVolume() only
     • Not passed to NodeUnpublishVolume() which we can consider adding in the future, if needed.

[saad-ali]

#200 implements proposal 3

[jieyu]

@saad-ali I got a bit confused. I thought that it's CO's job to decide the mapping between the credentials in CO's API and the credentials used in the CSI calls. Why we need this change? As a CO, can I always map provisioner_secrets (in CO's API) to both Create and Delete Volume CSI calls?

[saad-ali]

@saad-ali I got a bit confused. I thought that it's CO's job to decide the mapping between the credentials in CO's API and the credentials used in the CSI calls. Why we need this change? As a CO, can I always map provisioner_secrets (in CO's API) to both Create and Delete Volume CSI calls?

The way it is currently worded SHALL permit passing through the required credentials that is not the case. It implies that end user can specify each of these.

[jieyu]

@saad-ali maybe just change that wording? I do like the flexibility in the current API, and leave to the CO to decide the mapping. For instance, the credentials used for CreateVolume (user) might not always be the same than that used in DeleteVolume (admin).

[saad-ali]

Doing that could result in inconsistencies such that if one CO allows setting all secrets but another maps some subset, plugins will need to depend on the lowest common denominator and basically eliminate the benefit.

[jieyu]

@saad-ali I don't quite follow. I think the logic of dealing with credentials in those calls in a plugin should be largely the same, irrespective of how CO maps them.

[saad-ali]

You're right, if a CO exposes the ability to set 4 secrets but internally maps those 4 secrets to 8 when passed to CSI. And another CO decides to expose all 8 secrets to the end user. It doesn't matter because as far as the CO is concerned, since it still gets the requested secret.

I am fine with just changing the wording to allow for this. What are your thoughts on 1) credential to secret rename, and 2) removing credentials from node unpublish and node unstage?

[jieyu]

@saad-ali i don't have a strong opinion on 1). For 2), what's the motivation? if the motivation is that it is not needed, and we can add that later, then I am ok with that.

[saad-ali]

if the motivation is that it is not needed, and we can add that later, then I am ok with that.

Yes, just that.

Ok, proposal 4:

1. Leave secrets per operation, but loosen wording to allow CO to expose as they choose (1 set of secret, 4 sets of secrets, 8 sets of secrets, etc. and specify there own mapping).
2. Rename credentials to secrets.
3. Remove credentials from node unpublish and nodeunstage, and add it in later if use case arises.

[saad-ali]

One draw back of this approach (proposal 4 vs 3) is that if, for some reason, a plugin expects a secret with the same key but expects different values for different operations, but a CO decides to expose them as a single set of secrets, then there will be name collision. So I would have to add wording like:

  // CO SHALL permit passing through the required secrets. CO MAY expose
  // secrets as a one or more map to its end users, therefore the keys
  // for all unique secrets that a plugin expects, regardless of RPC,
  // must be unique.

#201 implements proposal 4.
#200 implements proposal 3.

We can discuss at today's meeting which one to go with.

[sbezverk]

@saad-ali I would vote for option 1 as it gives maximum flexibility to address any possible use cases which we cannot come up with at this moment. Even a corner case when one credential is needed to create volume but another must be used to delete it, can be addressed if option 1 is selected.

[saad-ali]

I would vote for option 1

Do you mean proposal 4?

[sbezverk]

@saad-ali right, I was referring to point 1 of proposal 4.