Take TypedUuids out of the external API

[david-crespo]

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. This is no longer true with 823d0d5.

[davepacheco]

CC @sunshowers

I thought that it was possible for the typed uuids to make it across the server-client boundary, and maybe that that's already happening? That seems pretty valuable. But maybe that's currently happening through replace directives? Would those keep working?

[sunshowers]

We do handle typed UUIDs across internal boundaries (currently via replace statements, and in the future through automatic replacement), but in general exposing typed UUIDs in the external API is probably lower value.

[jmpesp]

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.

I was also hoping in a future PR to change to typed UUIDs in the path parameters, request bodies, etc, so that they come in typed and don't have to be converted from a generic UUID. Hopefully the answer is that clients can deal with this change?

[david-crespo]

Right, that's why I called out the unfortunate spots where we seem to be using the same struct both internally and externally.

[david-crespo]

Hopefully the answer is that clients can deal with this change?

For me, the ideal situation would be that this is done at the API request handling layer but it doesn't actually show up in the OpenAPI schema. The clients wouldn't know about it at all. And this reflects the purpose of typed UUIDs: the fact that a client calls it an instance ID doesn't make it one — what makes it one is that it was passed as the ID parameter to an instance endpoint.

That suggests another approach: use TypedUuid everywhere and just define its JSON schema or whatever so that it gets turned into a regular uuid for OpenAPI purposes. However this would kill all typed UUIDs in the internal API.

[david-crespo]

To be clear, I think exposing these in the public API is not lower value, I think it has negative value. It is confusing but adds nothing otherwise.

[sunshowers]

We could provide an alternative schema for TypedUuid for use in the external API via a separate function.

[jmpesp]

Also: what about the replace statements in clients/oxide-client/src/lib.rs? Should those get removed too?

[jmpesp]

?

[jmpesp]

can this be enforced somehow, like not including the uuid kinds dep in the Cargo.toml for this crate?

[david-crespo]

Good idea, I'll try. Though it's not necessary if we go the other route and just have typed UUIDs act like normal ones in the OpenAPI schema.

[david-crespo]

The replaces in clients/oxide-client/src/lib.rs are about the internal API, and the only typed UUID removed from the internal OpenAPI is TypedUuidForSupportBundleKind, which does not appear in the list:

omicron/clients/nexus-client/src/lib.rs

Lines 32 to 83 in 900085e

	replace = {
	// It's kind of unfortunate to pull in such a complex and unstable type
	// as "blueprint" this way, but we have really useful functionality
	// (e.g., diff'ing) that's implemented on our local type.
	Blueprint = nexus_types::deployment::Blueprint,
	BlueprintPhysicalDiskConfig = nexus_types::deployment::BlueprintPhysicalDiskConfig,
	BlueprintPhysicalDiskDisposition = nexus_types::deployment::BlueprintPhysicalDiskDisposition,
	BlueprintZoneImageSource = nexus_types::deployment::BlueprintZoneImageSource,
	Certificate = omicron_common::api::internal::nexus::Certificate,
	ClickhouseMode = nexus_types::deployment::ClickhouseMode,
	ClickhousePolicy = nexus_types::deployment::ClickhousePolicy,
	DatasetKind = omicron_common::api::internal::shared::DatasetKind,
	DnsConfigParams = nexus_types::internal_api::params::DnsConfigParams,
	DnsConfigZone = nexus_types::internal_api::params::DnsConfigZone,
	DnsRecord = nexus_types::internal_api::params::DnsRecord,
	Generation = omicron_common::api::external::Generation,
	ImportExportPolicy = omicron_common::api::external::ImportExportPolicy,
	MacAddr = omicron_common::api::external::MacAddr,
	MgsUpdateDriverStatus = nexus_types::internal_api::views::MgsUpdateDriverStatus,
	Name = omicron_common::api::external::Name,
	NetworkInterface = omicron_common::api::internal::shared::NetworkInterface,
	NetworkInterfaceKind = omicron_common::api::internal::shared::NetworkInterfaceKind,
	NewPasswordHash = omicron_passwords::NewPasswordHash,
	OmicronPhysicalDiskConfig = omicron_common::disk::OmicronPhysicalDiskConfig,
	OmicronPhysicalDisksConfig = omicron_common::disk::OmicronPhysicalDisksConfig,
	OximeterReadMode = nexus_types::deployment::OximeterReadMode,
	OximeterReadPolicy = nexus_types::deployment::OximeterReadPolicy,
	PendingMgsUpdate = nexus_types::deployment::PendingMgsUpdate,
	PlannerChickenSwitches = nexus_types::deployment::PlannerChickenSwitches,
	ReconfiguratorChickenSwitches = nexus_types::deployment::ReconfiguratorChickenSwitches,
	ReconfiguratorChickenSwitchesParam = nexus_types::deployment::ReconfiguratorChickenSwitchesParam,
	ReconfiguratorChickenSwitchesView = nexus_types::deployment::ReconfiguratorChickenSwitchesView,
	RecoverySiloConfig = nexus_sled_agent_shared::recovery_silo::RecoverySiloConfig,
	Srv = nexus_types::internal_api::params::Srv,
	TypedUuidForBlueprintKind = omicron_uuid_kinds::BlueprintUuid,
	TypedUuidForCollectionKind = omicron_uuid_kinds::CollectionUuid,
	TypedUuidForDatasetKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::DatasetKind>,
	TypedUuidForDemoSagaKind = omicron_uuid_kinds::DemoSagaUuid,
	TypedUuidForDownstairsKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::DownstairsKind>,
	TypedUuidForPhysicalDiskKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::PhysicalDiskKind>,
	TypedUuidForPropolisKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::PropolisKind>,
	TypedUuidForSledKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::SledKind>,
	TypedUuidForUpstairsKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::UpstairsKind>,
	TypedUuidForUpstairsRepairKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::UpstairsRepairKind>,
	TypedUuidForUpstairsSessionKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::UpstairsSessionKind>,
	TypedUuidForVolumeKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::VolumeKind>,
	TypedUuidForZpoolKind = omicron_uuid_kinds::TypedUuid<omicron_uuid_kinds::ZpoolKind>,
	UpdateStatus = nexus_types::internal_api::views::UpdateStatus,
	ZoneStatus = nexus_types::internal_api::views::ZoneStatus,
	ZoneStatusVersion = nexus_types::internal_api::views::ZoneStatusVersion,
	ZpoolName = omicron_common::zpool_name::ZpoolName,
	},

[david-crespo]

Completely changed the approach in 823d0d5 to use manual #[schemars(with = "Uuid")] annotations on the fields in question. Much simpler, no test or app code changes.