Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ListSnapshots implementation does not comply with spec #580

Open
ebblake opened this issue Jan 18, 2025 · 0 comments
Open

ListSnapshots implementation does not comply with spec #580

ebblake opened this issue Jan 18, 2025 · 0 comments

Comments

@ebblake
Copy link

ebblake commented Jan 18, 2025

While implementing my CSI driver by using hostpath as a reference, I noticed several spots where the hostpath ListSnapshots() implementation does not comply with the CSI spec:

  • ListSnapshots( MaxEntries: -1 ) This should probably fail with INVALID_ARGUMENT rather than ignoring the value, or dying with "panic: runtime error: makeslice: len out of range"

  • Starting with pre-existing volume "vol" and two Snapshots "vol-snap1" and "vol-snap2" that both used "vol" as their source. ListSnapshots( SourceVolumeId: "vol" ) is hard-coded to return only the first snapshot, instead of an array of all such snapshots. (Or, if you don't want to allow multiple snapshots from a given source (even though CSI says that is valid), then CreateSnapshot should diagnose the attempt to create "vol-snap2" in the first place.)

  • Assuming the previous item is fixed, be careful that ListSnapshots( MaxEntries: 1, SourceVolumeId: "vol" ) returns only one item in Entries plus a non-empty NextToken, rather than two items in Entries and an empty NextToken.

  • Starting with pre-existing volumes "vol1" and "vol2" and snapshot "vol1-snap1". ListSnapshots( SourceVolumeId: "vol2", SnapshotId: "vol1-snap1" ) This should probably either fail with NOT_FOUND or succeed with an empty list, rather than succeeding with "vol1-snap1", since the returned snapshot does NOT match the requested filter on volumes (admittedly, the spec doesn't say precisely how this should be handled)

  • Starting with pre-existing snapshot "vol-snap". ListSnapshots( StartingToken: "garbage", SnapshotId: "vol-snap" ) This should fail with ABORTED, rather than succeeding. My reasoning: StartingToken should only ever be equal to the NextToken returned when all other parameters were the same - but when SnapshotId is set, we can only ever return at most one Snapshot. Since one Snapshot can never exceed MaxEntries, NextToken should always be "" when a call with non-empty SnapshotId succeeds, and therefore, a non-empty StartingToken coupled with a non-empty SnapshotId should always represent a case of the user supplying a next token that was not the result of continuing from a previous ListSnapshots

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant