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

Include display names in resource policy response #1207

Open
david-crespo opened this issue Jun 14, 2022 · 3 comments
Open

Include display names in resource policy response #1207

david-crespo opened this issue Jun 14, 2022 · 3 comments
Labels
customer For any bug reports or feature requests tied to customer requests

Comments

@david-crespo
Copy link
Contributor

david-crespo commented Jun 14, 2022

As discussed at 6/10 control plane huddle, I am hacking in the user name on the project access page (screenshot below) by fetching all the users and effectively doing the join client side. The upshot of the discussion at huddle was that because

  • The total number of users is potentially quite large
  • Requesting full users just to get the names will send down a lot of stuff we don't need (not to mention a lot of users we don't need)
  • A policy is much more limited in size and likely only mentions a small subset of users

It probably makes sense to do a proper join server-side and include the names in the policy response.

Another significant point I didn't think of at the huddle is that permissions will be handled through role assignment to groups at least as often as to individual users, if not way more often, and in that case we would have to fetch group info in addition to user info. To me this further increases the cost of handling this on the client, and strengthens the case for including human-readable display names for all role-assignable entities referred to in a policy.

image

Alternatives

One alternative is to keep fetching all users. But we can expect the total number of users to exceed our max page size of 1000. As mentioned above there will also be group names to fetch.

Another option could be to have a bulk user fetch endpoint that takes a comma-separated list of user IDs, but you only know what users to fetch after you've seen the policy, so the requests have to happen sequentially. Plus whether it's a bulk fetch specific users or fetch all, we're querying the users table either way — it doesn't seem clearly better than doing it as a real join.

@david-crespo david-crespo changed the title Include name or username in resource policy response? Include name or username in resource policy response Jun 14, 2022
@david-crespo david-crespo changed the title Include name or username in resource policy response Include display names in resource policy response Jun 14, 2022
@davepacheco
Copy link
Collaborator

Plus whether it's a bulk fetch specific users or fetch all, we're querying the users table either way — it doesn't seem clearly better than doing it as a real join.

Two big differences I see:

  • We're always doing the join, even for consumers that don't care (e.g., terraform), and even if the console already has this information. For example, if you're looking at a bunch of policies with overlapping users and groups, we'll do the join a lot more times than the console would have if it just looked up users and groups as they came out (and cached them).
  • On some level we're committing to doing the join in the future (i.e., we'd have to making a breaking API change to separate these in the future).

My usual bias is towards API minimalism to keep requests simple and short ... but I admit the usual considerations around DoS and variable-size requests don't seem that significant here. We do probably need to do an authz check to see whether you're allowed to know anything about the user in the policy, which I think means this information might be absent in the response. This makes me feel like it'd be cleaner to separate these.

Really, this feels like the sort of thing where we should do what's most expedient. If there are performance issues down the line, we'll have more information then. I'm not sure which is most expedient -- probably providing an API for looking up user info by id? I assume we need to build that anyway.

@david-crespo
Copy link
Contributor Author

I can live with the bulk ID version even though I don't prefer it, and it doesn't create any significant tech debt so we can revisit later if bulk fetch doesn't feel great. We have some new tools on the frontend that make it easier to wait to show the entire page until all the data is ready, i.e., until the both policy and the user data have come back. I'll make an issue about bulk user fetch.

@zephraph
Copy link
Contributor

This is also an issue for NICs where we want to display the VPC/Subnet names that they're configured with (if any) but we currently can't do that b/c we only have ids. #1266 will give us the capability to be able to lookup ids which will make it feasible to do on the client with an extra N requests. Ultimately this doesn't seem like the correct way to solve the issue though.

@twinfees twinfees added the customer For any bug reports or feature requests tied to customer requests label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer For any bug reports or feature requests tied to customer requests
Projects
None yet
Development

No branches or pull requests

4 participants