Use typed UUIDs for silo user and group #8803

jmpesp · 2025-08-08T19:45:54Z

This commit changes SiloUser and SiloGroup to use typed UUIDs.

The biggest reason that this couldn't happen without a bunch of work was the lookup_resource macro: for a resource like SshKey, it looked like

lookup_resource! {
    name = "SshKey",
    ancestors = [ "Silo", "SiloUser" ],
    lookup_by_name = true,
    soft_deletes = true,
    primary_key_columns = [ { column_name = "id", rust_type = Uuid } ]
}

One of the methods that the lookup_resource macro generates will create a lookup for ancestors of the resource, and this was one using the type returned from the corresponding authz resource's id method:

            quote! { .filter(dsl::#parent_id.eq(#parent_authz_name.id())) },

Changing SiloUser to use a typed UUID in the authz resource:

authz_resource! {
    name = "SiloUser",
    parent = "Silo",
    primary_key = { uuid_kind = SiloUserKind }, <-- here
    roles_allowed = false,
    polar_snippet = Custom,
}

and changing the SiloUser db model to use DbTypedUuid meant that a call to to_db_typed_uuid was required. The lookup_resource macro has no type information from the string "SiloUser", so this PR adds a check: if the ancestor string is suffixed with a '*', then the lookup_resource macro should assume that the parent_id is a typed UUID, and generate the call to to_db_typed_uuid.

Most of the work after that was mechanical, changing Uuid to their typed equivalent, changing method argument types, etc etc.

Some other related things made it into this PR:

UserBuiltIn now also uses a typed UUID as well, distinguishing them from silo users
Actor no longer has the actor_id method, instead requiring call sites to check which variant of Actor is being used
AuthenticatedActor stores the full Actor instead of only the actor id, leading to typed comparisons in its oso::PolarClass impl
User and Group path params are now typed

This commit changes SiloUser and SiloGroup to use typed UUIDs. The biggest reason that this couldn't happen without a bunch of work was the `lookup_resource` macro: for a resource like SshKey, it looked like ``` lookup_resource! { name = "SshKey", ancestors = [ "Silo", "SiloUser" ], lookup_by_name = true, soft_deletes = true, primary_key_columns = [ { column_name = "id", rust_type = Uuid } ] } ``` One of the methods that the `lookup_resource` macro generates will create a lookup for ancestors of the resource, and this was one using the type returned from the corresponding authz resource's `id` method: ``` quote! { .filter(dsl::#parent_id.eq(#parent_authz_name.id())) }, ``` Changing SiloUser to use a typed UUID in the authz resource: ``` authz_resource! { name = "SiloUser", parent = "Silo", primary_key = { uuid_kind = SiloUserKind }, <-- here roles_allowed = false, polar_snippet = Custom, } ``` and changing the `SiloUser` db model to use `DbTypedUuid` meant that a call to `to_db_typed_uuid` was required. The lookup_resource macro has no type information from the string "SiloUser", so this PR adds a check: if the ancestor string is suffixed with a '*', then the lookup_resource macro should assume that the `parent_id` is a typed UUID, and generate the call to `to_db_typed_uuid`. Most of the work after that was mechanical, changing Uuid to their typed equivalent, changing method argument types, etc etc. Some other related things made it into this PR: - UserBuiltIn now also uses a typed UUID as well, distinguishing them from silo users - Actor no longer has the `actor_id` method, instead requiring call sites to check which variant of Actor is being used - AuthenticatedActor stores the full Actor instead of only the actor id, leading to typed comparisons in its oso::PolarClass impl - User and Group path params are now typed

david-crespo · 2025-08-08T20:24:30Z

Can't wait to resolve the conflicts with #7339! I just put that on auto merge, happy to handle rebasing this on top of that when it's in.

jmpesp · 2025-08-08T21:18:51Z

Can't wait to resolve the conflicts with #7339! I just put that on auto merge, happy to handle rebasing this on top of that when it's in.

No no, definitely land that one first! :)

david-crespo · 2025-08-26T16:53:04Z

nexus/types/src/external_api/views.rs

-    UserBuiltin { user_builtin_id: Uuid },
-    SiloUser { silo_user_id: Uuid, silo_id: Uuid },
+    UserBuiltin { user_builtin_id: BuiltInUserUuid },
+    SiloUser { silo_user_id: SiloUserUuid, silo_id: Uuid },


IMO at the view layer these should all be regular UUIDs. I don't think this information is likely to be useful for clients, and might even require shenanigans we'd rather avoid. There are a couple in the OpenAPI schema already but I lean toward considering that a mistake to be fixed.

omicron/openapi/nexus.json

Lines 25998 to 26013 in 33d4f82

"TypedUuidForAlertKind": {

"type": "string",

"format": "uuid"

},

"TypedUuidForAlertReceiverKind": {

"type": "string",

"format": "uuid"

},

"TypedUuidForInstanceKind": {

"type": "string",

"format": "uuid"

},

"TypedUuidForSupportBundleKind": {

"type": "string",

"format": "uuid"

},

PR to get rid of those here #8910

ab1ae6f tries to use the same with trick

david-crespo · 2025-08-26T21:50:24Z

nexus/db-lookup/src/lookup.rs

+            )),
+        }
+    }
+


