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

clean up remaining "fetch" interfaces in datastore #845

Closed
davepacheco opened this issue Apr 1, 2022 · 0 comments
Closed

clean up remaining "fetch" interfaces in datastore #845

davepacheco opened this issue Apr 1, 2022 · 0 comments

Comments

@davepacheco
Copy link
Collaborator

davepacheco commented Apr 1, 2022

#798 and follow-on work converted the vast majority of Nexus to use the new internal lookup API, but there are a handful of stragglers in the DataStore that will be more work to convert.

The two big blockers are:

  • Tables with primary keys other than "id" and ([parent_id], name). The macro that implements new internal lookup API #798 only works for resources backed by tables with an "id" and "name" column per RFD 192. Some resources are missing a "name" (we call these "assets" in some places), some are missing an "id", and some have composite primary keys. More below. I'd propose that we make the macro more configurable to support these cases.
  • A couple of the remaining resource types are (or should be) implemented with authz::FleetChild, which is constructed differently than both the "generic" and "typed" authz resources that are supported by the macro. It'd be pretty easy to extend the macro to support this, but I think that's wasted effort. I think we should do generic authz types could be more type-safe #848 instead.

A fair question is whether it's worth fixing these just to get these stragglers into the main lookup API. I think it is worth that effort because the main problem is just the different primary keys, and I think that's a really useful pattern that I want to make sure remains first-class. We don't want to start adding columns that don't make sense or indexes that don't make sense just to make things fit a pattern that doesn't make sense for them.

More details on the remaining "fetch" stuff in DataStore:

  • sled_fetch: these have no "name" column.
  • user_builtin_fetch: for authz, these use the generic authz::FleetChild. Like ProjectChild, these are constructed using parent.child_generic(), but they take a different number of arguments. As I mentioned above, we could generalize the macro's AuthzKind argument to support this but it feels like going further down the wrong path.
  • silo_fetch: generic FleetChild problem.
  • role_builtin_fetch: these have no id column and their primary key is the composite (resource_type, role_name). They will also have the generic authz::FleetChild problem.
  • update_available_artifact_fetch: these have no id column and their primary key is the composite (name, kind, version). Their authz check today is based on being able to read the Fleet. It'd be easy to add a new authz::FleetChild for them... at which point they'll have the same problem as the above.
  • silo_user_fetch: silo_user has no name column.
  • silo_user_fetch and session_fetch: see authentication operations should use special Nexus context #846.

This is essentially blocked on #847 and #848.

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

No branches or pull requests

1 participant