ListSnapshots should be more clear about usage/semantics of the snapshot_id parameter

[timoreimann]

The spec says the following about the snapshot_id parameter to the ListSnapshots RPC:

// Identity information for a specific snapshot. This field is
// OPTIONAL. It can be used to list only a specific snapshot.
// ListSnapshots will return with current snapshot information
// and will not block if the snapshot is being processed after
// it is cut.
string snapshot_id = 4;

(source)

To me, this reads like that CSI drivers may opt to not support filtering by snapshot ID while still assuming that everything should work just fine. Looking at the csi-snapshotter code, however, it becomes obvious that there is an expectation for the CSI driver to apply the snapshot ID filter (the result set must be either zero or one). @xing-yang confirmed this expectation on Slack as well.

The mandatory filtering responsibility for the case that the field is given does not become super clear to me from reading the spec. I think the relevant section should be improved to make it more clear to SPs how their drivers should behave when snapshot_id is set.

[timoreimann]

At the risk of being too pedantic, it seems like the spec would allow setting both starting_token and snapshot_id, possibly leading to the situation where a target snapshot exists but won't be returned because it exists before the start of given pagination token (i.e., token is 42 but the snapshot is on page 3 already).

I'd think that if snapshot_id is given, the pagination-related fields should be ignored. Maybe that's something which should be clarified as well.

[jdef]

To me, this reads like that CSI drivers may opt to not support filtering by snapshot ID while still assuming that everything should work just fine

Yes, snapshot ID is OPTIONAL. If a particular plugin implementation assumes otherwise, then it sounds like a bug in the plugin. The next part of your question pertains to "csi-snapshotter" which is outside the purview of this specification.

At the risk of being too pedantic, it seems like the spec would allow setting both...

The spec says that snapshot_id is for listing a single snapshot, not paging through snapshots. I think that's clear. What's unclear is how a plugin SHOULD respond if both are provided (and they should not be). So we can probably tighten that up.

[timoreimann]

@jdef thanks for the prompt response.

Yes, snapshot ID is OPTIONAL. If a particular plugin implementation assumes otherwise, then it sounds like a bug in the plugin.

My misunderstanding was pretty much the other way around: I assumed that it was optional for the plugin to process the snapshot ID, and that nothing would break if the plugin would always return all snapshots (i.e., it did no filtering). From what I understand now, the parameter is optional to pass in for the CO but once it's specified, the plugin must handle it. Clarification about the latter part is what I filed this issue for.

That said, I'm not sure if the general meaning of the OPTIONAL qualifier in the context of the spec always pertains to the CO. In that case changing one particular endpoint's description may not make sense. I'm having a difficult time making that call, so I'm going to rely on your (and possibly other's) judgment.