[RFC] add max-depth to the datastore snapshots API endpoint

[thomas moore <thomas@mooresolutions.co.nz>]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

Hi all

This is a proposal for discussion, not a patch. The
developer documentation asks that plans be discussed before
starting work, so I'd like to check whether this would be
acceptable before writing it.

GET /api2/json/admin/datastore/{store}/snapshots returns only
the snapshots in the requested namespace, so a client wanting
a datastore-wide view has to call
/admin/datastore/{store}/namespace first and then issue one
snapshots request per namespace. I ran into this writing a
third-party monitoring client.

Tested against PBS 4.2:

/admin/datastore/pbs/snapshots           -> 485 snapshots (root
only) 

/admin/datastore/pbs/snapshots?ns=test   -> 1 snapshot 

/admin/datastore/pbs/snapshots?max-depth=7 -> 400
parameter verification failed - 'max-depth': schema does not
allow additional properties

Proposal: accept an optional max-depth using the
existing NS_MAX_DEPTH_SCHEMA, defaulting to 0 so behaviour is
unchanged for current callers.

This looks anticipated in the code. As of
5d95fc20e, src/api2/admin/datastore.rs lines 571, 575 and 583
each carry a "// FIXME: Recursion" on the group
construction in list_snapshots_blocking, with a further one at
565 about filtering by owner before collecting.
ListAccessibleBackupGroups already takes max_depth and applies
the per-namespace privilege and owner checks during iteration,
and get_snapshots_count uses it in roughly the shape list_snapshots
would need.

I'd send it as two patches: the refactor
onto ListAccessibleBackupGroups with no behavioural change, then
max-depth on top. I can test on a 4.2 instance including with a
token holding DatastoreAudit on the root namespace only, to
confirm namespaces the token cannot see stay absent from a
recursive listing.





Questions for the list

1. Is adding max-depth to the snapshots endpoint acceptable in
principle?

2. Is ListAccessibleBackupGroups the right vehicle here, or is
there a reason list_snapshots 	does not already use it?

3. Snapshot entries carry no namespace field, so a recursive
listing is ambiguous unless the 	client tracks it per request. Adding
one would mean a new field on SnapshotListItem, which 	is also
decoded from remote instances in server/sync.rs and server/push.rs

If you'd rather this were solved another way or not at all I'm
happy to hear it before writing anything.

Regards Thomas Moore

[-- Attachment #2: Type: text/html, Size: 4201 bytes --]