Allow different versions for differently signed RoT images #5580

labbott · 2024-04-19T19:30:19Z

Currently we restrict all RoT archives for {Gimlet, PSC, Sidecar} to be the same version. This restricts us to having staging and production images to be the same version. What we actually want is all archives for a particular kind signed with the same key to have the same version.

sunshowers

Had a quick look, generally looks good!

sunshowers · 2024-04-19T21:02:51Z

update-common/src/artifacts/update_plan.rs

+        // version at builder time (builder time is not async)
+        let image_a = rot_a_data
+            .reader_stream()
+            .and_then(|stream| async {


Could this use a .await rather than a future combinator? In general, .await is preferable -- it's easier to understand, particularly because .and_then exists both on futures and on results. (There are exceptions where future combinators make sense, but this doesn't seem to be one of them.)

I was going off of what wicketd was doing (https://github.com/oxidecomputer/omicron/blob/main/wicketd/src/update_tracker.rs#L1679-L1696) but I'm fine with changing it for readability.

sunshowers · 2024-04-19T21:03:23Z

update-common/src/artifacts/update_plan.rs

+                tokio_util::io::StreamReader::new(stream)
+                    .read_to_end(&mut buf)
+                    .await
+                    .unwrap();


These are extracted as files on disk, so I believe they can produce errors. Even if they can't, it would be good to replace the unwrap() with an expect("justification").

wicketd mentions this can be an I/O error https://github.com/oxidecomputer/omicron/blob/main/wicketd/src/update_tracker.rs#L1690 . I'll match that here.

sunshowers · 2024-04-19T21:13:24Z

update-common/src/artifacts/update_plan.rs

+            .and_then(|stream| async {
+                let mut buf = Vec::with_capacity(rot_b_data.file_size());
+                tokio_util::io::StreamReader::new(stream)
+                    .read_to_end(&mut buf)
+                    .await
+                    .unwrap();
+                Ok(buf)
+            })
+            .await
+            .unwrap();


Same comments about both using .await and bubbling up errors here.

sunshowers · 2024-04-19T21:14:10Z

update-common/src/artifacts/update_plan.rs

+        let archive_a = make_fake_rot_image(sign, board);
+        let archive_b = make_fake_rot_image(sign, board);


haha I was wondering when this would become less random, thanks for doing this :)

labbott · 2024-04-30T14:51:02Z

update-common/src/errors.rs

+    #[error("error creating reader stream")]
+    CreateReaderStream(#[source] anyhow::Error),


Not sure the opinion on adding new errors here

Adding new errors (when needed) is good 👍

jgallagher · 2024-05-16T17:18:44Z

update-common/src/errors.rs

+    #[error("error creating reader stream")]
+    CreateReaderStream(#[source] anyhow::Error),


Adding new errors (when needed) is good 👍

jgallagher · 2024-05-16T17:22:11Z

update-common/src/artifacts/update_plan.rs

+        tokio_util::io::StreamReader::new(image_a_stream)
+            .read_to_end(&mut image_a)
+            .await
+            .expect("failed to read RoT image a");


I don't think we can .expect() these - reader_stream() returns streams that are backed by tokio::fs::Files, which can fail at runtime. Probably need something like a

RepositoryError::ReadExtractedArchive { artifact: ArtifactHashId, #[source] error: std::io::Error, }

variant to use here?

update-common/src/artifacts/update_plan.rs

jgallagher · 2024-05-16T17:28:22Z

update-common/src/artifacts/update_plan.rs

+            return Err(RepositoryError::ReadHubrisCabooseBoard { id, error });
+        }
+    };
+    let sign = match std::str::from_utf8(sign) {


Do we use the fact that we're storing sign as a String anywhere? I'm wondering if this conversion is necessary at all, or if we could store (ArtifactId, Vec<u8>) instead.

sunshowers

General logic looks great! Just a few comments + co-signing John's comments.

sunshowers · 2024-05-16T17:37:20Z

update-common/src/artifacts/update_plan.rs

+        tokio_util::io::StreamReader::new(image_b_stream)
+            .read_to_end(&mut image_b)
+            .await
+            .expect("failed to read RoT image b");


Yeah same here -- we should produce an error.

sunshowers · 2024-05-16T18:16:52Z

update-common/src/artifacts/update_plan.rs

+        let logctx = test_setup_log("test_multi_rot_version");
+
+        let mut plan_builder =
+            UpdatePlanBuilder::new("0.0.0".parse().unwrap(), &logctx.log)


Can this use VERSION_0 as well?

sunshowers · 2024-05-16T18:18:24Z

update-common/src/artifacts/update_plan.rs

@@ -112,6 +113,7 @@ pub struct UpdatePlanBuilder<'a> {
    // The by_id and by_hash maps, and metadata, used in `ArtifactsWithPlan`.


This comment should be updated.

sunshowers · 2024-05-16T18:20:26Z

update-common/src/artifacts/update_plan.rs

@@ -112,6 +113,7 @@ pub struct UpdatePlanBuilder<'a> {
    // The by_id and by_hash maps, and metadata, used in `ArtifactsWithPlan`.
    by_id: BTreeMap<ArtifactId, Vec<ArtifactHashId>>,
    by_hash: HashMap<ArtifactHashId, ExtractedArtifactDataHandle>,
+    by_sign: HashMap<(KnownArtifactKind, String), Vec<ArtifactId>>,


What does the key stand for?

What do you think about turning this into a struct with named fields?

by_sign seems to only contain Hubris artifacts, is that right? If so could this be documented here?

I'll clarify this with some rework and comments

sunshowers · 2024-05-16T18:23:11Z

update-common/src/artifacts/update_plan.rs

+            return Err(RepositoryError::ReadHubrisCaboose { id, error });
+        }
+    };
+    let sign = match caboose.sign() {


hmm, what kind of values does sign have? Is it one of a small fixed set of values, or is it arbitrary?

is it worth wrapping the sign in a newtype (probably in the caboose API I guess)?

It's a &[u8] (https://github.com/oxidecomputer/hubtools/blob/main/hubtools/src/caboose.rs#L47-L49) although as @jgallagher noted below the extra step of conversion to a string is probably unnecessary except for error cases.

Currently we restrict all RoT archives for {Gimlet, PSC, Sidecar} to be the same version. This restricts us to having staging and production images to be the same version. What we actually want is all archives for a particular kind _signed with the same key_ to have the same version.

jgallagher

LGTM 👍

sunshowers

agreed, looks great!

…5580)" This reverts commit 94ef8e5. It turns out there are several places that are still making assumptions about versions. Until we've identified them all just revert this change to avoid any unexpected surprises.

labbott marked this pull request as draft April 19, 2024 19:31

labbott force-pushed the staging_and_prod_diff branch from b557ccf to 5cd0db8 Compare April 19, 2024 20:01

sunshowers reviewed Apr 19, 2024

View reviewed changes

labbott force-pushed the staging_and_prod_diff branch from 5cd0db8 to bd58a59 Compare April 30, 2024 14:47

labbott commented Apr 30, 2024

View reviewed changes

labbott marked this pull request as ready for review May 13, 2024 16:01

jgallagher reviewed May 16, 2024

View reviewed changes

sunshowers reviewed May 16, 2024

View reviewed changes

labbott force-pushed the staging_and_prod_diff branch 3 times, most recently from 24de5b0 to e7c16a0 Compare May 20, 2024 14:39

Address comments

463aa0d

labbott force-pushed the staging_and_prod_diff branch from e7c16a0 to 463aa0d Compare May 20, 2024 15:13

jgallagher approved these changes May 20, 2024

View reviewed changes

sunshowers approved these changes May 20, 2024

View reviewed changes

labbott merged commit 94ef8e5 into main May 20, 2024
18 checks passed

labbott deleted the staging_and_prod_diff branch May 20, 2024 20:27

labbott mentioned this pull request Sep 19, 2024

Allow multiple RoT versions in the same repository #6586

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow different versions for differently signed RoT images #5580

Allow different versions for differently signed RoT images #5580

labbott commented Apr 19, 2024

sunshowers left a comment

sunshowers Apr 19, 2024

labbott Apr 30, 2024

sunshowers Apr 19, 2024

labbott Apr 30, 2024

sunshowers Apr 19, 2024

sunshowers Apr 19, 2024

labbott Apr 30, 2024

jgallagher May 16, 2024

jgallagher May 16, 2024

jgallagher May 16, 2024

jgallagher May 16, 2024

sunshowers left a comment

sunshowers May 16, 2024

sunshowers May 16, 2024

sunshowers May 16, 2024

sunshowers May 16, 2024

labbott May 20, 2024

sunshowers May 16, 2024

labbott May 20, 2024

jgallagher left a comment

sunshowers left a comment

		let archive_a = make_fake_rot_image(sign, board);
		let archive_b = make_fake_rot_image(sign, board);

		#[error("error creating reader stream")]
		CreateReaderStream(#[source] anyhow::Error),

		@@ -112,6 +113,7 @@ pub struct UpdatePlanBuilder<'a> {
		// The by_id and by_hash maps, and metadata, used in `ArtifactsWithPlan`.

Allow different versions for differently signed RoT images #5580

Allow different versions for differently signed RoT images #5580

Conversation

labbott commented Apr 19, 2024

sunshowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunshowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgallagher left a comment

Choose a reason for hiding this comment

sunshowers left a comment

Choose a reason for hiding this comment