From 5c5c53f4a22db348528e6b531d43963ae3827ff9 Mon Sep 17 00:00:00 2001 From: iliana etaoin Date: Tue, 14 Jan 2025 17:44:41 +0000 Subject: [PATCH] replace TUF artifact primary key with a UUID --- nexus/auth/src/authz/api_resources.rs | 4 +- nexus/db-model/src/schema.rs | 12 +-- nexus/db-model/src/schema_versions.rs | 3 +- nexus/db-model/src/tuf_repo.rs | 97 +++++-------------- nexus/db-queries/src/db/datastore/update.rs | 25 ++--- nexus/db-queries/src/db/lookup.rs | 20 +--- nexus/db-queries/src/policy_test/resources.rs | 14 +-- nexus/db-queries/tests/output/authz-roles.out | 2 +- schema/crdb/dbinit.sql | 22 +---- schema/crdb/tuf-artifact-key-uuid/up01.sql | 4 + schema/crdb/tuf-artifact-key-uuid/up02.sql | 2 + schema/crdb/tuf-artifact-key-uuid/up03.sql | 10 ++ schema/crdb/tuf-artifact-key-uuid/up04.sql | 5 + uuid-kinds/src/lib.rs | 1 + 14 files changed, 79 insertions(+), 142 deletions(-) create mode 100644 schema/crdb/tuf-artifact-key-uuid/up01.sql create mode 100644 schema/crdb/tuf-artifact-key-uuid/up02.sql create mode 100644 schema/crdb/tuf-artifact-key-uuid/up03.sql create mode 100644 schema/crdb/tuf-artifact-key-uuid/up04.sql diff --git a/nexus/auth/src/authz/api_resources.rs b/nexus/auth/src/authz/api_resources.rs index bfde3d3b972..ee12c453627 100644 --- a/nexus/auth/src/authz/api_resources.rs +++ b/nexus/auth/src/authz/api_resources.rs @@ -38,7 +38,6 @@ use authz_macros::authz_resource; use futures::future::BoxFuture; use futures::FutureExt; use nexus_db_fixed_data::FLEET_ID; -use nexus_db_model::{ArtifactId, SemverVersion}; use nexus_types::external_api::shared::{FleetRole, ProjectRole, SiloRole}; use omicron_common::api::external::{Error, LookupType, ResourceType}; use once_cell::sync::Lazy; @@ -1014,8 +1013,7 @@ authz_resource! { authz_resource! { name = "TufArtifact", parent = "Fleet", - primary_key = (String, SemverVersion, String), - input_key = ArtifactId, + primary_key = { uuid_kind = TufArtifactKind }, roles_allowed = false, polar_snippet = FleetChild, } diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index a8e1141db6e..e464a6e0ac3 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -1336,7 +1336,8 @@ table! { } table! { - tuf_artifact (name, version, kind) { + tuf_artifact (id) { + id -> Uuid, name -> Text, version -> Text, kind -> Text, @@ -1347,11 +1348,9 @@ table! { } table! { - tuf_repo_artifact (tuf_repo_id, tuf_artifact_name, tuf_artifact_version, tuf_artifact_kind) { + tuf_repo_artifact (tuf_repo_id, tuf_artifact_id) { tuf_repo_id -> Uuid, - tuf_artifact_name -> Text, - tuf_artifact_version -> Text, - tuf_artifact_kind -> Text, + tuf_artifact_id -> Uuid, } } @@ -1361,8 +1360,7 @@ allow_tables_to_appear_in_same_query!( tuf_artifact ); joinable!(tuf_repo_artifact -> tuf_repo (tuf_repo_id)); -// Can't specify joinable for a composite primary key (tuf_repo_artifact -> -// tuf_artifact). +joinable!(tuf_repo_artifact -> tuf_artifact (tuf_artifact_id)); table! { support_bundle { diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 4542283aac1..11f6e68b6c8 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(118, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(119, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -29,6 +29,7 @@ static KNOWN_VERSIONS: Lazy> = Lazy::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(119, "tuf-artifact-key-uuid"), KnownVersion::new(118, "support-bundles"), KnownVersion::new(117, "add-completing-and-new-region-volume"), KnownVersion::new(116, "bp-physical-disk-disposition"), diff --git a/nexus/db-model/src/tuf_repo.rs b/nexus/db-model/src/tuf_repo.rs index 6f5a898a2d6..f603e681887 100644 --- a/nexus/db-model/src/tuf_repo.rs +++ b/nexus/db-model/src/tuf_repo.rs @@ -13,11 +13,9 @@ use chrono::{DateTime, Utc}; use diesel::{deserialize::FromSql, serialize::ToSql, sql_types::Text}; use omicron_common::{ api::external, - update::{ - ArtifactHash as ExternalArtifactHash, ArtifactId as ExternalArtifactId, - ArtifactKind, - }, + update::{ArtifactHash as ExternalArtifactHash, ArtifactId, ArtifactKind}, }; +use omicron_uuid_kinds::TufArtifactKind; use omicron_uuid_kinds::TufRepoKind; use omicron_uuid_kinds::TypedUuid; use serde::{Deserialize, Serialize}; @@ -148,8 +146,10 @@ impl TufRepo { #[derive(Queryable, Insertable, Clone, Debug, Selectable, AsChangeset)] #[diesel(table_name = tuf_artifact)] pub struct TufArtifact { - #[diesel(embed)] - pub id: ArtifactId, + pub id: DbTypedUuid, + pub name: String, + pub version: SemverVersion, + pub kind: String, pub time_created: DateTime, pub sha256: ArtifactHash, artifact_size: i64, @@ -158,12 +158,15 @@ pub struct TufArtifact { impl TufArtifact { /// Creates a new `TufArtifact` ready for insertion. pub fn new( - id: ArtifactId, + artifact_id: ArtifactId, sha256: ArtifactHash, artifact_size: u64, ) -> Self { Self { - id, + id: TypedUuid::new_v4().into(), + name: artifact_id.name, + version: artifact_id.version.into(), + kind: artifact_id.kind.as_str().to_owned(), time_created: Utc::now(), sha256, artifact_size: artifact_size as i64, @@ -177,21 +180,31 @@ impl TufArtifact { /// as part of the process, which `From` doesn't necessarily communicate /// and can be surprising. pub fn from_external(artifact: external::TufArtifactMeta) -> Self { - Self::new(artifact.id.into(), artifact.hash.into(), artifact.size) + Self::new(artifact.id, artifact.hash.into(), artifact.size) } /// Converts self into [`external::TufArtifactMeta`]. pub fn into_external(self) -> external::TufArtifactMeta { external::TufArtifactMeta { - id: self.id.into(), + id: ArtifactId { + name: self.name, + version: self.version.into(), + kind: ArtifactKind::new(self.kind), + }, hash: self.sha256.into(), size: self.artifact_size as u64, } } /// Returns the artifact's ID. - pub fn id(&self) -> (String, SemverVersion, String) { - (self.id.name.clone(), self.id.version.clone(), self.id.kind.clone()) + pub fn id(&self) -> TypedUuid { + self.id.into() + } + + /// Returns the artifact's name, version, and kind, which is unique across + /// all artifacts. + pub fn nvk(&self) -> (&str, &SemverVersion, &str) { + (&self.name, &self.version, &self.kind) } /// Returns the artifact length in bytes. @@ -200,70 +213,12 @@ impl TufArtifact { } } -/// The ID (primary key) of a [`TufArtifact`]. -/// -/// This is the internal variant of a [`ExternalArtifactId`]. -#[derive( - Queryable, - Insertable, - Clone, - Debug, - Selectable, - PartialEq, - Eq, - Hash, - Deserialize, - Serialize, -)] -#[diesel(table_name = tuf_artifact)] -pub struct ArtifactId { - pub name: String, - pub version: SemverVersion, - pub kind: String, -} - -impl From for ArtifactId { - fn from(id: ExternalArtifactId) -> Self { - Self { - name: id.name, - version: id.version.into(), - kind: id.kind.as_str().to_owned(), - } - } -} - -impl From for ExternalArtifactId { - fn from(id: ArtifactId) -> Self { - Self { - name: id.name, - version: id.version.into(), - kind: ArtifactKind::new(id.kind), - } - } -} - -impl fmt::Display for ArtifactId { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - // This is the same as ExternalArtifactId's Display impl. - write!(f, "{} v{} ({})", self.name, self.version, self.kind) - } -} - -/// Required by the authz_resource macro. -impl From for (String, SemverVersion, String) { - fn from(id: ArtifactId) -> Self { - (id.name, id.version, id.kind) - } -} - /// A many-to-many relationship between [`TufRepo`] and [`TufArtifact`]. #[derive(Queryable, Insertable, Clone, Debug, Selectable)] #[diesel(table_name = tuf_repo_artifact)] pub struct TufRepoArtifact { pub tuf_repo_id: Uuid, - pub tuf_artifact_name: String, - pub tuf_artifact_version: SemverVersion, - pub tuf_artifact_kind: String, + pub tuf_artifact_id: Uuid, } /// A wrapper around omicron-common's [`ArtifactHash`](ExternalArtifactHash), diff --git a/nexus/db-queries/src/db/datastore/update.rs b/nexus/db-queries/src/db/datastore/update.rs index 37339beb628..f1a62d53d2a 100644 --- a/nexus/db-queries/src/db/datastore/update.rs +++ b/nexus/db-queries/src/db/datastore/update.rs @@ -50,15 +50,8 @@ async fn artifacts_for_repo( use db::schema::tuf_artifact::dsl as tuf_artifact_dsl; use db::schema::tuf_repo_artifact::dsl as tuf_repo_artifact_dsl; - let join_on_dsl = tuf_artifact_dsl::name - .eq(tuf_repo_artifact_dsl::tuf_artifact_name) - .and( - tuf_artifact_dsl::version - .eq(tuf_repo_artifact_dsl::tuf_artifact_version), - ) - .and( - tuf_artifact_dsl::kind.eq(tuf_repo_artifact_dsl::tuf_artifact_kind), - ); + let join_on_dsl = + tuf_artifact_dsl::id.eq(tuf_repo_artifact_dsl::tuf_artifact_id); // Don't bother paginating because each repo should only have a few (under // 20) artifacts. tuf_repo_artifact_dsl::tuf_repo_artifact @@ -215,9 +208,9 @@ async fn insert_impl( for artifact in desc.artifacts.clone() { filter_dsl = filter_dsl.or_filter( dsl::name - .eq(artifact.id.name) - .and(dsl::version.eq(artifact.id.version)) - .and(dsl::kind.eq(artifact.id.kind)), + .eq(artifact.name) + .and(dsl::version.eq(artifact.version)) + .and(dsl::kind.eq(artifact.kind)), ); } @@ -233,7 +226,7 @@ async fn insert_impl( let results_by_id = results .iter() - .map(|artifact| (&artifact.id, artifact)) + .map(|artifact| (artifact.nvk(), artifact)) .collect::>(); // uploaded_and_existing contains non-matching artifacts in pairs of @@ -244,7 +237,7 @@ async fn insert_impl( for uploaded_artifact in desc.artifacts.clone() { let Some(&existing_artifact) = - results_by_id.get(&uploaded_artifact.id) + results_by_id.get(&uploaded_artifact.nvk()) else { // This is a new artifact. new_artifacts.push(uploaded_artifact.clone()); @@ -301,9 +294,7 @@ async fn insert_impl( ); values.push(( dsl::tuf_repo_id.eq(desc.repo.id), - dsl::tuf_artifact_name.eq(artifact.id.name), - dsl::tuf_artifact_version.eq(artifact.id.version), - dsl::tuf_artifact_kind.eq(artifact.id.kind), + dsl::tuf_artifact_id.eq(artifact.id), )); } diff --git a/nexus/db-queries/src/db/lookup.rs b/nexus/db-queries/src/db/lookup.rs index e3b118f94fe..0507da6839d 100644 --- a/nexus/db-queries/src/db/lookup.rs +++ b/nexus/db-queries/src/db/lookup.rs @@ -22,6 +22,7 @@ use omicron_common::api::external::Error; use omicron_common::api::external::InternalContext; use omicron_common::api::external::{LookupResult, LookupType, ResourceType}; use omicron_uuid_kinds::PhysicalDiskUuid; +use omicron_uuid_kinds::TufArtifactKind; use omicron_uuid_kinds::TufRepoKind; use omicron_uuid_kinds::TypedUuid; use uuid::Uuid; @@ -446,18 +447,11 @@ impl<'a> LookupPath<'a> { /// Select a resource of type UpdateArtifact, identified by its /// `(name, version, kind)` tuple - pub fn tuf_artifact_tuple( + pub fn tuf_artifact_id( self, - name: impl Into, - version: db::model::SemverVersion, - kind: impl Into, + id: TypedUuid, ) -> TufArtifact<'a> { - TufArtifact::PrimaryKey( - Root { lookup_root: self }, - name.into(), - version, - kind.into(), - ) + TufArtifact::PrimaryKey(Root { lookup_root: self }, id) } /// Select a resource of type UserBuiltin, identified by its `name` @@ -895,11 +889,7 @@ lookup_resource! { children = [], lookup_by_name = false, soft_deletes = false, - primary_key_columns = [ - { column_name = "name", rust_type = String }, - { column_name = "version", rust_type = db::model::SemverVersion }, - { column_name = "kind", rust_type = String }, - ] + primary_key_columns = [ { column_name = "id", uuid_kind = TufArtifactKind } ] } lookup_resource! { diff --git a/nexus/db-queries/src/policy_test/resources.rs b/nexus/db-queries/src/policy_test/resources.rs index 8075908d05a..dcd8704086b 100644 --- a/nexus/db-queries/src/policy_test/resources.rs +++ b/nexus/db-queries/src/policy_test/resources.rs @@ -6,9 +6,7 @@ use super::resource_builder::ResourceBuilder; use super::resource_builder::ResourceSet; -use crate::db::model::ArtifactId; use nexus_auth::authz; -use nexus_db_model::SemverVersion; use omicron_common::api::external::LookupType; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::PhysicalDiskUuid; @@ -139,16 +137,12 @@ pub async fn make_resources( LookupType::ById(tuf_repo_id.into_untyped_uuid()), )); - let artifact_id = ArtifactId { - name: "a".to_owned(), - version: SemverVersion("1.0.0".parse().unwrap()), - kind: "b".to_owned(), - }; - let artifact_id_desc = artifact_id.to_string(); + let tuf_artifact_id = + "6827813e-bfaa-4205-9b9f-9f7901e4aab1".parse().unwrap(); builder.new_resource(authz::TufArtifact::new( authz::FLEET, - artifact_id, - LookupType::ByCompositeId(artifact_id_desc), + tuf_artifact_id, + LookupType::ById(tuf_artifact_id.into_untyped_uuid()), )); let address_lot_id = diff --git a/nexus/db-queries/tests/output/authz-roles.out b/nexus/db-queries/tests/output/authz-roles.out index 36403fa700b..eebece9d524 100644 --- a/nexus/db-queries/tests/output/authz-roles.out +++ b/nexus/db-queries/tests/output/authz-roles.out @@ -1090,7 +1090,7 @@ resource: TufRepo id "3c52d72f-cbf7-4951-a62f-a4154e74da87" silo1-proj1-viewer ✘ ✘ ✘ ✘ ✘ ✘ ✘ ✘ unauthenticated ! ! ! ! ! ! ! ! -resource: TufArtifact id "a v1.0.0 (b)" +resource: TufArtifact id "6827813e-bfaa-4205-9b9f-9f7901e4aab1" USER Q R LC RP M MP CC D fleet-admin ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔ diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index ce6764bd17c..91aa2461b0c 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2352,6 +2352,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.tuf_repo ( -- In the future, this may also be used to describe artifacts that are fetched -- from a remote TUF repo, but that requires some additional design work. CREATE TABLE IF NOT EXISTS omicron.public.tuf_artifact ( + id UUID PRIMARY KEY, name STRING(63) NOT NULL, version STRING(63) NOT NULL, -- This used to be an enum but is now a string, because it can represent @@ -2368,29 +2369,16 @@ CREATE TABLE IF NOT EXISTS omicron.public.tuf_artifact ( -- The length of the artifact, in bytes. artifact_size INT8 NOT NULL, - PRIMARY KEY (name, version, kind) + CONSTRAINT unique_name_version_kind UNIQUE (name, version, kind) ); -- Reflects that a particular artifact was provided by a particular TUF repo. -- This is a many-many mapping. CREATE TABLE IF NOT EXISTS omicron.public.tuf_repo_artifact ( tuf_repo_id UUID NOT NULL, - tuf_artifact_name STRING(63) NOT NULL, - tuf_artifact_version STRING(63) NOT NULL, - tuf_artifact_kind STRING(63) NOT NULL, - - /* - For the primary key, this definition uses the natural key rather than a - smaller surrogate key (UUID). That's because with CockroachDB the most - important factor in selecting a primary key is the ability to distribute - well. In this case, the first element of the primary key is the tuf_repo_id, - which is a random UUID. + tuf_artifact_id UUID NOT NULL, - For more, see https://www.cockroachlabs.com/blog/how-to-choose-a-primary-key/. - */ - PRIMARY KEY ( - tuf_repo_id, tuf_artifact_name, tuf_artifact_version, tuf_artifact_kind - ) + PRIMARY KEY (tuf_repo_id, tuf_artifact_id) ); /*******************************************************************/ @@ -4757,7 +4745,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '118.0.0', NULL) + (TRUE, NOW(), NOW(), '119.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/tuf-artifact-key-uuid/up01.sql b/schema/crdb/tuf-artifact-key-uuid/up01.sql new file mode 100644 index 00000000000..d2f8184ee6a --- /dev/null +++ b/schema/crdb/tuf-artifact-key-uuid/up01.sql @@ -0,0 +1,4 @@ +-- At the time of this migration, no racks are able to use the tuf_artifact +-- and tuf_repo_artifact tables outside of testing; even if they were, those +-- artifacts couldn't be durably stored. Drop these tables and recreate them. +DROP TABLE IF EXISTS omicron.public.tuf_artifact diff --git a/schema/crdb/tuf-artifact-key-uuid/up02.sql b/schema/crdb/tuf-artifact-key-uuid/up02.sql new file mode 100644 index 00000000000..e11a78d6948 --- /dev/null +++ b/schema/crdb/tuf-artifact-key-uuid/up02.sql @@ -0,0 +1,2 @@ +-- See up01.sql. +DROP TABLE IF EXISTS omicron.public.tuf_repo_artifact diff --git a/schema/crdb/tuf-artifact-key-uuid/up03.sql b/schema/crdb/tuf-artifact-key-uuid/up03.sql new file mode 100644 index 00000000000..3a0b7e7aafd --- /dev/null +++ b/schema/crdb/tuf-artifact-key-uuid/up03.sql @@ -0,0 +1,10 @@ +CREATE TABLE IF NOT EXISTS omicron.public.tuf_artifact ( + id UUID PRIMARY KEY, + name STRING(63) NOT NULL, + version STRING(63) NOT NULL, + kind STRING(63) NOT NULL, + time_created TIMESTAMPTZ NOT NULL, + sha256 STRING(64) NOT NULL, + artifact_size INT8 NOT NULL, + CONSTRAINT unique_name_version_kind UNIQUE (name, version, kind) +) diff --git a/schema/crdb/tuf-artifact-key-uuid/up04.sql b/schema/crdb/tuf-artifact-key-uuid/up04.sql new file mode 100644 index 00000000000..872a1e52965 --- /dev/null +++ b/schema/crdb/tuf-artifact-key-uuid/up04.sql @@ -0,0 +1,5 @@ +CREATE TABLE IF NOT EXISTS omicron.public.tuf_repo_artifact ( + tuf_repo_id UUID NOT NULL, + tuf_artifact_id UUID NOT NULL, + PRIMARY KEY (tuf_repo_id, tuf_artifact_id) +) diff --git a/uuid-kinds/src/lib.rs b/uuid-kinds/src/lib.rs index e4f65d70392..4983352b2dd 100644 --- a/uuid-kinds/src/lib.rs +++ b/uuid-kinds/src/lib.rs @@ -69,6 +69,7 @@ impl_typed_uuid_kind! { Region => "region", Sled => "sled", SupportBundle => "support_bundle", + TufArtifact => "tuf_artifact", TufRepo => "tuf_repo", Upstairs => "upstairs", UpstairsRepair => "upstairs_repair",