-
Notifications
You must be signed in to change notification settings - Fork 62
set_target_release: Fail if setting target release to a pruned TUF repo #9117
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,9 @@ use crate::db::model::{ | |
| use async_bb8_diesel::AsyncRunQueryDsl as _; | ||
| use diesel::insert_into; | ||
| use diesel::prelude::*; | ||
| use diesel::sql_types; | ||
| use nexus_db_errors::{ErrorHandler, public_error_from_diesel}; | ||
| use nexus_db_schema::enums::TargetReleaseSourceEnum; | ||
| use nexus_db_schema::schema::target_release::dsl; | ||
| use nexus_types::external_api::views; | ||
| use omicron_common::api::external::{CreateResult, Error, LookupResult}; | ||
|
|
@@ -77,12 +79,104 @@ impl DataStore { | |
| .authorize(authz::Action::Modify, &authz::TARGET_RELEASE_CONFIG) | ||
| .await?; | ||
| let conn = self.pool_connection_authorized(opctx).await?; | ||
| insert_into(dsl::target_release) | ||
| .values(target_release) | ||
| .returning(TargetRelease::as_returning()) | ||
| .get_result_async(&*conn) | ||
| .await | ||
| .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) | ||
|
|
||
| // If we have a TUF repo ID, we need a more complex query to confirm | ||
| // (transactionally) that it isn't the pruned as we make it the target | ||
| // release. | ||
| if let Some(tuf_repo_id) = target_release.tuf_repo_id { | ||
| let selection = { | ||
| use nexus_db_schema::schema::tuf_repo::dsl as repo_dsl; | ||
|
|
||
| // This statement is just here to force a compilation error if | ||
| // the set of columns in `target_release` changes, because that | ||
| // will affect the correctness of the query below. | ||
| // | ||
| // If you're here because of a compile error, you might be | ||
| // changing the `target_release` table. Update the statement | ||
| // below to match! | ||
| let _: ( | ||
| dsl::generation, | ||
| dsl::time_requested, | ||
| dsl::release_source, | ||
| dsl::tuf_repo_id, | ||
| ) = dsl::target_release::all_columns(); | ||
|
|
||
| // What we want to write here is a query that confirms | ||
| // `tuf_repo_id` is not pruned and avoids performing an insert | ||
| // otherwise. We'll do that via an `INSERT SELECT ...` where the | ||
| // `SELECT` is: | ||
| // | ||
| // ``` | ||
| // SELECT $target_release WHERE EXISTS ( | ||
| // SELECT 1 FROM tuf_repo WHERE | ||
| // id = $tuf_repo_id | ||
| // AND time_pruned IS NULL | ||
| // ) | ||
| // ``` | ||
| // | ||
| // but with a couple of diesel quirks: | ||
| // | ||
| // 1. We can't splat the `$target_release` value directly into a | ||
| // SELECT, so we select each of its columns individually. See | ||
| // the above check that the columns of this table haven't | ||
| // changed. | ||
| // 2. We don't bother getting it to `SELECT 1 ...` in the | ||
| // subquery. diesel defaults to `SELECT * ...` there instead, | ||
| // but that should be fine since it's inside a `WHERE | ||
| // EXISTS`. | ||
| diesel::select(( | ||
| target_release.generation.into_sql::<sql_types::Int8>(), | ||
| target_release | ||
| .time_requested | ||
| .into_sql::<sql_types::Timestamptz>(), | ||
| target_release | ||
| .release_source | ||
| .into_sql::<TargetReleaseSourceEnum>(), | ||
| tuf_repo_id | ||
| .into_sql::<sql_types::Nullable<sql_types::Uuid>>(), | ||
| )) | ||
| .filter(diesel::dsl::exists( | ||
| repo_dsl::tuf_repo | ||
| .filter(repo_dsl::id.eq(tuf_repo_id)) | ||
| .filter(repo_dsl::time_pruned.is_null()), | ||
| )) | ||
| }; | ||
|
|
||
| // Attempt the insert; use `.optional()` so we can attach a custom | ||
| // error message if we get back no rows. | ||
| let result = insert_into(dsl::target_release) | ||
| .values(selection) | ||
| .returning(TargetRelease::as_returning()) | ||
| .get_result_async(&*conn) | ||
| .await | ||
| .optional() | ||
| .map_err(|e| { | ||
| public_error_from_diesel(e, ErrorHandler::Server) | ||
| })?; | ||
|
|
||
| match result { | ||
| Some(target_release) => { | ||
| // Insertion succeeded and returned the newly-inserted | ||
| // target release. | ||
| Ok(target_release) | ||
| } | ||
| None => { | ||
| // Insertion succeeded but didn't return any rows: we tried | ||
| // to insert a target release for a pruned repo. | ||
| Err(Error::invalid_request(format!( | ||
| "cannot make TUF repo {tuf_repo_id} the \ | ||
| target release: it has been pruned from the system" | ||
smklein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ))) | ||
| } | ||
| } | ||
| } else { | ||
| insert_into(dsl::target_release) | ||
| .values(target_release) | ||
| .returning(TargetRelease::as_returning()) | ||
| .get_result_async(&*conn) | ||
| .await | ||
| .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) | ||
| } | ||
| } | ||
|
|
||
| /// Convert a model-level target release to an external view. | ||
|
|
@@ -135,13 +229,56 @@ mod test { | |
| use crate::db::model::{Generation, TargetReleaseSource}; | ||
| use crate::db::pub_test_utils::TestDatabase; | ||
| use chrono::{TimeDelta, Utc}; | ||
| use nexus_db_model::TufRepo; | ||
| use omicron_common::api::external::{ | ||
| TufArtifactMeta, TufRepoDescription, TufRepoMeta, | ||
| }; | ||
| use omicron_common::update::ArtifactId; | ||
| use omicron_test_utils::dev; | ||
| use semver::Version; | ||
| use tufaceous_artifact::{ArtifactKind, ArtifactVersion}; | ||
| use sha2::Digest; | ||
| use sha2::Sha256; | ||
| use slog_error_chain::InlineErrorChain; | ||
| use tufaceous_artifact::{ArtifactHash, ArtifactKind, ArtifactVersion}; | ||
|
|
||
| async fn insert_tuf_repo( | ||
| opctx: &OpContext, | ||
| datastore: &DataStore, | ||
| version: &Version, | ||
| ) -> TufRepo { | ||
| let artifact_version = ArtifactVersion::new(version.to_string()) | ||
| .expect("version is valid for artifacts too"); | ||
| let hash = ArtifactHash(Sha256::digest(version.to_string()).into()); | ||
|
|
||
| datastore | ||
| .tuf_repo_insert( | ||
| opctx, | ||
| &TufRepoDescription { | ||
| repo: TufRepoMeta { | ||
| hash, | ||
| targets_role_version: 0, | ||
| valid_until: Utc::now(), | ||
| system_version: version.clone(), | ||
| file_name: String::new(), | ||
| }, | ||
| artifacts: vec![TufArtifactMeta { | ||
| id: ArtifactId { | ||
| name: String::new(), | ||
| version: artifact_version, | ||
| kind: ArtifactKind::from_static("empty"), | ||
| }, | ||
| hash, | ||
| size: 0, | ||
| board: None, | ||
| sign: None, | ||
| }], | ||
| }, | ||
| ) | ||
| .await | ||
| .expect("inserted TUF repo description") | ||
| .recorded | ||
| .repo | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn target_release_datastore() { | ||
|
|
@@ -203,40 +340,7 @@ mod test { | |
|
|
||
| // Now add a new TUF repo and use it as the source. | ||
| let version = Version::new(0, 0, 1); | ||
| const ARTIFACT_VERSION: ArtifactVersion = | ||
| ArtifactVersion::new_const("0.0.1"); | ||
| let hash = | ||
| "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" | ||
| .parse() | ||
| .expect("SHA256('')"); | ||
| let repo = datastore | ||
| .tuf_repo_insert( | ||
| opctx, | ||
| &TufRepoDescription { | ||
| repo: TufRepoMeta { | ||
| hash, | ||
| targets_role_version: 0, | ||
| valid_until: Utc::now(), | ||
| system_version: version.clone(), | ||
| file_name: String::new(), | ||
| }, | ||
| artifacts: vec![TufArtifactMeta { | ||
| id: ArtifactId { | ||
| name: String::new(), | ||
| version: ARTIFACT_VERSION, | ||
| kind: ArtifactKind::from_static("empty"), | ||
| }, | ||
| hash, | ||
| size: 0, | ||
| board: None, | ||
| sign: None, | ||
| }], | ||
| }, | ||
| ) | ||
| .await | ||
| .unwrap() | ||
| .recorded | ||
| .repo; | ||
| let repo = insert_tuf_repo(opctx, datastore, &version).await; | ||
| assert_eq!(repo.system_version, version.into()); | ||
| let tuf_repo_id = repo.id; | ||
|
|
||
|
|
@@ -262,4 +366,85 @@ mod test { | |
| db.terminate().await; | ||
| logctx.cleanup_successful(); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn reject_target_release_if_repo_pruned() { | ||
| let logctx = | ||
| dev::test_setup_log("reject_target_release_if_repo_pruned"); | ||
| let db = TestDatabase::new_with_datastore(&logctx.log).await; | ||
| let (opctx, datastore) = (db.opctx(), db.datastore()); | ||
|
|
||
| // Insert two TUF repos. | ||
| let repo1 = | ||
| insert_tuf_repo(opctx, datastore, &Version::new(0, 0, 1)).await; | ||
| let repo2 = | ||
| insert_tuf_repo(opctx, datastore, &Version::new(0, 0, 2)).await; | ||
|
|
||
| // Manually prune the second one. | ||
| { | ||
| use nexus_db_schema::schema::tuf_repo::dsl; | ||
|
|
||
| let conn = datastore | ||
| .pool_connection_for_tests() | ||
| .await | ||
| .expect("got connection"); | ||
| let n = diesel::update(dsl::tuf_repo) | ||
| .filter(dsl::id.eq(repo2.id)) | ||
| .set(dsl::time_pruned.eq(Some(Utc::now()))) | ||
| .execute_async(&*conn) | ||
| .await | ||
| .expect("pruned repo2"); | ||
| assert_eq!(n, 1, "should have only pruned 1 repo"); | ||
| } | ||
|
|
||
| // There should always be an initial target release. | ||
| let target_release = datastore | ||
| .target_release_get_current(opctx) | ||
| .await | ||
| .expect("should be a target release"); | ||
|
|
||
| // Make repo1 the target release. This should succeed. | ||
| let target_release = datastore | ||
| .target_release_insert( | ||
| opctx, | ||
| TargetRelease::new_system_version(&target_release, repo1.id), | ||
| ) | ||
| .await | ||
| .expect("made repo1 the target release"); | ||
| assert_eq!(target_release.generation, Generation(2.into())); | ||
|
|
||
| // Attempting to make repo1 the target release again should fail with a | ||
| // reasonable error message (we need a higher generation). | ||
| let err = datastore | ||
| .target_release_insert(opctx, target_release.clone()) | ||
| .await | ||
| .expect_err("making repo1 the target release again should fail"); | ||
| let err = InlineErrorChain::new(&err).to_string(); | ||
| assert!( | ||
| err.contains("violates unique constraint"), | ||
| "unexpected error: {err}" | ||
| ); | ||
|
|
||
| // Attempt to make repo2 the target release. This should fail with a | ||
| // reasonable error message (it's been pruned). | ||
| let err = datastore | ||
| .target_release_insert( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would it still claim to be pruned? |
||
| opctx, | ||
| TargetRelease::new_system_version(&target_release, repo2.id), | ||
| ) | ||
| .await | ||
| .expect_err("making repo2 the target release should fail"); | ||
| let err = InlineErrorChain::new(&err).to_string(); | ||
| assert!( | ||
| err.contains("cannot make TUF repo") | ||
| && err.contains(&repo2.id.to_string()) | ||
| && err.contains("target release") | ||
| && err.contains("pruned"), | ||
| "unexpected error: {err}" | ||
| ); | ||
|
|
||
| // Clean up. | ||
| db.terminate().await; | ||
| logctx.cleanup_successful(); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the same as it was before, but does this do something reasonable if the INSERT fails for some other reason (like the generation number was already inserted)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I added a test for exactly that case, which still causes the insertion to fail with the same error it did before. The
.optional()here only lets us handle "insert succeeded but returned 0 rows", which I believe can only be caused by the repo being pruned (the error we expect) or we got passed a UUID that isn't in thetuf_repotable at all (which would be a mistake somewhere else, and would only result in us claiming that the nonexistent repo was pruned instead of more clearly saying it doesn't exist).