Skip to content
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.

Sign up for GitHub

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

Don't allow downgrading the system target release #7807

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Don't allow downgrading the system target release #7807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
01b1fc3
Don't allow downgrading the system target release
plotnick Mar 17, 2025
e019fda
Merge branch 'main' into no-downgrades
plotnick Mar 22, 2025
c2233b9
Better comments & error message
plotnick Mar 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions nexus/src/external_api/http_entrypoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6442,14 +6442,44 @@ impl NexusExternalApi for NexusExternalApiImpl {
crate::context::op_context_for_external_api(&rqctx).await?;
let params = body.into_inner();
let system_version = params.system_version;

// We don't need a transaction for the following queries because
// (1) the generation numbers provide optimistic concurrency control:
// if another request were to successfully update the target release
// between when we fetch it here and when we try to update it below,
// our update would fail because the next generation number would
// would already be taken; and
// (2) we assume that TUF repo depot records are immutable, i.e.,
// system version X.Y.Z won't designate different repos over time.
let current_target_release =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I like the test, too.

I wonder if this condition needs to be checked transactionally with the update, though? I'm thinking of the time-of-check-to-time-of-use race where say the system is at version 14, the user is trying to update to 15, but someone else comes in and sets it to 16 in the meantime, then we overwrite that with 15. But maybe this is prevented because at L6469 below, we construct the next target release with a generation number of 1 + whatever we just saw (that was version 14), so we'll necessarily fail the database update if someone else comes in in between? If that's the case, could we add a comment here explaining why this race isn't a problem?

Copy link
Contributor Author

@plotnick plotnick Mar 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is indeed the case; documented in c2233b9.

nexus.datastore().target_release_get_current(&opctx).await?;

// Disallow downgrades.
if let views::TargetReleaseSource::SystemVersion { version } = nexus
.datastore()
.target_release_view(&opctx, &current_target_release)
.await?
.release_source
{
if version > system_version {
return Err(HttpError::for_bad_request(
None,
format!(
"The requested target system release ({system_version}) \
is older than the current target system release ({version}). \
This is not supported."
),
));
}
}

// Fetch the TUF repo metadata and update the target release.
let tuf_repo_id = nexus
.datastore()
.update_tuf_repo_get(&opctx, system_version.into())
.await?
.repo
.id;
let current_target_release =
nexus.datastore().target_release_get_current(&opctx).await?;
let next_target_release =
nexus_db_model::TargetRelease::new_system_version(
&current_target_release,
Expand All @@ -6459,6 +6489,7 @@ impl NexusExternalApi for NexusExternalApiImpl {
.datastore()
.target_release_insert(&opctx, next_target_release)
.await?;

Ok(HttpResponseCreated(
nexus
.datastore()
Expand Down
31 changes: 17 additions & 14 deletions nexus/tests/integration_tests/target_release.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

//! Get/set the target release via the external API.

use anyhow::Result;
use camino::Utf8Path;
use camino_tempfile::Utf8TempDir;
use chrono::Utc;
Expand All @@ -23,7 +24,7 @@ use omicron_sled_agent::sim;
use semver::Version;

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn get_set_target_release() -> anyhow::Result<()> {
async fn get_set_target_release() -> Result<()> {
let mut config = load_test_config();
config.pkg.updates = Some(UpdatesConfig {
// XXX: This is currently not used by the update system, but
Expand Down Expand Up @@ -87,11 +88,11 @@ async fn get_set_target_release() -> anyhow::Result<()> {

assert_eq!(
system_version,
upload_tuf_repo(client, &path).await.recorded.repo.system_version
upload_tuf_repo(client, &path).await?.recorded.repo.system_version
);

let target_release =
set_target_release(client, system_version.clone()).await;
set_target_release(client, system_version.clone()).await?;
let after = Utc::now();
assert_eq!(target_release.generation, 2);
assert!(target_release.time_requested >= before);
Expand All @@ -102,7 +103,7 @@ async fn get_set_target_release() -> anyhow::Result<()> {
);
}

// Add a repo with non-semver versions.
// Adding a repo with non-semver artifact versions should be ok, too.
{
let before = Utc::now();
let system_version = Version::new(2, 0, 0);
Expand All @@ -121,11 +122,11 @@ async fn get_set_target_release() -> anyhow::Result<()> {

assert_eq!(
system_version,
upload_tuf_repo(client, &path).await.recorded.repo.system_version
upload_tuf_repo(client, &path).await?.recorded.repo.system_version
);

let target_release =
set_target_release(client, system_version.clone()).await;
set_target_release(client, system_version.clone()).await?;
let after = Utc::now();
assert_eq!(target_release.generation, 3);
assert!(target_release.time_requested >= before);
Expand All @@ -136,6 +137,12 @@ async fn get_set_target_release() -> anyhow::Result<()> {
);
}

// Attempting to downgrade to an earlier system version (2.0.0 → 1.0.0)
// should not be allowed.
set_target_release(client, Version::new(1, 0, 0))
.await
.expect_err("shouldn't be able to downgrade system");

ctx.teardown().await;
logctx.cleanup_successful();
Ok(())
Expand All @@ -144,7 +151,7 @@ async fn get_set_target_release() -> anyhow::Result<()> {
async fn upload_tuf_repo(
client: &ClientTestContext,
path: &Utf8Path,
) -> TufRepoInsertResponse {
) -> Result<TufRepoInsertResponse> {
NexusRequest::new(
RequestBuilder::new(
client,
Expand All @@ -157,15 +164,13 @@ async fn upload_tuf_repo(
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap()
.parsed_body::<TufRepoInsertResponse>()
.unwrap()
.map(|response| response.parsed_body().unwrap())
}

async fn set_target_release(
client: &ClientTestContext,
system_version: Version,
) -> TargetRelease {
) -> Result<TargetRelease> {
NexusRequest::new(
RequestBuilder::new(
client,
Expand All @@ -178,7 +183,5 @@ async fn set_target_release(
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap()
.parsed_body()
.unwrap()
.map(|response| response.parsed_body().unwrap())
}
Loading