feat: overriding ic admin versions (#864)

.github/workflows/main.yaml

@@ -39,13 +39,15 @@ jobs:
 
       ########################################
       # Build and test
+      # Will run test as a local subprocess because for some tests
+      # create status files on certain locations (like $HOME)
       ########################################
       - name: "🚀 Building"
         uses: ./.github/workflows/build
         with:
           GITHUB_TOKEN: "${{ secrets.GIX_CREATE_PR_PAT }}"
       - name: "🚀 Testing"
-        run: bazel test ...
+        run: bazel test --spawn_strategy=local ...
 
       # We don't need the linear-jira build and test step for now
       # - name: "🚀 Build and Test Linear-Jira with Bazel"

rs/cli/src/commands/mod.rs

@@ -20,6 +20,7 @@ use proposals::Proposals;
 use propose::Propose;
 use qualify::Qualify;
 use registry::Registry;
+use strum::Display;
 use update_authorized_subnets::UpdateAuthorizedSubnets;
 use update_unassigned_nodes::UpdateUnassignedNodes;
 use upgrade::Upgrade;
@@ -124,6 +125,13 @@ pub(crate) struct Args {
     #[clap(long, global = true, env = "IC_ADMIN")]
     pub ic_admin: Option<String>,
 
+    #[clap(long, global = true, env = "IC_ADMIN_VERSION", default_value_t = IcAdminVersion::FromGovernance, value_parser = clap::value_parser!(IcAdminVersion), help = r#"Specify the version of ic admin to use
+Options:
+    1. from-governance, governance, govn, g => same as governance canister
+    2. default, d => strict default version, embedded at build time
+    3. <commit> => specific commit"#)]
+    pub ic_admin_version: IcAdminVersion,
+
     /// To skip the confirmation prompt
     #[clap(short, long, global = true, env = "YES", conflicts_with = "dry_run")]
     pub yes: bool,
@@ -299,3 +307,20 @@ pub enum IcAdminRequirement {
     Detect,                                                 // detect the neuron
     OverridableBy { network: Network, neuron: AuthNeuron }, // eg automation which we know where is placed
 }
+
+#[derive(Debug, Display, Clone)]
+pub enum IcAdminVersion {
+    FromGovernance,
+    Default,
+    Strict(String),
+}
+
+impl From<&str> for IcAdminVersion {
+    fn from(value: &str) -> Self {
+        match value {
+            "from-governance" | "governance" | "g" | "govn" => Self::FromGovernance,
+            "default" | "d" => Self::Default,
+            s => Self::Strict(s.to_string()),
+        }
+    }
+}

rs/cli/src/ctx.rs

@@ -16,13 +16,13 @@ use ic_management_backend::{
 };
 use ic_management_types::Network;
 use ic_registry_local_registry::LocalRegistry;
-use log::info;
+use log::{debug, info};
 use url::Url;
 
 use crate::{
     auth::{Auth, Neuron},
-    commands::{Args, ExecutableCommand, IcAdminRequirement},
-    ic_admin::{download_ic_admin, should_update_ic_admin, IcAdmin, IcAdminImpl},
+    commands::{Args, ExecutableCommand, IcAdminRequirement, IcAdminVersion},
+    ic_admin::{download_ic_admin, should_update_ic_admin, IcAdmin, IcAdminImpl, DEFAULT_IC_ADMIN_VERSION},
     runner::Runner,
     subnet_manager::SubnetManager,
 };
@@ -55,6 +55,7 @@ impl DreContext {
         dry_run: bool,
         ic_admin_requirement: IcAdminRequirement,
         forum_post_link: Option<String>,
+        ic_admin_version: IcAdminVersion,
     ) -> anyhow::Result<Self> {
         let network = match no_sync {
             false => ic_management_types::Network::new(network.clone(), &nns_urls)
@@ -83,7 +84,8 @@ impl DreContext {
             (neuron_id, auth.clone())
         };
 
-        let (ic_admin, ic_admin_path) = Self::init_ic_admin(&network, neuron_id, auth_opts, yes, dry_run, ic_admin_requirement).await?;
+        let (ic_admin, ic_admin_path) =
+            Self::init_ic_admin(&network, neuron_id, auth_opts, yes, dry_run, ic_admin_requirement, ic_admin_version).await?;
 
         Ok(Self {
             proposal_agent: Arc::new(ProposalAgentImpl::new(&network.nns_urls)),
@@ -112,6 +114,7 @@ impl DreContext {
             args.dry_run,
             args.subcommands.require_ic_admin(),
             args.forum_post_link.clone(),
+            args.ic_admin_version.clone(),
         )
         .await
     }
@@ -123,6 +126,7 @@ impl DreContext {
         proceed_without_confirmation: bool,
         dry_run: bool,
         requirement: IcAdminRequirement,
+        version: IcAdminVersion,
     ) -> anyhow::Result<(Option<Arc<dyn IcAdmin>>, Option<String>)> {
         if let IcAdminRequirement::None = requirement {
             return Ok((None, None));
@@ -147,18 +151,36 @@ impl DreContext {
                 }
             }
         };
-        let ic_admin_path = match should_update_ic_admin()? {
-            (true, _) => {
-                let govn_canister_version = governance_canister_version(network.get_nns_urls()).await?;
-                download_ic_admin(match govn_canister_version.stringified_hash.as_str() {
-                    // Some testnets could have this version setup if deployed
-                    // from HEAD of the branch they are created from
-                    "0000000000000000000000000000000000000000" => None,
-                    v => Some(v.to_owned()),
-                })
-                .await?
+
+        let ic_admin_path = match version {
+            IcAdminVersion::FromGovernance => match should_update_ic_admin()? {
+                (true, _) => {
+                    let govn_canister_version = governance_canister_version(network.get_nns_urls()).await?;
+                    debug!(
+                        "Using ic-admin matching the version of governance canister, version: {}",
+                        govn_canister_version.stringified_hash
+                    );
+                    download_ic_admin(match govn_canister_version.stringified_hash.as_str() {
+                        // Some testnets could have this version setup if deployed
+                        // from HEAD of the branch they are created from
+                        "0000000000000000000000000000000000000000" => None,
+                        v => Some(v.to_owned()),
+                    })
+                    .await?
+                }
+                (false, s) => {
+                    debug!("Using cached ic-admin matching the version of governance canister, path: {}", s);
+                    s
+                }
+            },
+            IcAdminVersion::Default => {
+                debug!("Using default ic-admin, version: {}", DEFAULT_IC_ADMIN_VERSION);
+                download_ic_admin(None).await?
+            }
+            IcAdminVersion::Strict(ver) => {
+                debug!("Using ic-admin specified via args: {}", ver);
+                download_ic_admin(Some(ver)).await?
             }
-            (false, s) => s,
         };
 
         let ic_admin = Some(Arc::new(IcAdminImpl::new(
@@ -253,6 +275,11 @@ impl DreContext {
     pub fn forum_post_link(&self) -> Option<String> {
         self.forum_post_link.clone()
     }
+
+    #[cfg(test)]
+    pub fn ic_admin_path(&self) -> Option<String> {
+        self.ic_admin_path.clone()
+    }
 }
 
 #[cfg(test)]

rs/cli/src/ic_admin.rs

@@ -767,7 +767,7 @@ pub struct ProposeOptions {
     pub motivation: Option<String>,
     pub forum_post_link: Option<String>,
 }
-const DEFAULT_IC_ADMIN_VERSION: &str = "0ca139ca39dfee21c8ca75e7fe37422df65e4b96";
+pub const DEFAULT_IC_ADMIN_VERSION: &str = "0ca139ca39dfee21c8ca75e7fe37422df65e4b96";
 
 fn get_ic_admin_revisions_dir() -> anyhow::Result<PathBuf> {
     let dir = dirs::home_dir()
@@ -817,7 +817,7 @@ pub async fn download_ic_admin(version: Option<String>) -> Result<String> {
             format!("https://download.dfinity.systems/ic/{version}/binaries/x86_64-linux/ic-admin.gz")
         };
         info!("Downloading ic-admin version: {} from {}", version, url);
-        let body = reqwest::get(url).await?.bytes().await?;
+        let body = reqwest::get(url).await?.error_for_status()?.bytes().await?;
         let mut decoded = GzDecoder::new(body.as_ref());
 
         let path_parent = path.parent().expect("path parent unwrap failed!");

rs/cli/src/unit_tests/ctx_init.rs

@@ -0,0 +1,137 @@
+use std::path::PathBuf;
+
+use crate::auth::Auth;
+use ic_canisters::governance::governance_canister_version;
+use ic_management_types::Network;
+
+use crate::{commands::IcAdminVersion, ctx::DreContext, ic_admin::DEFAULT_IC_ADMIN_VERSION};
+
+fn status_file_path() -> PathBuf {
+    dirs::home_dir().unwrap().join("bin").join("ic-admin.revisions").join("ic-admin.status")
+}
+
+fn get_deleted_status_file() -> PathBuf {
+    let status_file = status_file_path();
+    if status_file.exists() {
+        std::fs::remove_file(&status_file).unwrap()
+    }
+    status_file
+}
+
+async fn get_context(network: &Network, version: IcAdminVersion) -> anyhow::Result<DreContext> {
+    DreContext::new(
+        network.name.clone(),
+        network.nns_urls.clone(),
+        Auth::Anonymous,
+        None,
+        false,
+        true,
+        false,
+        true,
+        crate::commands::IcAdminRequirement::Detect,
+        None,
+        version,
+    )
+    .await
+}
+
+struct TestScenario<'a> {
+    name: &'static str,
+    version: IcAdminVersion,
+    should_delete_status_file: bool,
+    should_contain: Option<&'a str>,
+}
+
+impl<'a> TestScenario<'a> {
+    fn new(name: &'static str) -> Self {
+        Self {
+            name,
+            version: IcAdminVersion::FromGovernance,
+            should_delete_status_file: false,
+            should_contain: None,
+        }
+    }
+
+    fn version(self, version: IcAdminVersion) -> Self {
+        Self { version, ..self }
+    }
+
+    fn delete_status_file(self) -> Self {
+        Self {
+            should_delete_status_file: true,
+            ..self
+        }
+    }
+
+    fn should_contain(self, ver: &'a str) -> Self {
+        Self {
+            should_contain: Some(ver),
+            ..self
+        }
+    }
+}
+
+#[tokio::test]
+async fn init_tests_ic_admin_version() {
+    let version_on_s3 = "e47293c0bd7f39540245913f7f75be3d6863183c";
+    let mainnet = Network::mainnet_unchecked().unwrap();
+    let governance_version = governance_canister_version(&mainnet.nns_urls).await.unwrap();
+
+    let tests = &[
+        TestScenario::new("match governance canister")
+            .delete_status_file()
+            .should_contain(&governance_version.stringified_hash),
+        TestScenario::new("use default version")
+            .delete_status_file()
+            .version(IcAdminVersion::Default)
+            .should_contain(DEFAULT_IC_ADMIN_VERSION),
+        TestScenario::new("existing version on s3")
+            .delete_status_file()
+            .version(IcAdminVersion::Strict(version_on_s3.to_string()))
+            .should_contain(version_on_s3),
+        TestScenario::new("random version not present on s3").version(IcAdminVersion::Strict("random-version".to_string())),
+    ];
+
+    for test in tests {
+        let mut deleted_status_file: PathBuf = dirs::home_dir().unwrap();
+        if test.should_delete_status_file {
+            deleted_status_file = get_deleted_status_file();
+        }
+
+        let maybe_ctx = get_context(&mainnet, test.version.clone()).await;
+
+        if let Some(ver) = test.should_contain {
+            assert!(
+                maybe_ctx.is_ok(),
+                "Test `{}`: expected to create DreContext, but got error: {:?}",
+                test.name,
+                maybe_ctx.err().unwrap()
+            );
+            let ctx = maybe_ctx.unwrap();
+
+            let ic_admin_path = ctx.ic_admin_path().unwrap();
+            assert!(
+                ic_admin_path.contains(ver),
+                "Test `{}`: ic_admin_path `{}`, expected version `{}`",
+                test.name,
+                ic_admin_path,
+                ver
+            )
+        } else {
+            assert!(
+                maybe_ctx.is_err(),
+                "Test `{}`: expected error but got ok with version: {}",
+                test.name,
+                maybe_ctx.unwrap().ic_admin_path().unwrap()
+            );
+        }
+
+        if test.should_delete_status_file {
+            assert!(
+                deleted_status_file.exists(),
+                "Test `{}`: expected ic-admin.status file to be recreated, but it wasn't",
+                test.name
+            )
+        }
+    }
+}

rs/cli/src/unit_tests/mod.rs

@@ -1 +1,2 @@
+mod ctx_init;
 mod update_unassigned_nodes;

rs/qualifier/src/qualify_util.rs

@@ -85,6 +85,7 @@ pub async fn qualify(
         false,
         cmd.require_ic_admin(),
         None,
+        dre::commands::IcAdminVersion::FromGovernance,
     )
     .await?;