I think match actor would make it easier to see why this might fail.

agreed, done in f926dad

I noticed in #8803 (comment) that there are some typed UUIDs leaking into the external API. As I said in the comment, I don't think this information is likely to be useful for clients because we don't actually use it in any of the request body types, only the responses. I don't think this change hurts correctness in our code because the conversion to generic UUID usually happens right at the end of request handling, when models are converted to views for response serialization. However, there are a couple of spots where the struct is used a little earlier in the process (see how the affinity datastore functions return view structs) or is shared with internal API code, so there may be a tiny risk to correctness.

david-crespo · 2025-08-26T22:04:47Z

nexus/db-macros/src/lookup.rs

+                (name, true)
+            } else {
+                (name, false)
+            };


same thing, but look how nice

let (name, primary_key_is_typed_uuid) = match name.strip_suffix('*') { Some(stripped) => (stripped, true), None => (name, false), };

removed this code in 94fe84b

david-crespo · 2025-08-26T22:26:48Z

nexus/db-macros/src/lookup.rs

+                    ::nexus_db_model::to_db_typed_uuid(#parent_authz_name.id())
+                )) }
+            } else {
+                quote! { .filter(dsl::#parent_id.eq(#parent_authz_name.id())) }


This makes sense to me. gpt-5 had some interesting suggestions for alternative approaches.

https://gist.github.com/david-crespo/cab60b2f494a3728661dfe7d2926eec0

I don't think any are clearly better, but I kind of like the trait one. The below change does compile, but I'm not sure if there's a similarly simple approach for the parent_id. If there is, it's kinda nice!

diff --git a/nexus/db-macros/src/lookup.rs b/nexus/db-macros/src/lookup.rs index d2c4868277..13cb71f307 100644 --- a/nexus/db-macros/src/lookup.rs +++ b/nexus/db-macros/src/lookup.rs @@ -817,14 +817,8 @@ ).await?; } }, - // If the parent's id is a typed uuid, then the `to_db_typed_uuid` - // method is required to convert to the db typed uuid - if p.primary_key_is_typed_uuid { - quote! { .filter(dsl::#parent_id.eq( - ::nexus_db_model::to_db_typed_uuid(#parent_authz_name.id()) - )) } - } else { - quote! { .filter(dsl::#parent_id.eq(#parent_authz_name.id())) } + quote! { + .filter(dsl::#parent_id.eq(::nexus_db_model::ToDbId::to_db_id(#parent_authz_name.id()))) }, quote! { #parent_authz_name }, ) diff --git a/nexus/db-model/src/lib.rs b/nexus/db-model/src/lib.rs index 92ab5c14e1..6db3ecb060 100644 --- a/nexus/db-model/src/lib.rs +++ b/nexus/db-model/src/lib.rs @@ -196,6 +196,8 @@ pub use migration_state::*; pub use name::*; pub use network_interface::*; +use omicron_uuid_kinds::TypedUuid; +use omicron_uuid_kinds::TypedUuidKind; pub use oximeter_info::*; pub use oximeter_read_policy::*; pub use physical_disk::*; @@ -519,6 +521,26 @@ } } +pub trait ToDbId { + type DbId; + fn to_db_id(self) -> Self::DbId; +} + +impl ToDbId for uuid::Uuid { + type DbId = uuid::Uuid; + fn to_db_id(self) -> Self::DbId { + self + } +} + +impl<K: TypedUuidKind> ToDbId for TypedUuid<K> { + type DbId = DbTypedUuid<K>; + + fn to_db_id(self) -> Self::DbId { + to_db_typed_uuid(self) + } +} + #[cfg(test)] mod tests { use crate::RequestAddressError;

hah, I originally both tried the "make it structured" approach and the "use a trait" approach, but settled on the approach in this PR :) The trait didn't make sense because it was such a small impl, and the structure approach ended up being too complicated to implement without changing every lookup_resource! invocation.

I tried another idea and ended up with a different approach that is simpler than the asterisk suffix: match on the resource name to determine if the lookup should use the to_db_typed_uuid call. Right now this is only true for SiloUser, but for something like Project this avoids having to suffix everything that uses project as a parent.

See commit 94fe84b

Ingenious. The kind of out of the box thinking LLMs will figure out in about a month.

david-crespo

heroic. epic. zero changes to public API

jmpesp requested a review from david-crespo August 8, 2025 19:45

jmpesp added 3 commits August 9, 2025 01:19

Merge branch 'main' into silo_user_and_group_uuids

d041208

regen openapi

beaeb13

Merge branch 'main' into silo_user_and_group_uuids

79bddb2

david-crespo reviewed Aug 26, 2025

View reviewed changes

david-crespo mentioned this pull request Aug 26, 2025

Take TypedUuids out of the external API #8910

Merged

david-crespo reviewed Aug 26, 2025

View reviewed changes

jmpesp added 4 commits August 28, 2025 17:03

Merge branch 'main' into silo_user_and_group_uuids

0c4488d

remove typed uuids in the external api

ab1ae6f

match on actor instead of result of actor function

f926dad

forget asterisk suffix, match on name

94fe84b

david-crespo approved these changes Aug 28, 2025

View reviewed changes

jmpesp merged commit 33ad127 into oxidecomputer:main Aug 28, 2025
17 checks passed

jmpesp deleted the silo_user_and_group_uuids branch August 29, 2025 02:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use typed UUIDs for silo user and group #8803

Use typed UUIDs for silo user and group #8803

Uh oh!

jmpesp commented Aug 8, 2025

Uh oh!

david-crespo commented Aug 8, 2025

Uh oh!

jmpesp commented Aug 8, 2025

Uh oh!

david-crespo Aug 26, 2025

Uh oh!

david-crespo Aug 26, 2025

Uh oh!

jmpesp Aug 28, 2025

Uh oh!

david-crespo Aug 26, 2025

Uh oh!

jmpesp Aug 28, 2025

Uh oh!

david-crespo Aug 26, 2025

Uh oh!

jmpesp Aug 28, 2025

Uh oh!

david-crespo Aug 26, 2025

Uh oh!

jmpesp Aug 28, 2025

Uh oh!

david-crespo Aug 28, 2025

Uh oh!

david-crespo left a comment

Uh oh!

Uh oh!

Uh oh!

	"TypedUuidForAlertKind": {
	"type": "string",
	"format": "uuid"
	},
	"TypedUuidForAlertReceiverKind": {
	"type": "string",
	"format": "uuid"
	},
	"TypedUuidForInstanceKind": {
	"type": "string",
	"format": "uuid"
	},
	"TypedUuidForSupportBundleKind": {
	"type": "string",
	"format": "uuid"
	},

Use typed UUIDs for silo user and group #8803

Use typed UUIDs for silo user and group #8803

Uh oh!

Conversation

jmpesp commented Aug 8, 2025

Uh oh!

david-crespo commented Aug 8, 2025

Uh oh!

jmpesp commented Aug 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!