Skip to content

Switch ListRoles to the paginated API#40165

Merged
gzdunek merged 11 commits intomasterfrom
gzdunek/paginated-roles
Apr 9, 2024
Merged

Switch ListRoles to the paginated API#40165
gzdunek merged 11 commits intomasterfrom
gzdunek/paginated-roles

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Apr 3, 2024

Contributes to https://github.com/gravitational/customer-sensitive-requests/issues/182 (switches ListRoles in Web UI to the paginated API).

e-counterpart: https://github.com/gravitational/teleport.e/pull/3881

Instead of loading all roles with a single call to GetRoles, we now use the paginated API.
The user roles are used in two kinds of UI elements in the Web UI:

  • Resource tables (roles screen, locks screen) - I switched these to serviserside tables (I must admit that it was more difficult than I thought, I hope I used the servierside props correctly).
    • One note: since there is a short delay between updating the resource and having the updated value propagate to the cache, we can't refetch the data immediately after updating the role. Because of that, I have to update the changed role locally, on the browser side (so we have a mix of server side and browser side pagination).
  • Select inputs - these were switched to SelectFieldAsync that loads the options asynchronously on demand. SelectFieldAsync passes a search string to the loadOptions function so we can filter the roles on the server side.

I recommend review commit-by-commit.
I'd also appreciate if you could test these changes locally, as I'm not fully familiar with all the places I have changed 🙏

Changelog: Use a paginated API for listing roles to improve efficiency

Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

Everything looks great so far, and I did test locally. Check the comment I left and let me know what you think.

Comment thread lib/web/resources.go
Comment on lines +118 to +136
return &listResourcesWithoutCountGetResponse{
Items: uiRoles,
StartKey: roles.GetNextKey(),
}, nil
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to be able to handle requests from an old webclient (in the case of two different proxies running together) which would still expect a response that is only an array of items, rather than this paginated object.

Without having to resort to a larger refactor that can version this endpoint, we can just use the existence of one of these new parameters to determine between old/new clients. For example "if limit exists, send new response. otherwise, send only uiRoles"

We did something similar here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, make sure the TODO i left in useKeyBasedPagination would fix this from the frontend side as well please (new webclient being able to handle "old" response types).

Copy link
Copy Markdown
Contributor Author

@gzdunek gzdunek Apr 4, 2024

Choose a reason for hiding this comment

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

Oh, I completely forgot that we can have proxies running different versions.
I added a check on the parameters to determine what response type should be returned. It's not beautiful but works.

Also, make sure the TODO i left in useKeyBasedPagination would fix this from the frontend side as well please

The thing I could do to allow new webclient being able to handle "old" response types, would be to wrap the response in the service layer if I detect that I didn't receive the paginated object. But I'm not sure if it makes any sense, things like limit or search would not work at all.

I don't know how to handle this, to be honest. If I have a server-side table, how can I make it work with both paginated and non-paginated responses?
Maybe instead of modifying the existing endpoint I should add a new one? At least the request would fail with 404 error instead of something like res.map is not a function.
EDIT: I could also throw an error in resource.ts saying ...please update your Teleport Proxy to version ... if I detect a non-paginated response.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know how to handle this, to be honest. If I have a server-side table, how can I make it work with both paginated and non-paginated responses?

The worst case scenario is we receive a very large dataset from the "old" api. I don't have any answer for this yet. An upcoming project will be to add a properly versioned API. I don't know exactly how it would work yet for newer web clients receiving 404s but my guess is there would have to be a fallback to a previous version or just accept the 404. I want to discuss this with the team with an RFD once we get the greenlight for the project.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, we can support the "new webclient being able to handle old response types" case too.
I added a compatibility layer in resource.ts: if we detect that we received a non-paginated response, we can "paginate" it locally, in the browser. I added support for search, limit and startKey.
Let me know what you think about this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i LOVE this. amazing work!!

Copy link
Copy Markdown
Contributor

@avatus avatus left a comment

Choose a reason for hiding this comment

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

