Implement ListSnapshot

[leakingtapan]

Is your feature request related to a problem? Please describe.
As a follow up to #25

/cc @tsmetana

[zacharya]

Is anyone working on this? I'd like to take it on.

[leakingtapan]

/assign @zacharya

[k8s-ci-robot]

@leakingtapan: GitHub didn't allow me to assign the following users: zacharya.

Note that only kubernetes-sigs members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @zacharya

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.

[zacharya]

I'm ready to put in a PR for this, just have one question:

The CSI spec allows ListSnapshotsRequest.MaxEntries to be anything 0 or greater, but the AWS API requires DescribeSnapshotsInput.MaxResults to be between 5 and 1000. I'm setting DescribeSnapshotsInput.MaxResults if ListSnapshotsRequest.MaxEntries is greater than 0. If it's between 1 and 4, I set it to 5 and just trim the slice before returning to facilitate compliance with the CSI spec.

Do we want to do the same thing with values greater than 1000? One approach there would be to facilitate multiple calls, but there is currently no upper limit to ListSnapshotsRequest.MaxEntries in the spec other than int32 bounds. Another approach is to just let the AWS API behave as documented and we can return 1000 entries and a NextToken.

[leakingtapan]

but the AWS API requires DescribeSnapshotsInput.MaxResults to be between 5 and 1000. I'm setting

What's the DescribeSnapshotsInput behavior when maxResults is less than 5?

I set it to 5 and just trim the slice before returning to facilitate compliance with the CSI spec.

After trim, the next time ListSnapshotsis called, is the elements between 2 and 4 going to be returned?

Another approach is to just let the AWS API behave as documented and we can return 1000 entries and a NextToken

I feel this way is better. @bertinatto how do you think?

[zacharya]

When it's less than 5, it will return an InvalidParameterValue error. In this way it differs from being greater than 1000 where it will just truncate and return a NextToken value that starts with 1001. We could return an error, but then there would be AWS-specific CSI behavior leaking through.

Between 2 and 4 is tough. This is where some type of state comes in. I'm not sure I have a good idea here. Maybe using some type of custom NextToken value that subsequent requests could use to determine where to pick up? An example NextToken value from AWS which is just a base64 encoded JSON object is eyJ2IjoiMiIsImMiOiJRRnFmMGZzcjNkaUtKSldDK1VOOTQwZUt3TTMwbEdHUlFsNDVtTFo5SkNCdVFIL0plZXVyR1NZVGpCbDR3WFpmZnJZd1lzRFdLeEtkM3EydkZkZHZITGE1bHVkMFRwWW9hSktaM0MzY3JyTnZJWmtINmZZY0F3eHY3Ym5sQk9DcEVNNXNnSjBFeHFTRmM1QndYMy9rb04xY29CYzJQS2t0MUVXZCs5LzZocVNoTDA5WnRlbjRvRTFXUm1YbzA1QUJHZUp2Nlk3b3B3T0dqZz09IiwicyI6IjEifQ==. If we appended the number of entries we still have to process less than 5 e.g. eyJ2IjoiMiIsImMiOiJRRnFmMGZzcjNkaUtKSldDK1VOOTQwZUt3TTMwbEdHUlFsNDVtTFo5SkNCdVFIL0plZXVyR1NZVGpCbDR3WFpmZnJZd1lzRFdLeEtkM3EydkZkZHZITGE1bHVkMFRwWW9hSktaM0MzY3JyTnZJWmtINmZZY0F3eHY3Ym5sQk9DcEVNNXNnSjBFeHFTRmM1QndYMy9rb04xY29CYzJQS2t0MUVXZCs5LzZocVNoTDA5WnRlbjRvRTFXUm1YbzA1QUJHZUp2Nlk3b3B3T0dqZz09IiwicyI6IjEifQ==4 then the next request can ignore the NextToken, call the API again without it, truncate the responses for snapshots 2-4 then return the original NextToken. Or, if it's still less than 5, we can decrement the appended value and return that. Definitely not ideal, but it keeps us from having to store state anywhere.

Agreed that capping our response to 1000 entries is the better approach.

[zacharya]

@bertinatto Any thoughts here?

[leakingtapan]

I'm fine with keeping it simple by returnning gRPC InvalidArgument error when page size is smaller then 5. This way we could still improve it later for the <5 cases. Don't think we need to optimize this case too much just to cover the <5 case, since ListSnapshot API is not yet adopted by K8S AFAIK.