From fd5dcb3979ec3b566df091977ecca14bd7074053 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Fri, 6 Sep 2024 09:17:58 +0200 Subject: [PATCH 1/6] refactoring runner --- bin/cargo-deny-checks.sh | 2 +- rs/cli/src/artifact_downloader.rs | 133 +++++++++++++++ rs/cli/src/ic_admin.rs | 262 +----------------------------- rs/cli/src/lib.rs | 1 + rs/cli/src/main.rs | 1 + rs/cli/src/runner.rs | 128 ++++++++++++++- 6 files changed, 265 insertions(+), 262 deletions(-) create mode 100644 rs/cli/src/artifact_downloader.rs diff --git a/bin/cargo-deny-checks.sh b/bin/cargo-deny-checks.sh index 1046ebe4c..544b97cc0 100755 --- a/bin/cargo-deny-checks.sh +++ b/bin/cargo-deny-checks.sh @@ -1,4 +1,4 @@ #!/usr/bin/env bash command -v cargo-deny >/dev/null || echo "'cargo-deny' not found. Please install it by running 'cargo install cargo-deny'" -cargo deny check +cargo deny check --warn unmaintained diff --git a/rs/cli/src/artifact_downloader.rs b/rs/cli/src/artifact_downloader.rs new file mode 100644 index 000000000..d4ca7149a --- /dev/null +++ b/rs/cli/src/artifact_downloader.rs @@ -0,0 +1,133 @@ +#![allow(async_fn_in_trait)] +use std::{fs::File, io::Write, path::Path}; + +use futures::{stream, StreamExt}; +use ic_management_types::Artifact; +use itertools::Itertools; +use log::{info, warn}; +use mockall::automock; +use reqwest::StatusCode; +use sha2::{Digest, Sha256}; + +#[automock] +pub trait ArtifactDownloader { + fn get_s3_cdn_image_url<'a>(&'a self, version: &'a str, s3_subdir: &'a str) -> String { + format!( + "https://download.dfinity.systems/ic/{}/{}/update-img/update-img.tar.gz", + version, s3_subdir + ) + } + + fn get_r2_cdn_image_url<'a>(&'a self, version: &'a str, s3_subdir: &'a str) -> String { + format!( + "https://download.dfinity.network/ic/{}/{}/update-img/update-img.tar.gz", + version, s3_subdir + ) + } + + async fn download_file_and_get_sha256<'a>(&'a self, download_url: &'a str) -> anyhow::Result { + let url = url::Url::parse(download_url)?; + let subdir = format!("{}{}", url.domain().expect("url.domain() is None"), url.path().to_owned()); + // replace special characters in subdir with _ + let subdir = subdir.replace(|c: char| !c.is_ascii_alphanumeric(), "_"); + let download_dir = format!("{}/tmp/ic/{}", dirs::home_dir().expect("home_dir is not set").as_path().display(), subdir); + let download_dir = Path::new(&download_dir); + + std::fs::create_dir_all(download_dir).unwrap_or_else(|_| panic!("create_dir_all failed for {}", download_dir.display())); + + let download_image = format!("{}/update-img.tar.gz", download_dir.to_str().unwrap()); + let download_image = Path::new(&download_image); + + let response = reqwest::get(download_url).await?; + + if response.status() != StatusCode::RANGE_NOT_SATISFIABLE && !response.status().is_success() { + return Err(anyhow::anyhow!( + "Download failed with http_code {} for {}", + response.status(), + download_url + )); + } + info!("Download {} succeeded {}", download_url, response.status()); + + let mut file = match File::create(download_image) { + Ok(file) => file, + Err(err) => return Err(anyhow::anyhow!("Couldn't create a file: {}", err)), + }; + + let content = response.bytes().await?; + file.write_all(&content)?; + + let mut hasher = Sha256::new(); + hasher.update(&content); + let hash = hasher.finalize(); + let stringified_hash = hash[..].iter().map(|byte| format!("{:01$x?}", byte, 2)).collect::>().join(""); + info!("File saved at {} has sha256 {}", download_image.display(), stringified_hash); + Ok(stringified_hash) + } + + async fn download_images_and_validate_sha256<'a>( + &'a self, + image: &'a Artifact, + version: &'a str, + ignore_missing_urls: bool, + ) -> anyhow::Result<(Vec, String)> { + let update_urls = vec![ + self.get_s3_cdn_image_url(version, &image.s3_folder()), + self.get_r2_cdn_image_url(version, &image.s3_folder()), + ]; + + // Download images, verify them and compare the SHA256 + let hash_and_valid_urls: Vec<(String, &String)> = stream::iter(&update_urls) + .filter_map(|update_url| async move { + match self.download_file_and_get_sha256(update_url).await { + Ok(hash) => { + info!("SHA256 of {}: {}", update_url, hash); + Some((hash, update_url)) + } + Err(err) => { + warn!("Error downloading {}: {}", update_url, err); + None + } + } + }) + .collect() + .await; + let hashes_unique = hash_and_valid_urls.iter().map(|(h, _)| h.clone()).unique().collect::>(); + let expected_hash: String = match hashes_unique.len() { + 0 => { + return Err(anyhow::anyhow!( + "Unable to download the update image from none of the following URLs: {}", + update_urls.join(", ") + )) + } + 1 => { + let hash = hashes_unique.into_iter().next().unwrap(); + info!("SHA256 of all download images is: {}", hash); + hash + } + _ => { + return Err(anyhow::anyhow!( + "Update images do not have the same hash: {:?}", + hash_and_valid_urls.iter().map(|(h, u)| format!("{} {}", h, u)).join("\n") + )) + } + }; + let update_urls = hash_and_valid_urls.into_iter().map(|(_, u)| u.clone()).collect::>(); + + if update_urls.is_empty() { + return Err(anyhow::anyhow!( + "Unable to download the update image from none of the following URLs: {}", + update_urls.join(", ") + )); + } else if update_urls.len() == 1 { + if ignore_missing_urls { + warn!("Only 1 update image is available. At least 2 should be present in the proposal"); + } else { + return Err(anyhow::anyhow!( + "Only 1 update image is available. At least 2 should be present in the proposal" + )); + } + } + Ok((update_urls, expected_hash)) + } +} diff --git a/rs/cli/src/ic_admin.rs b/rs/cli/src/ic_admin.rs index fb2bf6380..d7a7b59c0 100644 --- a/rs/cli/src/ic_admin.rs +++ b/rs/cli/src/ic_admin.rs @@ -4,18 +4,13 @@ use colored::Colorize; use dialoguer::Confirm; use flate2::read::GzDecoder; use futures::future::BoxFuture; -use futures::stream::{self, StreamExt}; use ic_base_types::PrincipalId; use ic_management_types::{Artifact, Network}; -use itertools::Itertools; -use log::{error, info, warn}; +use log::{error, info}; use mockall::automock; use regex::Regex; -use reqwest::StatusCode; -use sha2::{Digest, Sha256}; use shlex::try_quote; -use std::fs::File; -use std::io::{Read, Write}; +use std::io::Read; use std::os::unix::fs::PermissionsExt; use std::path::PathBuf; use std::process::Stdio; @@ -25,40 +20,6 @@ use strum::Display; const MAX_SUMMARY_CHAR_COUNT: usize = 29000; -#[derive(Clone)] -pub struct UpdateVersion { - pub release_artifact: Artifact, - pub version: String, - pub title: String, - pub summary: String, - pub update_urls: Vec, - pub stringified_hash: String, - pub versions_to_retire: Option>, -} - -impl UpdateVersion { - pub fn get_update_cmd_args(&self) -> Vec { - [ - [ - vec![ - "--replica-version-to-elect".to_string(), - self.version.to_string(), - "--release-package-sha256-hex".to_string(), - self.stringified_hash.to_string(), - "--release-package-urls".to_string(), - ], - self.update_urls.clone(), - ] - .concat(), - match self.versions_to_retire.clone() { - Some(versions) => [vec!["--replica-versions-to-unelect".to_string()], versions].concat(), - None => vec![], - }, - ] - .concat() - } -} - #[automock] pub trait IcAdmin: Send + Sync { fn neuron(&self) -> Neuron; @@ -72,15 +33,6 @@ pub trait IcAdmin: Send + Sync { fn run_passthrough_get<'a>(&'a self, args: &'a [String], silent: bool) -> BoxFuture<'_, anyhow::Result>; fn run_passthrough_propose<'a>(&'a self, args: &'a [String]) -> BoxFuture<'_, anyhow::Result>; - - fn prepare_to_propose_to_revise_elected_versions<'a>( - &'a self, - release_artifact: &'a Artifact, - version: &'a str, - release_tag: &'a str, - force: bool, - retire_versions: Option>, - ) -> BoxFuture<'_, anyhow::Result>; } #[derive(Clone)] @@ -197,97 +149,6 @@ impl IcAdmin for IcAdminImpl { let dry_run = self.dry_run || cmd.args().contains(&String::from("--dry-run")); Box::pin(async move { self.propose_run_inner(cmd, Default::default(), dry_run).await }) } - - fn prepare_to_propose_to_revise_elected_versions<'a>( - &'a self, - release_artifact: &'a Artifact, - version: &'a str, - release_tag: &'a str, - force: bool, - retire_versions: Option>, - ) -> BoxFuture<'_, anyhow::Result> { - Box::pin(async move { - let (update_urls, expected_hash) = Self::download_images_and_validate_sha256(release_artifact, version, force).await?; - - let template = format!( - r#"Elect new {release_artifact} binary revision [{version}](https://github.com/dfinity/ic/tree/{release_tag}) - - # Release Notes: - - [comment]: <> Remove this block of text from the proposal. - [comment]: <> Then, add the {release_artifact} binary release notes as bullet points here. - [comment]: <> Any [commit ID] within square brackets will auto-link to the specific changeset. - - # IC-OS Verification - - To build and verify the IC-OS disk image, run: - - ``` - # From https://github.com/dfinity/ic#verifying-releases - sudo apt-get install -y curl && curl --proto '=https' --tlsv1.2 -sSLO https://raw.githubusercontent.com/dfinity/ic/{version}/gitlab-ci/tools/repro-check.sh && chmod +x repro-check.sh && ./repro-check.sh -c {version} - ``` - - The two SHA256 sums printed above from a) the downloaded CDN image and b) the locally built image, - must be identical, and must match the SHA256 from the payload of the NNS proposal. - "# - ); - - // Remove from the commit - // Leading or trailing spaces are removed as well and replaced with a single space. - // Regex can be analyzed and tested at: - // https://rregex.dev/?version=1.7&method=replace®ex=%5Cs*%3C%21--.%2B%3F--%3E%5Cs*&replace=+&text=*+%5Babc%5D+%3C%21--+ignored+1+--%3E+line%0A*+%5Babc%5D+%3C%21--+ignored+2+--%3E+comment+1+%3C%21--+ignored+3+--%3E+comment+2%0A - let re_comment = Regex::new(r"\s*\s*").unwrap(); - let mut builder = edit::Builder::new(); - let with_suffix = builder.suffix(".md"); - let edited = edit::edit_with_builder(template, with_suffix)? - .trim() - .replace("\r(\n)?", "\n") - .split('\n') - .map(|f| { - let f = re_comment.replace_all(f.trim(), " "); - - if !f.starts_with('*') { - return f.to_string(); - } - match f.split_once(']') { - Some((left, message)) => { - let commit_hash = left.split_once('[').unwrap().1.to_string(); - - format!("* [[{}](https://github.com/dfinity/ic/commit/{})] {}", commit_hash, commit_hash, message) - } - None => f.to_string(), - } - }) - .join("\n"); - if edited.contains(&String::from("Remove this block of text from the proposal.")) { - Err(anyhow::anyhow!("The edited proposal text has not been edited to add release notes.")) - } else { - let proposal_title = match &retire_versions { - Some(v) => { - let pluralize = if v.len() == 1 { "version" } else { "versions" }; - format!( - "Elect new IC/{} revision (commit {}), and retire old replica {} {}", - release_artifact.capitalized(), - &version[..8], - pluralize, - v.iter().map(|v| &v[..8]).join(",") - ) - } - None => format!("Elect new IC/{} revision (commit {})", release_artifact.capitalized(), &version[..8]), - }; - - Ok(UpdateVersion { - release_artifact: release_artifact.clone(), - version: version.to_string(), - title: proposal_title.clone(), - stringified_hash: expected_hash, - summary: edited, - update_urls, - versions_to_retire: retire_versions.clone(), - }) - } - }) - } } impl IcAdminImpl { @@ -498,125 +359,6 @@ impl IcAdminImpl { } } } - - fn get_s3_cdn_image_url(version: &str, s3_subdir: &String) -> String { - format!( - "https://download.dfinity.systems/ic/{}/{}/update-img/update-img.tar.gz", - version, s3_subdir - ) - } - - fn get_r2_cdn_image_url(version: &str, s3_subdir: &String) -> String { - format!( - "https://download.dfinity.network/ic/{}/{}/update-img/update-img.tar.gz", - version, s3_subdir - ) - } - - async fn download_file_and_get_sha256(download_url: &String) -> anyhow::Result { - let url = url::Url::parse(download_url)?; - let subdir = format!("{}{}", url.domain().expect("url.domain() is None"), url.path().to_owned()); - // replace special characters in subdir with _ - let subdir = subdir.replace(|c: char| !c.is_ascii_alphanumeric(), "_"); - let download_dir = format!("{}/tmp/ic/{}", dirs::home_dir().expect("home_dir is not set").as_path().display(), subdir); - let download_dir = Path::new(&download_dir); - - std::fs::create_dir_all(download_dir).unwrap_or_else(|_| panic!("create_dir_all failed for {}", download_dir.display())); - - let download_image = format!("{}/update-img.tar.gz", download_dir.to_str().unwrap()); - let download_image = Path::new(&download_image); - - let response = reqwest::get(download_url.clone()).await?; - - if response.status() != StatusCode::RANGE_NOT_SATISFIABLE && !response.status().is_success() { - return Err(anyhow::anyhow!( - "Download failed with http_code {} for {}", - response.status(), - download_url - )); - } - info!("Download {} succeeded {}", download_url, response.status()); - - let mut file = match File::create(download_image) { - Ok(file) => file, - Err(err) => return Err(anyhow::anyhow!("Couldn't create a file: {}", err)), - }; - - let content = response.bytes().await?; - file.write_all(&content)?; - - let mut hasher = Sha256::new(); - hasher.update(&content); - let hash = hasher.finalize(); - let stringified_hash = hash[..].iter().map(|byte| format!("{:01$x?}", byte, 2)).collect::>().join(""); - info!("File saved at {} has sha256 {}", download_image.display(), stringified_hash); - Ok(stringified_hash) - } - - async fn download_images_and_validate_sha256( - image: &Artifact, - version: &str, - ignore_missing_urls: bool, - ) -> anyhow::Result<(Vec, String)> { - let update_urls = vec![ - Self::get_s3_cdn_image_url(version, &image.s3_folder()), - Self::get_r2_cdn_image_url(version, &image.s3_folder()), - ]; - - // Download images, verify them and compare the SHA256 - let hash_and_valid_urls: Vec<(String, &String)> = stream::iter(&update_urls) - .filter_map(|update_url| async move { - match Self::download_file_and_get_sha256(update_url).await { - Ok(hash) => { - info!("SHA256 of {}: {}", update_url, hash); - Some((hash, update_url)) - } - Err(err) => { - warn!("Error downloading {}: {}", update_url, err); - None - } - } - }) - .collect() - .await; - let hashes_unique = hash_and_valid_urls.iter().map(|(h, _)| h.clone()).unique().collect::>(); - let expected_hash: String = match hashes_unique.len() { - 0 => { - return Err(anyhow::anyhow!( - "Unable to download the update image from none of the following URLs: {}", - update_urls.join(", ") - )) - } - 1 => { - let hash = hashes_unique.into_iter().next().unwrap(); - info!("SHA256 of all download images is: {}", hash); - hash - } - _ => { - return Err(anyhow::anyhow!( - "Update images do not have the same hash: {:?}", - hash_and_valid_urls.iter().map(|(h, u)| format!("{} {}", h, u)).join("\n") - )) - } - }; - let update_urls = hash_and_valid_urls.into_iter().map(|(_, u)| u.clone()).collect::>(); - - if update_urls.is_empty() { - return Err(anyhow::anyhow!( - "Unable to download the update image from none of the following URLs: {}", - update_urls.join(", ") - )); - } else if update_urls.len() == 1 { - if ignore_missing_urls { - warn!("Only 1 update image is available. At least 2 should be present in the proposal"); - } else { - return Err(anyhow::anyhow!( - "Only 1 update image is available. At least 2 should be present in the proposal" - )); - } - } - Ok((update_urls, expected_hash)) - } } #[derive(Display, Clone)] diff --git a/rs/cli/src/lib.rs b/rs/cli/src/lib.rs index ca5782f8e..f9e5a9735 100644 --- a/rs/cli/src/lib.rs +++ b/rs/cli/src/lib.rs @@ -1,4 +1,5 @@ #![allow(dead_code)] +pub mod artifact_downloader; pub mod auth; pub mod commands; pub mod ctx; diff --git a/rs/cli/src/main.rs b/rs/cli/src/main.rs index 4f0d2ebb4..49e89d798 100644 --- a/rs/cli/src/main.rs +++ b/rs/cli/src/main.rs @@ -7,6 +7,7 @@ use ctx::DreContext; use dotenv::dotenv; use log::{info, warn}; +mod artifact_downloader; mod auth; mod commands; mod ctx; diff --git a/rs/cli/src/runner.rs b/rs/cli/src/runner.rs index f2a79af97..8eecbc77f 100644 --- a/rs/cli/src/runner.rs +++ b/rs/cli/src/runner.rs @@ -32,10 +32,12 @@ use itertools::Itertools; use log::info; use log::warn; +use regex::Regex; use registry_canister::mutations::do_change_subnet_membership::ChangeSubnetMembershipPayload; use tabled::builder::Builder; use tabled::settings::Style; +use crate::artifact_downloader::ArtifactDownloader; use crate::ic_admin::{self, IcAdmin}; use crate::ic_admin::{ProposeCommand, ProposeOptions}; use crate::operations::hostos_rollout::HostosRollout; @@ -293,7 +295,6 @@ impl Runner { forum_post_link: Option, ) -> anyhow::Result<()> { let update_version = self - .ic_admin .prepare_to_propose_to_revise_elected_versions( release_artifact, version, @@ -320,6 +321,95 @@ impl Runner { Ok(()) } + async fn prepare_to_propose_to_revise_elected_versions( + &self, + release_artifact: &Artifact, + version: &str, + release_tag: &str, + force: bool, + retire_versions: Option>, + ) -> anyhow::Result { + let (update_urls, expected_hash) = self.download_images_and_validate_sha256(release_artifact, version, force).await?; + + let template = format!( + r#"Elect new {release_artifact} binary revision [{version}](https://github.com/dfinity/ic/tree/{release_tag}) + + # Release Notes: + + [comment]: <> Remove this block of text from the proposal. + [comment]: <> Then, add the {release_artifact} binary release notes as bullet points here. + [comment]: <> Any [commit ID] within square brackets will auto-link to the specific changeset. + + # IC-OS Verification + + To build and verify the IC-OS disk image, run: + + ``` + # From https://github.com/dfinity/ic#verifying-releases + sudo apt-get install -y curl && curl --proto '=https' --tlsv1.2 -sSLO https://raw.githubusercontent.com/dfinity/ic/{version}/gitlab-ci/tools/repro-check.sh && chmod +x repro-check.sh && ./repro-check.sh -c {version} + ``` + + The two SHA256 sums printed above from a) the downloaded CDN image and b) the locally built image, + must be identical, and must match the SHA256 from the payload of the NNS proposal. + "# + ); + + // Remove from the commit + // Leading or trailing spaces are removed as well and replaced with a single space. + // Regex can be analyzed and tested at: + // https://rregex.dev/?version=1.7&method=replace®ex=%5Cs*%3C%21--.%2B%3F--%3E%5Cs*&replace=+&text=*+%5Babc%5D+%3C%21--+ignored+1+--%3E+line%0A*+%5Babc%5D+%3C%21--+ignored+2+--%3E+comment+1+%3C%21--+ignored+3+--%3E+comment+2%0A + let re_comment = Regex::new(r"\s*\s*").unwrap(); + let mut builder = edit::Builder::new(); + let with_suffix = builder.suffix(".md"); + let edited = edit::edit_with_builder(template, with_suffix)? + .trim() + .replace("\r(\n)?", "\n") + .split('\n') + .map(|f| { + let f = re_comment.replace_all(f.trim(), " "); + + if !f.starts_with('*') { + return f.to_string(); + } + match f.split_once(']') { + Some((left, message)) => { + let commit_hash = left.split_once('[').unwrap().1.to_string(); + + format!("* [[{}](https://github.com/dfinity/ic/commit/{})] {}", commit_hash, commit_hash, message) + } + None => f.to_string(), + } + }) + .join("\n"); + if edited.contains(&String::from("Remove this block of text from the proposal.")) { + Err(anyhow::anyhow!("The edited proposal text has not been edited to add release notes.")) + } else { + let proposal_title = match &retire_versions { + Some(v) => { + let pluralize = if v.len() == 1 { "version" } else { "versions" }; + format!( + "Elect new IC/{} revision (commit {}), and retire old replica {} {}", + release_artifact.capitalized(), + &version[..8], + pluralize, + v.iter().map(|v| &v[..8]).join(",") + ) + } + None => format!("Elect new IC/{} revision (commit {})", release_artifact.capitalized(), &version[..8]), + }; + + Ok(UpdateVersion { + release_artifact: release_artifact.clone(), + version: version.to_string(), + title: proposal_title.clone(), + stringified_hash: expected_hash, + summary: edited, + update_urls, + versions_to_retire: retire_versions.clone(), + }) + } + } + pub async fn hostos_rollout_nodes( &self, node_group: NodeGroupUpdate, @@ -781,3 +871,39 @@ fn nodes_by_dc(nodes: Vec) -> BTreeMap> { acc }) } + +#[derive(Clone)] +pub struct UpdateVersion { + pub release_artifact: Artifact, + pub version: String, + pub title: String, + pub summary: String, + pub update_urls: Vec, + pub stringified_hash: String, + pub versions_to_retire: Option>, +} + +impl UpdateVersion { + pub fn get_update_cmd_args(&self) -> Vec { + [ + [ + vec![ + "--replica-version-to-elect".to_string(), + self.version.to_string(), + "--release-package-sha256-hex".to_string(), + self.stringified_hash.to_string(), + "--release-package-urls".to_string(), + ], + self.update_urls.clone(), + ] + .concat(), + match self.versions_to_retire.clone() { + Some(versions) => [vec!["--replica-versions-to-unelect".to_string()], versions].concat(), + None => vec![], + }, + ] + .concat() + } +} + +impl ArtifactDownloader for Runner {} From 6a0437b669a8f57b0bcfd8537ac96b217d980e4c Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Fri, 6 Sep 2024 09:43:28 +0200 Subject: [PATCH 2/6] doing the code --- .../src/commands/version/revise/guest_os.rs | 5 + rs/cli/src/commands/version/revise/host_os.rs | 5 + rs/cli/src/runner.rs | 131 +++++++++++------- 3 files changed, 89 insertions(+), 52 deletions(-) diff --git a/rs/cli/src/commands/version/revise/guest_os.rs b/rs/cli/src/commands/version/revise/guest_os.rs index 80bfda795..7a8dbf9be 100644 --- a/rs/cli/src/commands/version/revise/guest_os.rs +++ b/rs/cli/src/commands/version/revise/guest_os.rs @@ -15,6 +15,10 @@ pub struct GuestOs { /// Force proposal submission, ignoring missing download URLs #[clap(long)] pub force: bool, + + /// Mark version as a security hotfix + #[clap(long)] + pub security_fix: bool, } impl ExecutableCommand for GuestOs { @@ -31,6 +35,7 @@ impl ExecutableCommand for GuestOs { &self.release_tag, self.force, ctx.forum_post_link(), + self.security_fix, ) .await } diff --git a/rs/cli/src/commands/version/revise/host_os.rs b/rs/cli/src/commands/version/revise/host_os.rs index 045937645..ef52dfa41 100644 --- a/rs/cli/src/commands/version/revise/host_os.rs +++ b/rs/cli/src/commands/version/revise/host_os.rs @@ -15,6 +15,10 @@ pub struct HostOs { /// Force proposal submission, ignoring missing download URLs #[clap(long)] pub force: bool, + + /// Mark version as a security hotfix + #[clap(long)] + pub security_fix: bool, } impl ExecutableCommand for HostOs { @@ -31,6 +35,7 @@ impl ExecutableCommand for HostOs { &self.release_tag, self.force, ctx.forum_post_link(), + self.security_fix, ) .await } diff --git a/rs/cli/src/runner.rs b/rs/cli/src/runner.rs index 8eecbc77f..a12c5562a 100644 --- a/rs/cli/src/runner.rs +++ b/rs/cli/src/runner.rs @@ -293,6 +293,7 @@ impl Runner { release_tag: &str, force: bool, forum_post_link: Option, + security_fix: bool, ) -> anyhow::Result<()> { let update_version = self .prepare_to_propose_to_revise_elected_versions( @@ -301,6 +302,7 @@ impl Runner { release_tag, force, self.prepare_versions_to_retire(release_artifact, false).await.map(|r| r.1)?, + security_fix, ) .await?; @@ -328,60 +330,15 @@ impl Runner { release_tag: &str, force: bool, retire_versions: Option>, + security_fix: bool, ) -> anyhow::Result { let (update_urls, expected_hash) = self.download_images_and_validate_sha256(release_artifact, version, force).await?; - let template = format!( - r#"Elect new {release_artifact} binary revision [{version}](https://github.com/dfinity/ic/tree/{release_tag}) - - # Release Notes: - - [comment]: <> Remove this block of text from the proposal. - [comment]: <> Then, add the {release_artifact} binary release notes as bullet points here. - [comment]: <> Any [commit ID] within square brackets will auto-link to the specific changeset. - - # IC-OS Verification - - To build and verify the IC-OS disk image, run: - - ``` - # From https://github.com/dfinity/ic#verifying-releases - sudo apt-get install -y curl && curl --proto '=https' --tlsv1.2 -sSLO https://raw.githubusercontent.com/dfinity/ic/{version}/gitlab-ci/tools/repro-check.sh && chmod +x repro-check.sh && ./repro-check.sh -c {version} - ``` - - The two SHA256 sums printed above from a) the downloaded CDN image and b) the locally built image, - must be identical, and must match the SHA256 from the payload of the NNS proposal. - "# - ); - - // Remove from the commit - // Leading or trailing spaces are removed as well and replaced with a single space. - // Regex can be analyzed and tested at: - // https://rregex.dev/?version=1.7&method=replace®ex=%5Cs*%3C%21--.%2B%3F--%3E%5Cs*&replace=+&text=*+%5Babc%5D+%3C%21--+ignored+1+--%3E+line%0A*+%5Babc%5D+%3C%21--+ignored+2+--%3E+comment+1+%3C%21--+ignored+3+--%3E+comment+2%0A - let re_comment = Regex::new(r"\s*\s*").unwrap(); - let mut builder = edit::Builder::new(); - let with_suffix = builder.suffix(".md"); - let edited = edit::edit_with_builder(template, with_suffix)? - .trim() - .replace("\r(\n)?", "\n") - .split('\n') - .map(|f| { - let f = re_comment.replace_all(f.trim(), " "); - - if !f.starts_with('*') { - return f.to_string(); - } - match f.split_once(']') { - Some((left, message)) => { - let commit_hash = left.split_once('[').unwrap().1.to_string(); - - format!("* [[{}](https://github.com/dfinity/ic/commit/{})] {}", commit_hash, commit_hash, message) - } - None => f.to_string(), - } - }) - .join("\n"); - if edited.contains(&String::from("Remove this block of text from the proposal.")) { + let summary = match security_fix { + true => format_security_hotfix(), + false => format_regular_version_upgrade_summary(version, release_artifact, release_tag)?, + }; + if summary.contains("Remove this block of text from the proposal.") { Err(anyhow::anyhow!("The edited proposal text has not been edited to add release notes.")) } else { let proposal_title = match &retire_versions { @@ -403,7 +360,7 @@ impl Runner { version: version.to_string(), title: proposal_title.clone(), stringified_hash: expected_hash, - summary: edited, + summary, update_urls, versions_to_retire: retire_versions.clone(), }) @@ -907,3 +864,73 @@ impl UpdateVersion { } impl ArtifactDownloader for Runner {} + +pub fn format_regular_version_upgrade_summary(version: &str, release_artifact: &Artifact, release_tag: &str) -> anyhow::Result { + let template = format!( + r#"Elect new {release_artifact} binary revision [{version}](https://github.com/dfinity/ic/tree/{release_tag}) + + # Release Notes: + + [comment]: <> Remove this block of text from the proposal. + [comment]: <> Then, add the {release_artifact} binary release notes as bullet points here. + [comment]: <> Any [commit ID] within square brackets will auto-link to the specific changeset. + + # IC-OS Verification + + To build and verify the IC-OS disk image, run: + + ``` + # From https://github.com/dfinity/ic#verifying-releases + sudo apt-get install -y curl && curl --proto '=https' --tlsv1.2 -sSLO https://raw.githubusercontent.com/dfinity/ic/{version}/gitlab-ci/tools/repro-check.sh && chmod +x repro-check.sh && ./repro-check.sh -c {version} + ``` + + The two SHA256 sums printed above from a) the downloaded CDN image and b) the locally built image, + must be identical, and must match the SHA256 from the payload of the NNS proposal. + "# + ); + + // Remove from the commit + // Leading or trailing spaces are removed as well and replaced with a single space. + // Regex can be analyzed and tested at: + // https://rregex.dev/?version=1.7&method=replace®ex=%5Cs*%3C%21--.%2B%3F--%3E%5Cs*&replace=+&text=*+%5Babc%5D+%3C%21--+ignored+1+--%3E+line%0A*+%5Babc%5D+%3C%21--+ignored+2+--%3E+comment+1+%3C%21--+ignored+3+--%3E+comment+2%0A + let re_comment = Regex::new(r"\s*\s*").unwrap(); + + Ok(match cfg!(test) { + true => template.lines().map(|l| l.trim()).filter(|l| !l.starts_with("[comment]")).join("\n"), + false => { + let mut builder = edit::Builder::new(); + let with_suffix = builder.suffix(".md"); + edit::edit_with_builder(template, with_suffix)? + } + } + .trim() + .replace("\r(\n)?", "\n") + .split('\n') + .map(|f| { + let f = re_comment.replace_all(f.trim(), " "); + + if !f.starts_with('*') { + return f.to_string(); + } + match f.split_once(']') { + Some((left, message)) => { + let commit_hash = left.split_once('[').unwrap().1.to_string(); + + format!("* [[{}](https://github.com/dfinity/ic/commit/{})] {}", commit_hash, commit_hash, message) + } + None => f.to_string(), + } + }) + .join("\n")) +} + +pub fn format_security_hotfix() -> String { + format!( + r#"# Security patch update + + In accordance with the Security Patch Policy and Procedure that was adopted in proposal [48792](https://dashboard.internetcomputer.org/proposal/48792), the source code that was used to build this release will be exposed at the latest 10 days after the fix is rolled out to all subnets. + + The community will be able to retroactively verify the binaries that were rolled out. +"# + ).lines().map(|l| l.trim()).join("\n") +} From 5a2a74c4b854ffd1801c4d2d91c3f8b4c89932f5 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Fri, 6 Sep 2024 10:25:26 +0200 Subject: [PATCH 3/6] refactoring --- rs/cli/src/artifact_downloader.rs | 194 +++++++++++----------- rs/cli/src/commands/version/mod.rs | 2 +- rs/cli/src/commands/version/revise/mod.rs | 4 +- rs/cli/src/ctx.rs | 8 +- rs/cli/src/runner.rs | 45 ++--- rs/cli/src/unit_tests/mod.rs | 1 + rs/cli/src/unit_tests/version.rs | 80 +++++++++ 7 files changed, 216 insertions(+), 118 deletions(-) create mode 100644 rs/cli/src/unit_tests/version.rs diff --git a/rs/cli/src/artifact_downloader.rs b/rs/cli/src/artifact_downloader.rs index d4ca7149a..f22adced3 100644 --- a/rs/cli/src/artifact_downloader.rs +++ b/rs/cli/src/artifact_downloader.rs @@ -1,7 +1,7 @@ #![allow(async_fn_in_trait)] use std::{fs::File, io::Write, path::Path}; -use futures::{stream, StreamExt}; +use futures::{future::BoxFuture, stream, StreamExt}; use ic_management_types::Artifact; use itertools::Itertools; use log::{info, warn}; @@ -9,8 +9,10 @@ use mockall::automock; use reqwest::StatusCode; use sha2::{Digest, Sha256}; +pub struct ArtifactDownloaderImpl {} + #[automock] -pub trait ArtifactDownloader { +pub trait ArtifactDownloader: Sync + Send { fn get_s3_cdn_image_url<'a>(&'a self, version: &'a str, s3_subdir: &'a str) -> String { format!( "https://download.dfinity.systems/ic/{}/{}/update-img/update-img.tar.gz", @@ -25,109 +27,115 @@ pub trait ArtifactDownloader { ) } - async fn download_file_and_get_sha256<'a>(&'a self, download_url: &'a str) -> anyhow::Result { - let url = url::Url::parse(download_url)?; - let subdir = format!("{}{}", url.domain().expect("url.domain() is None"), url.path().to_owned()); - // replace special characters in subdir with _ - let subdir = subdir.replace(|c: char| !c.is_ascii_alphanumeric(), "_"); - let download_dir = format!("{}/tmp/ic/{}", dirs::home_dir().expect("home_dir is not set").as_path().display(), subdir); - let download_dir = Path::new(&download_dir); - - std::fs::create_dir_all(download_dir).unwrap_or_else(|_| panic!("create_dir_all failed for {}", download_dir.display())); - - let download_image = format!("{}/update-img.tar.gz", download_dir.to_str().unwrap()); - let download_image = Path::new(&download_image); - - let response = reqwest::get(download_url).await?; - - if response.status() != StatusCode::RANGE_NOT_SATISFIABLE && !response.status().is_success() { - return Err(anyhow::anyhow!( - "Download failed with http_code {} for {}", - response.status(), - download_url - )); - } - info!("Download {} succeeded {}", download_url, response.status()); - - let mut file = match File::create(download_image) { - Ok(file) => file, - Err(err) => return Err(anyhow::anyhow!("Couldn't create a file: {}", err)), - }; - - let content = response.bytes().await?; - file.write_all(&content)?; - - let mut hasher = Sha256::new(); - hasher.update(&content); - let hash = hasher.finalize(); - let stringified_hash = hash[..].iter().map(|byte| format!("{:01$x?}", byte, 2)).collect::>().join(""); - info!("File saved at {} has sha256 {}", download_image.display(), stringified_hash); - Ok(stringified_hash) + fn download_file_and_get_sha256<'a>(&'a self, download_url: &'a str) -> BoxFuture<'_, anyhow::Result> { + Box::pin(async move { + let url = url::Url::parse(download_url)?; + let subdir = format!("{}{}", url.domain().expect("url.domain() is None"), url.path().to_owned()); + // replace special characters in subdir with _ + let subdir = subdir.replace(|c: char| !c.is_ascii_alphanumeric(), "_"); + let download_dir = format!("{}/tmp/ic/{}", dirs::home_dir().expect("home_dir is not set").as_path().display(), subdir); + let download_dir = Path::new(&download_dir); + + std::fs::create_dir_all(download_dir).unwrap_or_else(|_| panic!("create_dir_all failed for {}", download_dir.display())); + + let download_image = format!("{}/update-img.tar.gz", download_dir.to_str().unwrap()); + let download_image = Path::new(&download_image); + + let response = reqwest::get(download_url).await?; + + if response.status() != StatusCode::RANGE_NOT_SATISFIABLE && !response.status().is_success() { + return Err(anyhow::anyhow!( + "Download failed with http_code {} for {}", + response.status(), + download_url + )); + } + info!("Download {} succeeded {}", download_url, response.status()); + + let mut file = match File::create(download_image) { + Ok(file) => file, + Err(err) => return Err(anyhow::anyhow!("Couldn't create a file: {}", err)), + }; + + let content = response.bytes().await?; + file.write_all(&content)?; + + let mut hasher = Sha256::new(); + hasher.update(&content); + let hash = hasher.finalize(); + let stringified_hash = hash[..].iter().map(|byte| format!("{:01$x?}", byte, 2)).collect::>().join(""); + info!("File saved at {} has sha256 {}", download_image.display(), stringified_hash); + Ok(stringified_hash) + }) } - async fn download_images_and_validate_sha256<'a>( + fn download_images_and_validate_sha256<'a>( &'a self, image: &'a Artifact, version: &'a str, ignore_missing_urls: bool, - ) -> anyhow::Result<(Vec, String)> { - let update_urls = vec![ - self.get_s3_cdn_image_url(version, &image.s3_folder()), - self.get_r2_cdn_image_url(version, &image.s3_folder()), - ]; - - // Download images, verify them and compare the SHA256 - let hash_and_valid_urls: Vec<(String, &String)> = stream::iter(&update_urls) - .filter_map(|update_url| async move { - match self.download_file_and_get_sha256(update_url).await { - Ok(hash) => { - info!("SHA256 of {}: {}", update_url, hash); - Some((hash, update_url)) - } - Err(err) => { - warn!("Error downloading {}: {}", update_url, err); - None + ) -> BoxFuture<'_, anyhow::Result<(Vec, String)>> { + Box::pin(async move { + let update_urls = vec![ + self.get_s3_cdn_image_url(version, &image.s3_folder()), + self.get_r2_cdn_image_url(version, &image.s3_folder()), + ]; + + // Download images, verify them and compare the SHA256 + let hash_and_valid_urls: Vec<(String, &String)> = stream::iter(&update_urls) + .filter_map(|update_url| async move { + match self.download_file_and_get_sha256(update_url).await { + Ok(hash) => { + info!("SHA256 of {}: {}", update_url, hash); + Some((hash, update_url)) + } + Err(err) => { + warn!("Error downloading {}: {}", update_url, err); + None + } } + }) + .collect() + .await; + let hashes_unique = hash_and_valid_urls.iter().map(|(h, _)| h.clone()).unique().collect::>(); + let expected_hash: String = match hashes_unique.len() { + 0 => { + return Err(anyhow::anyhow!( + "Unable to download the update image from none of the following URLs: {}", + update_urls.join(", ") + )) + } + 1 => { + let hash = hashes_unique.into_iter().next().unwrap(); + info!("SHA256 of all download images is: {}", hash); + hash + } + _ => { + return Err(anyhow::anyhow!( + "Update images do not have the same hash: {:?}", + hash_and_valid_urls.iter().map(|(h, u)| format!("{} {}", h, u)).join("\n") + )) } - }) - .collect() - .await; - let hashes_unique = hash_and_valid_urls.iter().map(|(h, _)| h.clone()).unique().collect::>(); - let expected_hash: String = match hashes_unique.len() { - 0 => { + }; + let update_urls = hash_and_valid_urls.into_iter().map(|(_, u)| u.clone()).collect::>(); + + if update_urls.is_empty() { return Err(anyhow::anyhow!( "Unable to download the update image from none of the following URLs: {}", update_urls.join(", ") - )) - } - 1 => { - let hash = hashes_unique.into_iter().next().unwrap(); - info!("SHA256 of all download images is: {}", hash); - hash - } - _ => { - return Err(anyhow::anyhow!( - "Update images do not have the same hash: {:?}", - hash_and_valid_urls.iter().map(|(h, u)| format!("{} {}", h, u)).join("\n") - )) - } - }; - let update_urls = hash_and_valid_urls.into_iter().map(|(_, u)| u.clone()).collect::>(); - - if update_urls.is_empty() { - return Err(anyhow::anyhow!( - "Unable to download the update image from none of the following URLs: {}", - update_urls.join(", ") - )); - } else if update_urls.len() == 1 { - if ignore_missing_urls { - warn!("Only 1 update image is available. At least 2 should be present in the proposal"); - } else { - return Err(anyhow::anyhow!( - "Only 1 update image is available. At least 2 should be present in the proposal" )); + } else if update_urls.len() == 1 { + if ignore_missing_urls { + warn!("Only 1 update image is available. At least 2 should be present in the proposal"); + } else { + return Err(anyhow::anyhow!( + "Only 1 update image is available. At least 2 should be present in the proposal" + )); + } } - } - Ok((update_urls, expected_hash)) + Ok((update_urls, expected_hash)) + }) } } + +impl ArtifactDownloader for ArtifactDownloaderImpl {} diff --git a/rs/cli/src/commands/version/mod.rs b/rs/cli/src/commands/version/mod.rs index a4a98289c..b00b35741 100644 --- a/rs/cli/src/commands/version/mod.rs +++ b/rs/cli/src/commands/version/mod.rs @@ -3,7 +3,7 @@ use clap::Args; use super::{impl_executable_command_for_enums, ExecutableCommand, IcAdminRequirement}; use crate::commands::version::revise::ReviseElectedVersions; -mod revise; +pub(crate) mod revise; #[derive(Args, Debug)] pub struct Version { diff --git a/rs/cli/src/commands/version/revise/mod.rs b/rs/cli/src/commands/version/revise/mod.rs index 807506f6b..260caf1fa 100644 --- a/rs/cli/src/commands/version/revise/mod.rs +++ b/rs/cli/src/commands/version/revise/mod.rs @@ -4,8 +4,8 @@ use crate::commands::version::revise::host_os::HostOs; use clap::Args; use ic_management_types::Artifact; -mod guest_os; -mod host_os; +pub(crate) mod guest_os; +pub(crate) mod host_os; #[derive(Args, Debug)] pub struct ReviseElectedVersions { diff --git a/rs/cli/src/ctx.rs b/rs/cli/src/ctx.rs index 2be3efe74..e9cd1e861 100644 --- a/rs/cli/src/ctx.rs +++ b/rs/cli/src/ctx.rs @@ -20,6 +20,7 @@ use log::{debug, info}; use url::Url; use crate::{ + artifact_downloader::{ArtifactDownloader, ArtifactDownloaderImpl}, auth::{Auth, Neuron}, commands::{Args, ExecutableCommand, IcAdminRequirement, IcAdminVersion}, ic_admin::{download_ic_admin, should_update_ic_admin, IcAdmin, IcAdminImpl, FALLBACK_IC_ADMIN_VERSION}, @@ -41,6 +42,7 @@ pub struct DreContext { ic_admin_path: Option, forum_post_link: Option, dry_run: bool, + artifact_downloader: Arc, } impl DreContext { @@ -99,6 +101,7 @@ impl DreContext { forum_post_link: forum_post_link.clone(), ic_repo: RefCell::new(None), dry_run, + artifact_downloader: Arc::new(ArtifactDownloaderImpl {}) as Arc, }) } @@ -267,6 +270,7 @@ impl DreContext { self.proposals_agent(), self.verbose_runner, self.ic_repo.clone(), + self.artifact_downloader.clone(), )); *self.runner.borrow_mut() = Some(runner.clone()); runner @@ -290,7 +294,7 @@ pub mod tests { use ic_management_backend::{lazy_git::LazyGit, lazy_registry::LazyRegistry, proposal::ProposalAgent}; use ic_management_types::Network; - use crate::ic_admin::IcAdmin; + use crate::{artifact_downloader::ArtifactDownloader, ic_admin::IcAdmin}; use super::DreContext; @@ -300,6 +304,7 @@ pub mod tests { ic_admin: Arc, git: Arc, proposal_agent: Arc, + artifact_downloader: Arc, ) -> DreContext { DreContext { network, @@ -313,6 +318,7 @@ pub mod tests { ic_admin_path: None, forum_post_link: None, dry_run: true, + artifact_downloader, } } } diff --git a/rs/cli/src/runner.rs b/rs/cli/src/runner.rs index a12c5562a..8a4d4d235 100644 --- a/rs/cli/src/runner.rs +++ b/rs/cli/src/runner.rs @@ -51,6 +51,7 @@ pub struct Runner { network: Network, proposal_agent: Arc, verbose: bool, + artifact_downloader: Arc, } impl Runner { @@ -61,6 +62,7 @@ impl Runner { agent: Arc, verbose: bool, ic_repo: RefCell>>, + artifact_downloader: Arc, ) -> Self { Self { ic_admin, @@ -69,6 +71,7 @@ impl Runner { network, proposal_agent: agent, verbose, + artifact_downloader, } } @@ -332,7 +335,10 @@ impl Runner { retire_versions: Option>, security_fix: bool, ) -> anyhow::Result { - let (update_urls, expected_hash) = self.download_images_and_validate_sha256(release_artifact, version, force).await?; + let (update_urls, expected_hash) = self + .artifact_downloader + .download_images_and_validate_sha256(release_artifact, version, force) + .await?; let summary = match security_fix { true => format_security_hotfix(), @@ -341,18 +347,21 @@ impl Runner { if summary.contains("Remove this block of text from the proposal.") { Err(anyhow::anyhow!("The edited proposal text has not been edited to add release notes.")) } else { - let proposal_title = match &retire_versions { - Some(v) => { - let pluralize = if v.len() == 1 { "version" } else { "versions" }; - format!( - "Elect new IC/{} revision (commit {}), and retire old replica {} {}", - release_artifact.capitalized(), - &version[..8], - pluralize, - v.iter().map(|v| &v[..8]).join(",") - ) - } - None => format!("Elect new IC/{} revision (commit {})", release_artifact.capitalized(), &version[..8]), + let proposal_title = match security_fix { + true => "Security patch update".to_string(), + false => match &retire_versions { + Some(v) => { + let pluralize = if v.len() == 1 { "version" } else { "versions" }; + format!( + "Elect new IC/{} revision (commit {}), and retire old replica {} {}", + release_artifact.capitalized(), + &version[..8], + pluralize, + v.iter().map(|v| &v[..8]).join(",") + ) + } + None => format!("Elect new IC/{} revision (commit {})", release_artifact.capitalized(), &version[..8]), + }, }; Ok(UpdateVersion { @@ -863,8 +872,6 @@ impl UpdateVersion { } } -impl ArtifactDownloader for Runner {} - pub fn format_regular_version_upgrade_summary(version: &str, release_artifact: &Artifact, release_tag: &str) -> anyhow::Result { let template = format!( r#"Elect new {release_artifact} binary revision [{version}](https://github.com/dfinity/ic/tree/{release_tag}) @@ -925,12 +932,8 @@ pub fn format_regular_version_upgrade_summary(version: &str, release_artifact: & } pub fn format_security_hotfix() -> String { - format!( - r#"# Security patch update - - In accordance with the Security Patch Policy and Procedure that was adopted in proposal [48792](https://dashboard.internetcomputer.org/proposal/48792), the source code that was used to build this release will be exposed at the latest 10 days after the fix is rolled out to all subnets. + r#"In accordance with the Security Patch Policy and Procedure that was adopted in proposal [48792](https://dashboard.internetcomputer.org/proposal/48792), the source code that was used to build this release will be exposed at the latest 10 days after the fix is rolled out to all subnets. The community will be able to retroactively verify the binaries that were rolled out. -"# - ).lines().map(|l| l.trim()).join("\n") +"#.to_string().lines().map(|l| l.trim()).join("\n") } diff --git a/rs/cli/src/unit_tests/mod.rs b/rs/cli/src/unit_tests/mod.rs index acccbf331..754210a4a 100644 --- a/rs/cli/src/unit_tests/mod.rs +++ b/rs/cli/src/unit_tests/mod.rs @@ -1,2 +1,3 @@ mod ctx_init; mod update_unassigned_nodes; +mod version; diff --git a/rs/cli/src/unit_tests/version.rs b/rs/cli/src/unit_tests/version.rs new file mode 100644 index 000000000..6a28ceb01 --- /dev/null +++ b/rs/cli/src/unit_tests/version.rs @@ -0,0 +1,80 @@ +use std::{collections::BTreeMap, sync::Arc}; + +use decentralization::network::Identifies; +use ic_management_backend::{lazy_git::MockLazyGit, lazy_registry::MockLazyRegistry, proposal::MockProposalAgent}; +use ic_management_types::{Artifact, ArtifactReleases, Network}; +use itertools::Itertools; + +use crate::{ + artifact_downloader::MockArtifactDownloader, + commands::ExecutableCommand, + ctx::tests::get_mocked_ctx, + ic_admin::{MockIcAdmin, ProposeCommand, ProposeOptions}, + runner::format_regular_version_upgrade_summary, +}; + +#[tokio::test] +async fn guest_os_elect_version_tests() { + let mut ic_admin = MockIcAdmin::new(); + let mut captured_cmd: Option = None; + let mut captured_opts: Option = None; + ic_admin.expect_propose_run().returning(|cmd, opts| { + captured_cmd = Some(cmd.clone()); + captured_opts = Some(captured_opts.clone()) + }); + + let mut git = MockLazyGit::new(); + git.expect_guestos_releases() + .returning(|| Ok(Arc::new(ArtifactReleases::new(ic_management_types::Artifact::GuestOs)))); + + let mut registry = MockLazyRegistry::new(); + registry.expect_subnets().returning(|| Ok(Arc::new(BTreeMap::new()))); + + let mut proposal_agent = MockProposalAgent::new(); + proposal_agent.expect_list_open_elect_replica_proposals().returning(|| Ok(vec![])); + + let download_urls = ["https://ver1.download.link", "https://ver1.alt.download.link"] + .iter() + .map(|s| s.to_string()) + .collect_vec(); + let sha = "sha_of_ver".to_string(); + let mut artifact_downloader = MockArtifactDownloader::new(); + artifact_downloader + .expect_download_images_and_validate_sha256() + .returning(|| (download_urls, sha)); + + let ctx = get_mocked_ctx( + Network::mainnet_unchecked().unwrap(), + registry, + ic_admin, + git, + proposal_agent, + artifact_downloader, + ); + + let cmd = crate::commands::version::revise::guest_os::GuestOs { + version: "new_version".to_string(), + release_tag: "rel_tag".to_string(), + force: false, + security_fix: false, + }; + + let resp = cmd.execute(ctx).await; + assert!(resp.is_ok()); + + assert!(captured_cmd.is_some() && captured_opts.is_some()); + let (artifact, args) = match captured_cmd.unwrap() { + ProposeCommand::ReviseElectedVersions { release_artifact, args } => (release_artifact, args), + _ => panic!("Unexpected proposal command"), + }; + + let opts = captured_opts.unwrap(); + + assert_eq!(artifact, Artifact::GuestOs); + assert!(args.contains(&sha) && args.contains(&cmd.version)); + assert!(opts.title.unwrap().starts_with("Elect new IC")); + assert!(opts + .summary + .unwrap() + .eq(&format_regular_version_upgrade_summary(&cmd.version, &Artifact::GuestOs, &cmd.release_tag).unwrap())) +} From e54c80ed514eee2ad0ddf8089a4dd235d2dae535 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Fri, 6 Sep 2024 12:59:55 +0200 Subject: [PATCH 4/6] adding tests --- .../src/unit_tests/update_unassigned_nodes.rs | 5 ++ rs/cli/src/unit_tests/version.rs | 69 ++++++++++++------- rs/ic-management-types/src/lib.rs | 2 +- 3 files changed, 51 insertions(+), 25 deletions(-) diff --git a/rs/cli/src/unit_tests/update_unassigned_nodes.rs b/rs/cli/src/unit_tests/update_unassigned_nodes.rs index 4a4c24dfd..d25b20a4a 100644 --- a/rs/cli/src/unit_tests/update_unassigned_nodes.rs +++ b/rs/cli/src/unit_tests/update_unassigned_nodes.rs @@ -1,5 +1,6 @@ use std::{str::FromStr, sync::Arc}; +use crate::artifact_downloader::MockArtifactDownloader; use crate::auth::Neuron; use crate::commands::{update_unassigned_nodes::UpdateUnassignedNodes, ExecutableCommand}; use crate::ic_admin::MockIcAdmin; @@ -53,6 +54,7 @@ async fn should_skip_update_same_version_nns_not_provided() { Arc::new(ic_admin), Arc::new(MockLazyGit::new()), Arc::new(MockProposalAgent::new()), + Arc::new(MockArtifactDownloader::new()), ); let cmd = UpdateUnassignedNodes { nns_subnet_id: None }; @@ -85,6 +87,7 @@ async fn should_skip_update_same_version_nns_provided() { Arc::new(ic_admin), Arc::new(MockLazyGit::new()), Arc::new(MockProposalAgent::new()), + Arc::new(MockArtifactDownloader::new()), ); let cmd = UpdateUnassignedNodes { @@ -122,6 +125,7 @@ async fn should_update_unassigned_nodes() { Arc::new(ic_admin), Arc::new(MockLazyGit::new()), Arc::new(MockProposalAgent::new()), + Arc::new(MockArtifactDownloader::new()), ); let cmd = UpdateUnassignedNodes { @@ -157,6 +161,7 @@ async fn should_fail_nns_not_found() { Arc::new(ic_admin), Arc::new(MockLazyGit::new()), Arc::new(MockProposalAgent::new()), + Arc::new(MockArtifactDownloader::new()), ); let cmd = UpdateUnassignedNodes { diff --git a/rs/cli/src/unit_tests/version.rs b/rs/cli/src/unit_tests/version.rs index 6a28ceb01..24bfccdcc 100644 --- a/rs/cli/src/unit_tests/version.rs +++ b/rs/cli/src/unit_tests/version.rs @@ -1,6 +1,9 @@ -use std::{collections::BTreeMap, sync::Arc}; +use std::{ + collections::BTreeMap, + sync::{Arc, RwLock}, +}; -use decentralization::network::Identifies; +use futures::future::ok; use ic_management_backend::{lazy_git::MockLazyGit, lazy_registry::MockLazyRegistry, proposal::MockProposalAgent}; use ic_management_types::{Artifact, ArtifactReleases, Network}; use itertools::Itertools; @@ -16,40 +19,56 @@ use crate::{ #[tokio::test] async fn guest_os_elect_version_tests() { let mut ic_admin = MockIcAdmin::new(); - let mut captured_cmd: Option = None; - let mut captured_opts: Option = None; - ic_admin.expect_propose_run().returning(|cmd, opts| { - captured_cmd = Some(cmd.clone()); - captured_opts = Some(captured_opts.clone()) + let captured_cmd: Arc>> = Arc::new(RwLock::new(None)); + let captured_opts: Arc>> = Arc::new(RwLock::new(None)); + let captured_cmd_clone = captured_cmd.clone(); + let captured_opts_clone = captured_opts.clone(); + ic_admin.expect_propose_run().returning(move |cmd, opts| { + *captured_cmd_clone.write().unwrap() = Some(cmd.clone()); + *captured_opts_clone.write().unwrap() = Some(opts.clone()); + Box::pin(ok("Proposal 123".to_string())) }); let mut git = MockLazyGit::new(); git.expect_guestos_releases() - .returning(|| Ok(Arc::new(ArtifactReleases::new(ic_management_types::Artifact::GuestOs)))); + .returning(|| Box::pin(ok(Arc::new(ArtifactReleases::new(ic_management_types::Artifact::GuestOs))))); let mut registry = MockLazyRegistry::new(); - registry.expect_subnets().returning(|| Ok(Arc::new(BTreeMap::new()))); + registry.expect_subnets().returning(|| Box::pin(ok(Arc::new(BTreeMap::new())))); + registry + .expect_unassigned_nodes_replica_version() + .returning(|| Box::pin(ok(Arc::new("some_ver".to_string())))); let mut proposal_agent = MockProposalAgent::new(); - proposal_agent.expect_list_open_elect_replica_proposals().returning(|| Ok(vec![])); + proposal_agent + .expect_list_open_elect_replica_proposals() + .returning(|| Box::pin(ok(vec![]))); let download_urls = ["https://ver1.download.link", "https://ver1.alt.download.link"] .iter() .map(|s| s.to_string()) .collect_vec(); + let downloads_urls_clone = download_urls.clone(); let sha = "sha_of_ver".to_string(); + let sha_clone = sha.clone(); let mut artifact_downloader = MockArtifactDownloader::new(); artifact_downloader .expect_download_images_and_validate_sha256() - .returning(|| (download_urls, sha)); + .returning(move |_, _, _| { + Box::pin({ + let sha_clone = sha_clone.clone(); + let downloads_urls_clone = downloads_urls_clone.clone(); + async move { Ok((downloads_urls_clone, sha_clone)) } + }) + }); let ctx = get_mocked_ctx( Network::mainnet_unchecked().unwrap(), - registry, - ic_admin, - git, - proposal_agent, - artifact_downloader, + Arc::new(registry), + Arc::new(ic_admin), + Arc::new(git), + Arc::new(proposal_agent), + Arc::new(artifact_downloader), ); let cmd = crate::commands::version::revise::guest_os::GuestOs { @@ -62,19 +81,21 @@ async fn guest_os_elect_version_tests() { let resp = cmd.execute(ctx).await; assert!(resp.is_ok()); + let captured_cmd = captured_cmd.read().unwrap(); + let captured_opts = captured_opts.read().unwrap(); assert!(captured_cmd.is_some() && captured_opts.is_some()); - let (artifact, args) = match captured_cmd.unwrap() { + let (artifact, args) = match captured_cmd.as_ref().unwrap() { ProposeCommand::ReviseElectedVersions { release_artifact, args } => (release_artifact, args), _ => panic!("Unexpected proposal command"), }; - let opts = captured_opts.unwrap(); + let opts = captured_opts.as_ref().unwrap(); - assert_eq!(artifact, Artifact::GuestOs); + assert_eq!(*artifact, Artifact::GuestOs); assert!(args.contains(&sha) && args.contains(&cmd.version)); - assert!(opts.title.unwrap().starts_with("Elect new IC")); - assert!(opts - .summary - .unwrap() - .eq(&format_regular_version_upgrade_summary(&cmd.version, &Artifact::GuestOs, &cmd.release_tag).unwrap())) + assert!(opts.title.as_ref().unwrap().starts_with("Elect new IC")); + assert_eq!( + format_regular_version_upgrade_summary(&cmd.version, &Artifact::GuestOs, &cmd.release_tag).unwrap(), + *opts.summary.as_ref().unwrap(), + ) } diff --git a/rs/ic-management-types/src/lib.rs b/rs/ic-management-types/src/lib.rs index 195aa18b5..b4cbf629c 100644 --- a/rs/ic-management-types/src/lib.rs +++ b/rs/ic-management-types/src/lib.rs @@ -607,7 +607,7 @@ impl ArtifactReleases { } } -#[derive(strum_macros::Display, Serialize, Deserialize, PartialEq, Eq, Hash, Clone)] +#[derive(strum_macros::Display, Serialize, Deserialize, PartialEq, Eq, Hash, Clone, Debug)] #[strum(serialize_all = "lowercase")] #[serde(rename_all = "lowercase")] pub enum Artifact { From df112c5ce4bcf79a5e26a6a068a02630859b1122 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Fri, 6 Sep 2024 13:08:20 +0200 Subject: [PATCH 5/6] refactoring tests --- rs/cli/src/unit_tests/version.rs | 82 ++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 24 deletions(-) diff --git a/rs/cli/src/unit_tests/version.rs b/rs/cli/src/unit_tests/version.rs index 24bfccdcc..4fb21c872 100644 --- a/rs/cli/src/unit_tests/version.rs +++ b/rs/cli/src/unit_tests/version.rs @@ -13,7 +13,7 @@ use crate::{ commands::ExecutableCommand, ctx::tests::get_mocked_ctx, ic_admin::{MockIcAdmin, ProposeCommand, ProposeOptions}, - runner::format_regular_version_upgrade_summary, + runner::{format_regular_version_upgrade_summary, format_security_hotfix}, }; #[tokio::test] @@ -71,31 +71,65 @@ async fn guest_os_elect_version_tests() { Arc::new(artifact_downloader), ); - let cmd = crate::commands::version::revise::guest_os::GuestOs { - version: "new_version".to_string(), - release_tag: "rel_tag".to_string(), - force: false, - security_fix: false, - }; + for (name, expected_title, cmd) in [ + ( + "Regular version upgrade", + "Elect new IC", + crate::commands::version::revise::guest_os::GuestOs { + version: "new_version".to_string(), + release_tag: "rel_tag".to_string(), + force: false, + security_fix: false, + }, + ), + ( + "Security fix", + "Security patch update", + crate::commands::version::revise::guest_os::GuestOs { + version: "new_version".to_string(), + release_tag: "rel_tag".to_string(), + force: false, + security_fix: true, + }, + ), + ] { + let resp = cmd.execute(ctx.clone()).await; + assert!(resp.is_ok(), "Test {} failed, command finished with err: {:?}", name, resp.err().unwrap()); - let resp = cmd.execute(ctx).await; - assert!(resp.is_ok()); + let mut captured_cmd = captured_cmd.write().unwrap(); + let mut captured_opts = captured_opts.write().unwrap(); + assert!( + captured_cmd.is_some() && captured_opts.is_some(), + "Test {} failed, ic-admin not called but expected to be", + name + ); + let (artifact, args) = match captured_cmd.as_ref().unwrap() { + ProposeCommand::ReviseElectedVersions { release_artifact, args } => (release_artifact, args), + _ => panic!("Test {} captured an unexpected proposal command", name), + }; - let captured_cmd = captured_cmd.read().unwrap(); - let captured_opts = captured_opts.read().unwrap(); - assert!(captured_cmd.is_some() && captured_opts.is_some()); - let (artifact, args) = match captured_cmd.as_ref().unwrap() { - ProposeCommand::ReviseElectedVersions { release_artifact, args } => (release_artifact, args), - _ => panic!("Unexpected proposal command"), - }; + let opts = captured_opts.as_ref().unwrap(); - let opts = captured_opts.as_ref().unwrap(); + assert_eq!(*artifact, Artifact::GuestOs, "Test {} received an unexpected artifact", name); + assert!( + args.contains(&sha) && args.contains(&cmd.version), + "Test {} arguments don't contain correct sha `{}` or version `{}`. Got [{}]", + sha, + cmd.version, + name, + args.iter().join(", ") + ); + assert!(opts.title.as_ref().unwrap().starts_with(expected_title)); + assert_eq!( + match cmd.security_fix { + true => format_security_hotfix(), + false => format_regular_version_upgrade_summary(&cmd.version, &Artifact::GuestOs, &cmd.release_tag).unwrap(), + }, + *opts.summary.as_ref().unwrap(), + ); - assert_eq!(*artifact, Artifact::GuestOs); - assert!(args.contains(&sha) && args.contains(&cmd.version)); - assert!(opts.title.as_ref().unwrap().starts_with("Elect new IC")); - assert_eq!( - format_regular_version_upgrade_summary(&cmd.version, &Artifact::GuestOs, &cmd.release_tag).unwrap(), - *opts.summary.as_ref().unwrap(), - ) + // Prepare for next test + *captured_cmd = None; + *captured_opts = None; + } } From 6198ed9e436e88a99680561c7d43fc58f87fc126 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Fri, 6 Sep 2024 13:20:35 +0200 Subject: [PATCH 6/6] fixing links --- rs/cli/src/artifact_downloader.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rs/cli/src/artifact_downloader.rs b/rs/cli/src/artifact_downloader.rs index f22adced3..ca07f4e54 100644 --- a/rs/cli/src/artifact_downloader.rs +++ b/rs/cli/src/artifact_downloader.rs @@ -15,14 +15,14 @@ pub struct ArtifactDownloaderImpl {} pub trait ArtifactDownloader: Sync + Send { fn get_s3_cdn_image_url<'a>(&'a self, version: &'a str, s3_subdir: &'a str) -> String { format!( - "https://download.dfinity.systems/ic/{}/{}/update-img/update-img.tar.gz", + "https://download.dfinity.systems/ic/{}/{}/update-img/update-img.tar.zst", version, s3_subdir ) } fn get_r2_cdn_image_url<'a>(&'a self, version: &'a str, s3_subdir: &'a str) -> String { format!( - "https://download.dfinity.network/ic/{}/{}/update-img/update-img.tar.gz", + "https://download.dfinity.network/ic/{}/{}/update-img/update-img.tar.zst", version, s3_subdir ) }