From 98e132b4674c0e97dfdac5677179defe50a962fa Mon Sep 17 00:00:00 2001 From: Timo Reimann Date: Fri, 17 Apr 2020 12:28:59 +0200 Subject: [PATCH] Support filtering snapshots by IDs Our implementation of ListSnapshots currently returns all snapshots. The request parameter also provides fields to filter by snapshot ID and source volume ID, which we have been ignoring so far. Based on discussions with SIG Storage [1], it became clear that CSI drivers are expected to process present IDs in the ListSnapshots call appropriately. While the spec marks them as OPTIONAL, it turned out that the optionality refers to the CO, not the SP. (A request for clarification has been filed in [2].) As soon as any of IDs are provided, drivers are expected to take them into account. In fact, csi-snapshotter always passes a snapshot ID in and only ever takes the first snapshot item from the list of snapshots returned, which means that our current implementation is buggy for cases where the account holds more than one snapshot (at least on Kubernetes; other COs could potentially behave differently). This change fixes the issue by retrieving the snapshot by direct lookup if a snapshot ID is given. If a source volume ID is given, we do additional filtering by only returning the snapshots that were sourced by the same volume. The existing csi-sanity tests did not catch this bug. A PR to that project was submitted [3] to close the gap. Once it is merged, we should upgrade our version of the test package. [1]: https://kubernetes.slack.com/archives/C8EJ01Z46/p1587038433091900 [2]: https://github.com/container-storage-interface/spec/issues/426 [3]: https://github.com/kubernetes-csi/csi-test/pull/259 --- CHANGELOG.md | 2 + driver/controller.go | 170 ++++++++++++++++++++++++++----------------- 2 files changed, 107 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a46979ec..a1e7949ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## unreleased +* Support filtering snapshots by ID + [[GH-299]](https://github.com/digitalocean/csi-digitalocean/pull/299) * Return minimum disk size field from snapshot response [[GH-298]](https://github.com/digitalocean/csi-digitalocean/pull/298) * Improve debug HTTP server usage diff --git a/driver/controller.go b/driver/controller.go index 6a879e47d..a7d3fa967 100644 --- a/driver/controller.go +++ b/driver/controller.go @@ -772,93 +772,119 @@ func (d *Driver) DeleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequ // ListSnapshots shold not list a snapshot that is being created but has not // been cut successfully yet. func (d *Driver) ListSnapshots(ctx context.Context, req *csi.ListSnapshotsRequest) (*csi.ListSnapshotsResponse, error) { - // Pagination in the CSI world works different than at DO. CSI sends the - // `req.MaxEntries` to indicate how much snapshots it wants. The - // req.StartingToken is returned by us, if we somehow need to indicate that - // we couldn't fetch and need to fetch again. But it's NOT the page number. - // I.e: suppose CSI wants us to fetch 50 entries, we only fetch 30, we need to - // return NextToken as 31 (so req.StartingToken will be set to 31 when CSI - // calls us again), to indicate that we want to continue returning from the - // index 31 up to 50. - - var nextToken int - var err error - if req.StartingToken != "" { - nextToken, err = strconv.Atoi(req.StartingToken) - if err != nil { - return nil, status.Errorf(codes.Aborted, "ListSnapshots starting token %s is not valid : %s", - req.StartingToken, err.Error()) - } - } - - if nextToken != 0 && req.MaxEntries != 0 { - return nil, status.Errorf(codes.Aborted, - "ListSnapshots invalid arguments starting token: %d and max entries: %d can't be non null at the same time", nextToken, req.MaxEntries) - } - + listResp := &csi.ListSnapshotsResponse{} log := d.log.WithFields(logrus.Fields{ + "snapshot_id": req.SnapshotId, + "source_volume_id": req.SourceVolumeId, "req_starting_token": req.StartingToken, "method": "list_snapshots", }) log.Info("list snapshots is called") - // fetch all entries - listOpts := &godo.ListOptions{ - PerPage: int(req.MaxEntries), - } - var snapshots []godo.Snapshot - for { - snaps, resp, err := d.snapshots.ListVolume(ctx, listOpts) + if req.SnapshotId != "" { + snapshot, resp, err := d.snapshots.Get(ctx, req.SnapshotId) if err != nil { - return nil, status.Errorf(codes.Aborted, "ListSnapshots listing volume snapshots has failed: %s", err.Error()) + if resp == nil || resp.StatusCode != http.StatusNotFound { + return nil, status.Errorf(codes.Internal, "failed to get snapshot by ID %s: %s", req.SnapshotId, err) + } + } else { + snap, err := toCSISnapshot(snapshot) + if err != nil { + return nil, status.Errorf(codes.Internal, + "failed to convert DO snapshot to CSI snapshot: %s", err) + } + listResp = &csi.ListSnapshotsResponse{ + Entries: []*csi.ListSnapshotsResponse_Entry{ + { + Snapshot: snap, + }, + }, + } + } + } else { + // Pagination in the CSI world works different than at DO. CSI sends the + // `req.MaxEntries` to indicate how much snapshots it wants. The + // req.StartingToken is returned by us, if we somehow need to indicate that + // we couldn't fetch and need to fetch again. But it's NOT the page number. + // I.e: suppose CSI wants us to fetch 50 entries, we only fetch 30, we need to + // return NextToken as 31 (so req.StartingToken will be set to 31 when CSI + // calls us again), to indicate that we want to continue returning from the + // index 31 up to 50. + + var nextToken int + var err error + if req.StartingToken != "" { + nextToken, err = strconv.Atoi(req.StartingToken) + if err != nil { + return nil, status.Errorf(codes.Aborted, "ListSnapshots starting token %s is not valid : %s", + req.StartingToken, err.Error()) + } } - snapshots = append(snapshots, snaps...) - - if resp.Links == nil || resp.Links.IsLastPage() { - break + if nextToken != 0 && req.MaxEntries != 0 { + return nil, status.Errorf(codes.Aborted, + "ListSnapshots invalid arguments starting token: %d and max entries: %d can't be non null at the same time", nextToken, req.MaxEntries) } - page, err := resp.Links.CurrentPage() - if err != nil { - return nil, err + // fetch all entries + listOpts := &godo.ListOptions{ + PerPage: int(req.MaxEntries), } + var snapshots []godo.Snapshot + for { + snaps, resp, err := d.snapshots.ListVolume(ctx, listOpts) + if err != nil { + return nil, status.Errorf(codes.Aborted, "ListSnapshots listing volume snapshots has failed: %s", err.Error()) + } - listOpts.Page = page + 1 - listOpts.PerPage = len(snaps) - } + snapshots = append(snapshots, snaps...) - if nextToken > len(snapshots) { - return nil, status.Error(codes.Aborted, "ListSnapshots starting token is greater than total number of snapshots") - } + if resp.Links == nil || resp.Links.IsLastPage() { + break + } - if nextToken != 0 { - snapshots = snapshots[nextToken:] - } + page, err := resp.Links.CurrentPage() + if err != nil { + return nil, err + } - if req.MaxEntries != 0 { - nextToken = len(snapshots) - int(req.MaxEntries) - 1 - snapshots = snapshots[:req.MaxEntries] - } + listOpts.Page = page + 1 + listOpts.PerPage = len(snaps) + } - entries := make([]*csi.ListSnapshotsResponse_Entry, 0, len(snapshots)) - for _, snapshot := range snapshots { - snap, err := toCSISnapshot(&snapshot) - if err != nil { - return nil, status.Errorf(codes.Internal, - "couldn't convert DO snapshot to CSI snapshot: %s", err.Error()) + if nextToken > len(snapshots) { + return nil, status.Error(codes.Aborted, "ListSnapshots starting token is greater than total number of snapshots") } - entries = append(entries, &csi.ListSnapshotsResponse_Entry{ - Snapshot: snap, - }) - } + if nextToken != 0 { + snapshots = snapshots[nextToken:] + } - listResp := &csi.ListSnapshotsResponse{ - Entries: entries, - NextToken: strconv.Itoa(nextToken), + if req.MaxEntries != 0 { + nextToken = len(snapshots) - int(req.MaxEntries) - 1 + snapshots = snapshots[:req.MaxEntries] + } + + entries := make([]*csi.ListSnapshotsResponse_Entry, 0, len(snapshots)) + for _, snapshot := range snapshots { + snap, err := toCSISnapshot(&snapshot) + if err != nil { + return nil, status.Errorf(codes.Internal, + "failed to convert DO snapshot to CSI snapshot: %s", err) + } + + entries = append(entries, &csi.ListSnapshotsResponse_Entry{ + Snapshot: snap, + }) + } + listResp = &csi.ListSnapshotsResponse{ + Entries: entries, + NextToken: strconv.Itoa(nextToken), + } } + filterSnapshotEntriesForVolumeID(listResp, req.SourceVolumeId) + log.WithField("response", listResp).Info("snapshots listed") return listResp, nil } @@ -1176,3 +1202,17 @@ func (d *Driver) tagVolume(parentCtx context.Context, vol *godo.Volume) error { _, err = d.tags.TagResources(ctx, d.doTag, tagReq) return err } + +func filterSnapshotEntriesForVolumeID(listResp *csi.ListSnapshotsResponse, sourceVolumeID string) { + if sourceVolumeID == "" { + return + } + + var filteredEntries []*csi.ListSnapshotsResponse_Entry + for _, entry := range listResp.Entries { + if entry.Snapshot.SourceVolumeId == sourceVolumeID { + filteredEntries = append(filteredEntries, entry) + } + } + listResp.Entries = filteredEntries +}