Thanks for the backward compatibility change. Everything looks fine to me and works locally. I would appreciate if other reviewers could click around locally as well with things they are more familiar w.r.t roles to double check this one.

Copy link
Copy Markdown
Contributor

@kimlisa kimlisa left a comment

Choose a reason for hiding this comment

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

looks good for the most part, i'm mostly concerned by the static 50 limit ok after playing more with it, i got how it works 👍

Comment thread web/packages/teleport/src/config.ts
Comment thread web/packages/teleport/src/services/resources/resource.ts Outdated
Comment thread web/packages/teleport/src/Roles/Roles.tsx
Comment thread web/packages/teleport/src/Roles/RoleList/RoleList.tsx Outdated
Comment thread web/packages/teleport/src/Roles/RoleList/RoleList.tsx Outdated
@gzdunek gzdunek requested review from avatus and kimlisa April 5, 2024 11:12
Base automatically changed from gzdunek/field-select-async to master April 8, 2024 11:29
@gzdunek gzdunek force-pushed the gzdunek/paginated-roles branch from 7ec8644 to 9a62dab Compare April 8, 2024 11:37
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from rudream April 8, 2024 15:38
@gzdunek gzdunek added this pull request to the merge queue Apr 9, 2024
Merged via the queue into master with commit d12d714 Apr 9, 2024
@gzdunek gzdunek deleted the gzdunek/paginated-roles branch April 9, 2024 07:42
@public-teleport-github-review-bot
Copy link
Copy Markdown

@gzdunek See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Create PR

@kimlisa
Copy link
Copy Markdown
Contributor

kimlisa commented Apr 9, 2024

i had a question on this. i am also introducing a new listing, and i was going to follow the same pattern as access request listing (which is infinite scrolling by use of useKeyBasedPagination) then realized the role listing here is manual fetching (click next).

i was curious why we preferred the manual fetching over the infinite scrolling?

@avatus
Copy link
Copy Markdown
Contributor

avatus commented Apr 9, 2024

I wont speak for @gzdunek , but I chose infinite scrolling over "next" because design wanted it (access requests) to be infinite scrolling. no other reason tbh.

I think it's probably case by case even if we do want a more standard approach. I know notifications are also infinite scrolling

@gzdunek
Copy link
Copy Markdown
Contributor Author

gzdunek commented Apr 10, 2024

Actually I didn't consider switching that screen to infinite scrolling, because I only wanted to change the API from non-paginated to the paginated one.
Also, I thought that "next" fetching was that "more standard approach" for resource tables 😄 (and tbh I didn't know that we now want to prefer infinite scrolling over "next").
I think you may ask Kenny about your case.

gzdunek added a commit that referenced this pull request Apr 11, 2024
* Switch "get roles" endpoint to the paginated version

* Use the paginated API in the roles screen

* Use the paginated API in the locks screen

* Switch select components for roles to an async version

* Do not break backward compatibility of list roles endpoint

* Allow the new web UI to handle old response types

* Disable pagination buttons in the serverside table when loading data

* Add TODO item about rename

* Add `UrlListRolesParams`

* Remove pagination config

* Remove unused `UrlIntegrationExecuteRequestParams`

(cherry picked from commit d12d714)
github-merge-queue Bot pushed a commit that referenced this pull request Apr 22, 2024
* Switch `ListRoles` to the paginated API (#40165)

* Switch "get roles" endpoint to the paginated version

* Use the paginated API in the roles screen

* Use the paginated API in the locks screen

* Switch select components for roles to an async version

* Do not break backward compatibility of list roles endpoint

* Allow the new web UI to handle old response types

* Disable pagination buttons in the serverside table when loading data

* Add TODO item about rename

* Add `UrlListRolesParams`

* Remove pagination config

* Remove unused `UrlIntegrationExecuteRequestParams`

(cherry picked from commit d12d714)

* Fix conflicting files

* `QueryLimitAsInt32` -> `queryLimitAsInt32`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants