Include Pinned Resources in User Preferences by avatus · Pull Request #32009 · gravitational/teleport

avatus · 2023-09-16T17:40:00Z

Closes #32008
In support of #30418
This will add Pinned Resources to the user preferences object. The shape on web is:

const pinnedResources = {
    "clustername1": ["node1", "node2", "node3"],
    "clustername2": ["node4", "node5", "node6"],
}

We will use this to get a list of resource IDs to display which resources are pinned in the unified resource view.

node: two PRs will be be opened that rely on this. The fetching of the actual resource based on the users preferences, and updating which resources are saved via the Pin button. Both of those will be opened shortly after this one.

It can be tested by inspecting the user preferences local storage object after logging in (it will be empty but there!)

rosstimothy · 2023-09-20T13:48:48Z

+// displayed in the user's pinned resources tab in the Web UI
+message PinnedResourcesUserPreferences {
+  // pinned_resources is a map of resource IDs pinned per cluster
+  map<string, ClusterPinnedResources> pinned_resources = 1;


I've got a few questions around pinned resources that I couldn't find answers to by poking around. If there is a RFD or some other documentation where these decisions have been made please point me to them.

Should pinned resources be stored per cluster rather than having them all in the root cluster?

Is there an upper bound to the number of pinned resources? Is it per cluster or total?

How do pinned resources get cleaned up? Is it manual? What happens if a leaf cluster is removed entirely?

What will be displayed to a user if a pinned resource is removed? Is the resource id enough information for a user to discern which resource they pinned?

Should pinned resources be stored per cluster rather than having them all in the root cluster?

I'm not sure yet. It seems reasonable enough to allow it but I haven't done any super thinking on it yet.

Is there an upper bound to the number of pinned resources? Is it per cluster or total?

There isn't an upperbound programmed in yet, but we'd want to stay under the max size of an item, lets say in dynamodb, which is 400kb. if we have a node id like "123123-123123123-123123123/node", well over 10k could fit and still have plenty of room for more (and other user preferences).

I don't think 10k pinned resources across all clusters is a realistic outcome in terms of what people actually pin, but we should put some safeguards up regardless. and also it comes into play with my next point.

How do pinned resources get cleaned up? Is it manual? What happens if a leaf cluster is removed entirely?

This will require some extra thinking. My first inclination is that when a pinned resources request is made, we can track how many resources "couldn't be found" from their pinned list, and then just remove them. But I don't think thats a good place to do it as I wouldn't want to add extra latency to the request. there should be some way to clean them up however, because if that was left to grow unbounded then we could run into the issue above.

What will be displayed to a user if a pinned resource is removed?

If they pinned something that gets removed or they lose access to it, it just wouldn't be in shown. a "pinned resource" is really just an extra filter with specific IDs. you can do the same with an advanced query, so the same UX if they opened the normal tab and a resource was gone. it just wouldn't be there.

Is the resource id enough information for a user to discern which resource they pinned?

This isn't super relevant yet since they won't see something that doesn't exist. However, I could envision a future where there is a 'user preferences' page that could show the IDs they have pinned (existing and not) and be able to edit it there. Then maybe ID wouldn't be enough, but ID + if it exists or not (with info from existing resources surrounding it) might be enough. That bridge is very far down the road tho.

To summarize, the main points that should probably be solved now-ish or at least documented are the cleanup phase and the max size. I will try to get something written up about this

Based on the above, I'd lean toward storing the pins per-cluster rather than storing all pins in root as is currently done. If we store pins per cluster, doing automatic cleanup as a periodic won't be too difficult. If, on the other hand, we do all pinning in the root that makes things very complicated. Cross-cluster communication is non-trivial.

Also note that a resource disappearing temporarily doesn't necessarily mean that it's gone. An automatic cleanup routine should probably track timestamps and only clean up resource that remain missing for some minimum amount of time + number of checks.

IMO wether or not this PR includes a cleanup mechanism, we should have a reasonably clear idea of how the cleanup mechanism will function prior to merging it.

I don't think it's a bad idea to put an upper limit on the number of pinned resources. Pinning resources becomes less helpful the more you pin, so I'd expect people won't pin too many. Maybe we set a hard cap to something like 300 or 500?

As far as cleanup goes, I think for now it's okay if your preferences reference a resource that no longer exists - they just won't show up in the listing.

We can work with @roraback to get some designs for how you might remove them.

avatus · 2023-09-26T00:05:11Z

Updated to store cluster preferences per cluster

avatus · 2023-10-05T18:50:07Z

-	}
-
-	item, err := createBackendItem(username, preferences)
+	item, err := createBackendItem(username, prefs)


This was removed because originally, we'd only be sending small chunks of the user preferences to update. Now we send the entire thing (before this PR) so we can just completely overwrite the prefs. Using a range to go through what was sent in the protomessage would skip empty values and therefor not allow us to "empty" our pinned resources. Now that we send the entire thing, it works as expected.

rosstimothy · 2023-10-10T14:06:44Z

nit: this is only called in one place and this function has become simple enough it might be worthwhile to just instantiate the response struct inline.

rosstimothy · 2023-10-10T14:07:37Z

nit: only the comments in the .ts files end with punctuation.

Suggested change

// PinnedResourcesUserPreferences is a collection of resource IDs that will be

// displayed in the user's pinned resources tab in the Web UI

// PinnedResourcesUserPreferences is a collection of resource IDs that will be

// displayed in the user's pinned resources tab in the Web UI.

github-actions · 2023-10-10T15:42:33Z

🤖 Vercel preview here: https://docs-5j32kwuz0-goteleport.vercel.app/docs/ver/preview

* Include Pinned Resources in User Preferences (#32009) * Cherry-pick b905eaf * Fix merge conflict * Properly check for SAMLIdPServiceProvider access (#33190) * Properly check for SAMLIdPServiceProvider access * Remove unneeded debug log

github-actions Bot requested review from ibeckermayer and kimlisa September 16, 2023 17:40

github-actions Bot added size/sm ui labels Sep 16, 2023

avatus requested a review from michellescripts September 16, 2023 17:59

kimlisa reviewed Sep 18, 2023

View reviewed changes

avatus mentioned this pull request Sep 18, 2023

Add PinnedResources to ListUnifiedResources Request #32077

Merged

kimlisa approved these changes Sep 18, 2023

View reviewed changes

rosstimothy reviewed Sep 20, 2023

View reviewed changes

avatus mentioned this pull request Sep 21, 2023

RFD 0148 - Pinned Resources in the web UI #32344

Merged

michellescripts removed their request for review September 28, 2023 19:30

avatus commented Oct 5, 2023

View reviewed changes

avatus requested a review from rosstimothy October 5, 2023 18:50

rosstimothy reviewed Oct 6, 2023

View reviewed changes

avatus requested a review from rosstimothy October 7, 2023 01:19

rosstimothy approved these changes Oct 10, 2023

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from ibeckermayer October 10, 2023 14:08

Include Pinned Resources in User Preferences

dceb604

avatus force-pushed the avatus/pinned_user_prefs branch from d2d0b55 to dceb604 Compare October 10, 2023 15:30

avatus temporarily deployed to vercel October 10, 2023 15:31 — with GitHub Actions Inactive

Merge branch 'master' into avatus/pinned_user_prefs

0ecd598

avatus enabled auto-merge October 10, 2023 15:33

avatus added this pull request to the merge queue Oct 10, 2023

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 10, 2023

avatus added this pull request to the merge queue Oct 10, 2023

Merged via the queue into master with commit af6dd45 Oct 10, 2023

avatus deleted the avatus/pinned_user_prefs branch October 10, 2023 18:30

avatus added a commit that referenced this pull request Oct 11, 2023

Include Pinned Resources in User Preferences (#32009)

560cf49

avatus mentioned this pull request Oct 11, 2023

[v14] Pinned Resources backend #33277

Merged

-// PinnedResourcesUserPreferences is a collection of resource IDs that will be
-// displayed in the user's pinned resources tab in the Web UI
+// PinnedResourcesUserPreferences is a collection of resource IDs that will be
+// displayed in the user's pinned resources tab in the Web UI.

Conversation

avatus commented Sep 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rosstimothy Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

fspmarshall Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

zmb3 Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

avatus commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avatus Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rosstimothy Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

rosstimothy Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 10, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rosstimothy Sep 20, 2023 •

edited

Loading

avatus commented Sep 26, 2023 •

edited

Loading