stop using Display/FromStr for database representations of per-resource roles

[davepacheco]

tl;dr: We have a couple of enums in omicron_nexus::authz that describe the roles that can be assigned to various resources (e.g., OrganizationRoles that says which roles can be assigned on Organizations). These currently impl Display and FromStr and we use these impls to translate to and from a String that we store into the database. While talking with @ahl, we realized this was a little brittle, in that somebody might not realize that by changing the Display/FromStr impl, they'd be changing the database representation (which has significant consequences for upgrade). This change creates a new trait called AllowedRoles with explicit conversions to/from a String. We use that instead of Display and FromStr.

@ahl @plotnick I'm interested in both of your feedback on this. If this seems like a promising direction, I may add a derive proc macro to generate these rather than doing it by hand. The flip side is that this makes it easy again to accidentally change the representation without realizing it. But maybe we add an expectorate-based unit test to make sure these don't change except very intentionally? Or maybe we just do that instead of all this?

A little more background

Today we have a number of types that are enums in Rust and also enums in the database. As an example, take omicron_nexus::db::model::BlockSize, which corresponds with the omicron.public.block_size database enum. The impl_enum_wrapper! macro is what actually generates the Rust enum and impls Diesel's ToSql and FromSql traits so that the enum can be directly serialized to/from the database. This pattern seems pretty good.

This case is a little different. We only have one role_assignment table that contains role assignments for all kinds of resources -- the Fleet, Silos, Organizations, and Projects. However, the set of allowed roles depends on the resource. So if you've got a role assignment for an Organization, the allowed roles are "viewer" and "admin". For Fleet, they're different. So we can't statically define a database enum and have role_assignment.role_name use that enum as its type. Instead, we allow role_assignment.role_name to be a string and the database does not enforce the constraints on its value.

Previously, we used parse_display to derive Display and FromStr and then used those traits to convert to/from the database values. This change makes that an explicit trait rather than overloading those.

Alternatives

1. Don't do this at all, and don't sweat the fact that we're sort of overloading Display and FromStr for a conversion to a format that we want to be careful about changing (for upgrade-related reasons).
2. Don't do any of this. Instead, create an expectorate-based unit test that dumps all of the values to a file so that you have to be more intentional about changing the format. Maybe if we do that, it's okay to overload Display and FromStr?
3. Can we use impl_enum_wrapper! to generate the enum as well as the ToSql/FromSql impls? I had assumed we couldn't because we're not converting to a database enum (because for the reasons mentioned above, the database is storing this as a String). But if we could do this, we wouldn't need to invent another trait and associated derive macro here.
4. If we can't do that because impl_enum_wrapper! assumes there's a corresponding database type, can we write a similar macro that assumes a string as the underlying database representation? That'd have the same benefit: we wouldn't need to invent another trait and associated derive macro here.
5. Change the database schema so that this is all more type-safe: have a role_assignment table per resource. It would have a role_name column whose type could be a real database enum. The benefit is the database would be checking this invariant and we could use the existing impl_enum_wrapper! mechanism. But I think this would lead to quite a lot more boilerplate in the authz subsystem. The functions that operate on the role_assignments table would need to become generic over some other trait that we'd have to separately impl for each resource that supports roles. My experience trying to do this sort of thing with Diesel has generally gone very poorly. Beyond that: in the future, we expect to support custom roles. In that world, we can't have database enums describing the set of allowed values. We'll have to validate it at the application level, which is what we're currently doing. It seems like the wrong direction to try to bake this into the database if we expect to have to rip it out later.

[ahl]

This is great! I love how you've made this explicit and searchable rather than just using to_string().

My only question is if it would be viable to use omicron_common::api::external::Error directly rather than using anyhow::Error in these trait impls and/or if anyhow is adding something additionally beneficial over, say, using a String which impls Error

[davepacheco]

My only question is if it would be viable to use omicron_common::api::external::Error directly rather than using anyhow::Error in these trait impls and/or if anyhow is adding something additionally beneficial over, say, using a String which impls Error

Good questions. I think these are a little too low-level to use omicron_common::api::external::Error. The caller is always converting this to Error::InternalError because a failure here means we can't understand what came out of the database. But it seems a little much for this thing to assume it's InternalError.

I don't think you can use String the same way as something else that impls Error, so I often reach for anyhow these days when I would be using String.

[plotnick]

I think the trait and its implementation are nice, but I wonder if it should have a more general name like DatabaseString that more closely matches its operational semantics. OTOH if it's really not intended for general use, I guess not.

My other thought is that maybe we could enforce the desired constraints using a database CHECK constraint in the table definition. I agree that complicating the impl_enum_wrapper macro is probably not the right move here, but it does seem like we'd like to ensure that the database can't have semantically invalid rows.

[david-crespo]

Looks good. I had the same thought as Alex about referring to DB serialization explicitly in the name. The "allowed" framing throws me off.

[davepacheco]

I don't mind a different name. Right now, I viewed it as "the trait that all the AllowedRoles types implement" rather than "a generic type for serializing things to a String for use in the database". I'm a little reluctant to first-class the latter given that I think we want to steer people towards database enums where possible. I wonder if there are other cases where this will come up?

My other thought is that maybe we could enforce the desired constraints using a database CHECK constraint in the table definition.

I'm familiar with these, but haven't used them. We could try? I'm still thinking that long-term this isn't going to be type-safe in the database anyway (because of custom roles) so I'm not sure it's worth investing in that?

[davepacheco]

I went ahead and moved authz::AllowedRoles to db::model::DatabaseString with a warning in the docs about when it might not be appropriate.

[davepacheco]

In 205f2e8 I also went ahead and added the test I described.