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

Implement paginated lists in CLI, APIs, and storage #170

Merged
merged 8 commits into from
Mar 25, 2024

Conversation

cipherboy
Copy link
Member

This implements list paginations using an after/limit style system, discussed in #140.

In particular, LIST API operations are GET requests with a ?list=true parameter to an endpoint. This adds the ability (in the CLI, API, and underlying storage mechanism) to specify two additional attributes, a key, after, to show results whose key is strictly greater than (alphabetically) the specified value, and a number, limit, of results to show.

This allows better user interface design: pagination can be done on the server (in addition to the UI), which allows the client to consume less resources (both locally and on the remote server), when lots of entries are present. Endpoints like PKI's revoked or valid certificates, Transit's lists of keys, or entries in K/V v2 are all common endpoints upgraded with pagination that benefit from this improvement.

This is an example of a change made possible by dropping support for alternative storage backends.

Resolves: #140

@schultz-is
Copy link
Contributor

This is great! I can attest to this being a highly desired feature. Notably, KV and PKI backend deployments can get large fast, so the ability to page over all data within them is super handy. The fact that this approach is also backwards compatible (doesn't add any output parameters like cursor paging,) is a nice bonus; that means nobody has to update any existing scripts. Passing the current List operations through the ListPage operation will also net free test coverage.

Copy link
Member

@JanMa JanMa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small remarks, otherwise it looks good 👍

builtin/credential/approle/path_role.go Outdated Show resolved Hide resolved
website/content/docs/internals/telemetry/metrics/all.mdx Outdated Show resolved Hide resolved
This adds the ability to list a specified number of entries, seeking
until after after a given reference entry, to the storage backends.
With efficient implementations, this should allow reducing the memory
overhead of API LIST operations, though at the lack of consistency
between fetches. This should allow more expensive tidy operations
(such as PKI's) to more correctly release resources between operations
and gracefully handle larger numbers of certificates.

Additionally, this functionality will be useful for implementing
better pagination on list views (again, like PKI's), as finer control
over fetched entries from the server can be had, avoiding the need
to keep the entire list in the browser's memory.

This has broader applicability beyond PKI.

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>
Unlike most other endpoints, List does not take a generic Data
parameter. For pagination, we wish to standardize the parameters
(generally) and so use the same calling convention (for after and
limit) as used in the storage APIs.

In the future, if more endpoints offer paginated lists and wish to do
so via other filtering mechanisms (or needing multi-part filters for
nested references such as with PKI's revocation queue list endpoints),
a generic ListWithData helper could be added instead.

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>
This exposes the new pagination filters on the `bao list` command line
and expands the existing description text for both pagination and
detailed list responses.

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>
This uses ListPage for most PKI list endpoints, with the exception of
the complex (two-level: cluster + serial) endpoints for cross-cluster
revocation queues. These hopefully drain rather quickly and it would be
harder to adapt truncation for two parameters (and the CLI would likely
have to allow general K/V parameters to list endpoints which it
currently doesn't as one of the two after values would have to be named
differently).

However, this enables it for leaf and revoked certs (both very large),
roles (which, due to the ACL model, can grow large), and the potentially
less-large but still useful to have paginated on the upper end issuers
and keys listings.

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>
As mentioned in the plugin proposal, external plugins which build
against OpenBao's SDK but run against HashiCorp Vault will need a shim
module to provide compatibility of the ListPage(...) call if it is used.

While the plugin could do this itself (catching the error and then
supplementing its own shim), doing this at the GRPC layer allows plugins
to mostly ignore the differences between the two server versions.

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>
This upgrades remaining plugin list endpoints to support pagination,
maximizing utility of the new CLI & API helpers. Many of these places
(such as K/V, Transit keys, or SSH roles) have the potential for large
quantities of entries, making these useful in general to have. This also
updates the sys/raw interface to support pagination as well, as many
plugins can write a large number of storage entires into any given
folder.

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>
Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>
Also fix the missing references to the metrics system.

Signed-off-by: Alexander Scheel <alexander.m.scheel@gmail.com>
@naphelps naphelps merged commit 4406ece into openbao:main Mar 25, 2024
73 of 74 checks passed
@cipherboy cipherboy mentioned this pull request Apr 6, 2024
6 tasks
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

Successfully merging this pull request may close these issues.

RFC - Support Paginated Lists in Storage, APIs
4 participants