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 1 commit
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
20 changes: 18 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,30 @@ impl NexusExternalApi for NexusExternalApiImpl {
crate::context::op_context_for_external_api(&rqctx).await?;
let params = body.into_inner();
let system_version = params.system_version;

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?;
let view = nexus
.datastore()
.target_release_view(&opctx, &current_target_release)
.await?;
if let views::TargetReleaseSource::SystemVersion { version } =
view.release_source
{
if version > system_version {
return Err(HttpError::for_bad_request(
None,
"can't downgrade system".into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"can't downgrade system".into(),
"The requested target system release ({system_version}) is older than the current target system release ({version}). This is not supported.".into()

I'm imagining an end user might see this message and want to make sure it's clear what's going on and what they've done wrong.

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.

Indeed, thanks! Also added in c2233b9.

));
}
}

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 Down
53 changes: 52 additions & 1 deletion nexus/tests/integration_tests/target_release.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ async fn get_set_target_release() -> anyhow::Result<()> {
.expect("can't parse tufaceous args")
.exec(&logctx.log)
.await
.expect("can't assemble TUF repo");
.expect("can't assemble version 1 TUF repo");

assert_eq!(
system_version,
Expand Down Expand Up @@ -130,6 +130,57 @@ async fn get_set_target_release() -> anyhow::Result<()> {
TargetReleaseSource::SystemVersion { version: system_version },
);

// Attempting to downgrade to an earlier system version (1.0.0 → 0.0.0) should fail.
let system_version = Version::new(0, 0, 0);
tufaceous::Args::try_parse_from([
"tufaceous",
"assemble",
"../update-common/manifests/fake0.toml",
path.as_str(),
])
.expect("can't parse tufaceous args")
.exec(&logctx.log)
.await
.expect("can't assemble version 0 TUF repo");

assert_eq!(
system_version,
NexusRequest::new(
RequestBuilder::new(
client,
http::Method::PUT,
"/v1/system/update/repository?file_name=/tmp/foo.zip",
)
.body_file(Some(&path))
.expect_status(Some(http::StatusCode::OK)),
)
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.unwrap()
.parsed_body::<TufRepoInsertResponse>()
.unwrap()
.recorded
.repo
.system_version
);

NexusRequest::new(
RequestBuilder::new(
client,
Method::PUT,
"/v1/system/update/target-release",
)
.body(Some(&SetTargetReleaseParams {
system_version: system_version.clone(),
}))
.expect_status(Some(StatusCode::CREATED)),
)
.authn_as(AuthnMode::PrivilegedUser)
.execute()
.await
.expect_err("shouldn't be able to downgrade");

ctx.teardown().await;
logctx.cleanup_successful();
Ok(())
Expand Down
87 changes: 87 additions & 0 deletions update-common/manifests/fake0.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# This is an artifact manifest that generates fake entries for all components.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about calling this fake-old.toml and adding a comment here that the only difference from fake.toml is the version numbers of everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#7832 added a (non-semver-containing) 2.0.0 and made the test use that, so e019fda just tries to revert back to 1.0.0 and removes this file entirely.

# This is completely non-functional and is only useful for testing archive
# extraction in other parts of the repository.

system_version = "0.0.0"

[[artifact.gimlet_sp]]
name = "fake-gimlet-sp"
version = "0.0.0"
source = { kind = "fake", size = "1MiB" }

[[artifact.gimlet_rot]]
name = "fake-gimlet-rot"
version = "0.0.0"
[artifact.gimlet_rot.source]
kind = "composite-rot"
archive_a = { kind = "fake", size = "512KiB" }
archive_b = { kind = "fake", size = "512KiB" }

[[artifact.host]]
name = "fake-host"
version = "0.0.0"
[artifact.host.source]
kind = "composite-host"
phase_1 = { kind = "fake", size = "512KiB" }
phase_2 = { kind = "fake", size = "1MiB" }

[[artifact.trampoline]]
name = "fake-trampoline"
version = "0.0.0"
[artifact.trampoline.source]
kind = "composite-host"
phase_1 = { kind = "fake", size = "512KiB" }
phase_2 = { kind = "fake", size = "1MiB" }

[[artifact.control_plane]]
name = "fake-control-plane"
version = "0.0.0"
[artifact.control_plane.source]
kind = "composite-control-plane"
zones = [
{ kind = "fake", name = "zone1", size = "1MiB" },
{ kind = "fake", name = "zone2", size = "1MiB" },
]

[[artifact.psc_sp]]
name = "fake-psc-sp"
version = "0.0.0"
source = { kind = "fake", size = "1MiB" }

[[artifact.psc_rot]]
name = "fake-psc-rot"
version = "0.0.0"
[artifact.psc_rot.source]
kind = "composite-rot"
archive_a = { kind = "fake", size = "512KiB" }
archive_b = { kind = "fake", size = "512KiB" }

[[artifact.switch_sp]]
name = "fake-switch-sp"
version = "0.0.0"
source = { kind = "fake", size = "1MiB" }

[[artifact.switch_rot]]
name = "fake-switch-rot"
version = "0.0.0"
[artifact.switch_rot.source]
kind = "composite-rot"
archive_a = { kind = "fake", size = "512KiB" }
archive_b = { kind = "fake", size = "512KiB" }

[[artifact.gimlet_rot_bootloader]]
name = "fake-gimlet-rot-bootloader"
version = "0.0.0"
source = { kind = "fake", size = "1MiB" }

[[artifact.psc_rot_bootloader]]
name = "fake-psc-rot-bootloader"
version = "0.0.0"
source = { kind = "fake", size = "1MiB" }

[[artifact.switch_rot_bootloader]]
name = "fake-switch-rot-bootloader"
version = "0.0.0"
source = { kind = "fake", size = "1MiB" }


Loading