From 05dd2d07af55ab15c9d054aa6b1dbb7768d74dff Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Tue, 27 Feb 2024 11:09:24 +0100 Subject: [PATCH 1/4] saving progress --- rs/cli/src/cli.rs | 3 ++ rs/cli/src/ic_admin.rs | 90 ++++++++++++++++++++++++++++++++++++++++++ rs/cli/src/main.rs | 5 +++ 3 files changed, 98 insertions(+) diff --git a/rs/cli/src/cli.rs b/rs/cli/src/cli.rs index ebe278e16..6b49f3263 100644 --- a/rs/cli/src/cli.rs +++ b/rs/cli/src/cli.rs @@ -143,6 +143,9 @@ pub(crate) enum Commands { #[clap(long, env = "LOCAL_REGISTRY_PATH")] path: Option, }, + + /// Firewall rules + Firewall, } pub(crate) mod subnet { diff --git a/rs/cli/src/ic_admin.rs b/rs/cli/src/ic_admin.rs index 2b8c593ff..dfc5f45df 100644 --- a/rs/cli/src/ic_admin.rs +++ b/rs/cli/src/ic_admin.rs @@ -6,15 +6,20 @@ use flate2::read::GzDecoder; use futures::stream::{self, StreamExt}; use futures::Future; use ic_base_types::PrincipalId; +use ic_interfaces_registry::RegistryClient; use ic_management_backend::registry::{local_registry_path, RegistryFamilyEntries, RegistryState}; use ic_management_types::{Artifact, Network}; +use ic_protobuf::registry::firewall::v1::{FirewallRule, FirewallRuleSet}; use ic_protobuf::registry::subnet::v1::SubnetRecord; +use ic_registry_keys::make_firewall_rules_record_key; use ic_registry_local_registry::LocalRegistry; use itertools::Itertools; use log::{error, info, warn}; +use prost::Message; use regex::Regex; use reqwest::StatusCode; use sha2::{Digest, Sha256}; +use std::collections::BTreeMap; use std::fs::File; use std::io::Write; use std::os::unix::fs::PermissionsExt; @@ -585,6 +590,91 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa self.propose_run(command, options, simulate) } + + pub async fn update_replica_nodes_firewall(&self, network: Network) -> Result<(), Error> { + let local_registry_path = local_registry_path(network.clone()); + let local_registry = LocalRegistry::new(local_registry_path, Duration::from_secs(10)) + .map_err(|e| anyhow::anyhow!("Error in creating local registry instance: {:?}", e))?; + + local_registry + .sync_with_nns() + .await + .map_err(|e| anyhow::anyhow!("Error when syncing with NNS: {:?}", e))?; + + let value = local_registry + .get_value( + &make_firewall_rules_record_key(&ic_registry_keys::FirewallRulesScope::ReplicaNodes), + local_registry.get_latest_version(), + ) + .map_err(|e| anyhow::anyhow!("Error fetching firewall rules for replica nodes: {:?}", e))?; + + let rules = if let Some(value) = value { + FirewallRuleSet::decode(value.as_slice()) + .map_err(|e| anyhow::anyhow!("Failed to deserialize firewall ruleset: {:?}", e))? + } else { + FirewallRuleSet::default() + }; + + let rules: BTreeMap = rules + .entries + .iter() + .enumerate() + .sorted_by(|a, b| a.0.cmp(&b.0)) + .collect(); + + let mut builder = edit::Builder::new(); + let with_suffix = builder.suffix(".json"); + let pretty = serde_json::to_string_pretty(&rules) + .map_err(|e| anyhow::anyhow!("Error serializing ruleset to string: {:?}", e))?; + let edited: BTreeMap; + loop { + info!("Spawning edit window..."); + let edited_string = edit::edit_with_builder(pretty.clone(), &with_suffix)?; + match serde_json::from_str(&edited_string) { + Ok(ruleset) => { + edited = ruleset; + break; + } + Err(e) => { + warn!("Couldn't parse the input you provided, please retry. Error: {:?}", e); + } + } + } + + let mut added_entries: BTreeMap = BTreeMap::new(); + let mut updated_entries: BTreeMap = BTreeMap::new(); + for (key, rule) in edited.iter() { + if let Some(old_rule) = rules.get(key) { + if rule != *old_rule { + // Same key but different value meaning it was just updated + updated_entries.insert(*key, rule); + } + continue; + } + // Doesn't exist in old ones meaning it was just added + added_entries.insert(*key, rule); + } + + // Collect removed entries (keys from old set not present in new set) + let removed_entries: BTreeMap = + rules.into_iter().filter(|(key, _)| !edited.contains_key(key)).collect(); + + let added = serde_json::to_string_pretty(&added_entries).unwrap(); + info!("Pretty printing diff ----- added: \n{}", added); + + let updated = serde_json::to_string_pretty(&updated_entries).unwrap(); + info!("Pretty printing diff ----- updated: \n{}", updated); + + let removed = serde_json::to_string_pretty(&removed_entries).unwrap(); + info!("Pretty printing diff ----- removed: \n{}", removed); + + if added_entries.is_empty() && updated_entries.is_empty() && removed_entries.is_empty() { + info!("No modifications should be made"); + return Ok(()); + } + + Ok(()) + } } #[derive(Display, Clone)] diff --git a/rs/cli/src/main.rs b/rs/cli/src/main.rs index 095b10bf6..e9d456477 100644 --- a/rs/cli/src/main.rs +++ b/rs/cli/src/main.rs @@ -288,6 +288,11 @@ async fn main() -> Result<(), anyhow::Error> { cli::Commands::DumpRegistry { version, path } => { registry_dump::dump_registry(path, cli_opts.network, version).await } + + cli::Commands::Firewall => { + let ic_admin: IcAdminWrapper = cli::Cli::from_opts(&cli_opts, true).await?.into(); + ic_admin.update_replica_nodes_firewall(cli_opts.network).await + } } }) .await?; From 0d18bd9631db920cfa4cc2cf13fbc417e8d7a68b Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Thu, 29 Feb 2024 15:58:20 +0100 Subject: [PATCH 2/4] adding first iteration of firewall rules --- rs/cli/Cargo.toml | 2 +- rs/cli/src/ic_admin.rs | 151 ++++++++++++++++++++++++++++++++++++----- rs/cli/src/main.rs | 4 +- rs/cli/src/runner.rs | 6 +- 4 files changed, 141 insertions(+), 22 deletions(-) diff --git a/rs/cli/Cargo.toml b/rs/cli/Cargo.toml index 286e59f92..37176db62 100644 --- a/rs/cli/Cargo.toml +++ b/rs/cli/Cargo.toml @@ -56,7 +56,7 @@ tabled = { workspace = true } tabular = { workspace = true } tokio = { workspace = true } url = { workspace = true } +tempfile = "3.10.0" [dev-dependencies] -tempfile = "3.10.0" wiremock = "0.6.0" diff --git a/rs/cli/src/ic_admin.rs b/rs/cli/src/ic_admin.rs index dfc5f45df..25191419d 100644 --- a/rs/cli/src/ic_admin.rs +++ b/rs/cli/src/ic_admin.rs @@ -21,11 +21,13 @@ use reqwest::StatusCode; use sha2::{Digest, Sha256}; use std::collections::BTreeMap; use std::fs::File; -use std::io::Write; +use std::io::{Read, Write}; use std::os::unix::fs::PermissionsExt; +use std::process::Stdio; use std::time::Duration; use std::{path::Path, process::Command}; use strum::Display; +use tempfile::NamedTempFile; use crate::cli::Cli; use crate::detect_neuron::{Auth, Neuron}; @@ -86,8 +88,22 @@ impl IcAdminWrapper { .yellow(), ); } + pub(crate) fn propose_run_mapped( + &self, + cmd: ProposeCommand, + opts: ProposeOptions, + simulate: bool, + ) -> anyhow::Result<()> { + self.propose_run(cmd, opts, simulate)?; + return Ok(()); + } - pub(crate) fn propose_run(&self, cmd: ProposeCommand, opts: ProposeOptions, simulate: bool) -> anyhow::Result<()> { + pub(crate) fn propose_run( + &self, + cmd: ProposeCommand, + opts: ProposeOptions, + simulate: bool, + ) -> anyhow::Result { let exec = |cli: &IcAdminWrapper, cmd: ProposeCommand, opts: ProposeOptions, add_dryrun_arg: bool| { if let Some(summary) = opts.clone().summary { let summary_count = summary.chars().count(); @@ -162,7 +178,7 @@ impl IcAdminWrapper { } } - fn _run_ic_admin_with_args(&self, ic_admin_args: &[String], with_auth: bool) -> anyhow::Result<()> { + fn _run_ic_admin_with_args(&self, ic_admin_args: &[String], with_auth: bool) -> anyhow::Result { let ic_admin_path = self.ic_admin.clone().unwrap_or_else(|| "ic-admin".to_string()); let mut cmd = Command::new(ic_admin_path); let auth_options = if with_auth { @@ -174,12 +190,22 @@ impl IcAdminWrapper { let cmd = cmd.args([&root_options, ic_admin_args].concat()); self.print_ic_admin_command_line(cmd); + cmd.stdout(Stdio::piped()); match cmd.spawn() { Ok(mut child) => match child.wait() { Ok(s) => { if s.success() { - Ok(()) + if let Some(mut output) = child.stdout { + let mut readbuf = vec![]; + output + .read_to_end(&mut readbuf) + .map_err(|e| anyhow::anyhow!("Error reading output: {:?}", e))?; + let converted = String::from_utf8_lossy(&readbuf).to_string(); + println!("{}", converted); + return Ok(converted); + } + Ok("".to_string()) } else { Err(anyhow::anyhow!( "ic-admin failed with non-zero exit code {}", @@ -193,7 +219,7 @@ impl IcAdminWrapper { } } - pub(crate) fn run(&self, command: &str, args: &[String], with_auth: bool) -> anyhow::Result<()> { + pub(crate) fn run(&self, command: &str, args: &[String], with_auth: bool) -> anyhow::Result { let ic_admin_args = [&[command.to_string()], args].concat(); self._run_ic_admin_with_args(&ic_admin_args, with_auth) } @@ -255,7 +281,8 @@ impl IcAdminWrapper { args_with_get_prefix }; - self.run(&args[0], &args.iter().skip(1).cloned().collect::>(), false) + self.run(&args[0], &args.iter().skip(1).cloned().collect::>(), false)?; + Ok(()) } /// Run an `ic-admin propose-to-*` command directly @@ -303,7 +330,7 @@ impl IcAdminWrapper { args: args.iter().skip(1).cloned().collect::>(), }; let simulate = simulate || cmd.args().contains(&String::from("--dry-run")); - self.propose_run(cmd, Default::default(), simulate) + self.propose_run_mapped(cmd, Default::default(), simulate) } fn get_s3_cdn_image_url(version: &String, s3_subdir: &String) -> String { @@ -588,10 +615,10 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa title: Some("Update all unassigned nodes".to_string()), }; - self.propose_run(command, options, simulate) + self.propose_run_mapped(command, options, simulate) } - pub async fn update_replica_nodes_firewall(&self, network: Network) -> Result<(), Error> { + pub async fn update_replica_nodes_firewall(&self, network: Network, simulate: bool) -> Result<(), Error> { let local_registry_path = local_registry_path(network.clone()); let local_registry = LocalRegistry::new(local_registry_path, Duration::from_secs(10)) .map_err(|e| anyhow::anyhow!("Error in creating local registry instance: {:?}", e))?; @@ -659,20 +686,112 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa let removed_entries: BTreeMap = rules.into_iter().filter(|(key, _)| !edited.contains_key(key)).collect(); - let added = serde_json::to_string_pretty(&added_entries).unwrap(); - info!("Pretty printing diff ----- added: \n{}", added); - - let updated = serde_json::to_string_pretty(&updated_entries).unwrap(); - info!("Pretty printing diff ----- updated: \n{}", updated); + fn pretty_print_diff(change_type: &str, entries: &BTreeMap) { + if entries.is_empty() { + return; + } + let diff = serde_json::to_string_pretty(entries).unwrap(); + info!("Pretty printing diff ----- {}:\n{}", change_type, diff); + } - let removed = serde_json::to_string_pretty(&removed_entries).unwrap(); - info!("Pretty printing diff ----- removed: \n{}", removed); + pretty_print_diff("added", &added_entries); + pretty_print_diff("updated", &updated_entries); + pretty_print_diff("removed", &removed_entries); if added_entries.is_empty() && updated_entries.is_empty() && removed_entries.is_empty() { info!("No modifications should be made"); return Ok(()); } + if added_entries.len() + updated_entries.len() + removed_entries.len() > 1 { + return Err(anyhow::anyhow!( + "Cannot apply more than 1 change at a time due to hash changes" + )); + } + + //TODO: adapt to use set-firewall config so we can modify more than 1 rule at a time + + fn submit_proposals( + admin_wrapper: &IcAdminWrapper, + change_type: &str, + rules: BTreeMap, + simulate: bool, + ) -> anyhow::Result<()> { + if rules.is_empty() { + return Ok(()); + } + info!("Proceeding with creating proposals to '{}' rules", change_type); + for (position, rule) in rules { + let mut file = + NamedTempFile::new().map_err(|e| anyhow::anyhow!("Couldn't create temp file: {:?}", e))?; + + let array = vec![rule]; + let serialized = serde_json::to_string(&array).unwrap(); + + file.write_all(serialized.as_bytes()) + .map_err(|e| anyhow::anyhow!("Couldn't write to tempfile: {:?}", e))?; + + let cmd = ProposeCommand::Raw { + command: format!("{}-firewall-rules", change_type), + args: vec![ + "--test", + "replica_nodes", + file.path().to_str().unwrap(), + position.to_string().as_str(), + "none", + ] + .iter() + .map(|arg| arg.to_string()) + .collect(), + }; + + let output = admin_wrapper + .propose_run(cmd, ProposeOptions::default(), true) + .map_err(|e| { + anyhow::anyhow!("Couldn't execute test for {}-firewall-rules: {:?}", change_type, e) + })?; + + let parsed: serde_json::Value = serde_json::from_str(&output).map_err(|e| { + anyhow::anyhow!( + "Error deserializing --test output while performing '{}': {:?}", + change_type, + e + ) + })?; + let hash = match parsed.get("hash") { + Some(serde_json::Value::String(hash)) => hash, + _ => { + return Err(anyhow::anyhow!( + "Couldn't find string value for key 'hash'. Whole dump:\n{}", + serde_json::to_string_pretty(&parsed).unwrap() + )) + } + }; + info!("Computed hash for firewall rule at position '{}': {}", position, hash); + + let cmd = ProposeCommand::Raw { + command: format!("{}-firewall-rules", change_type), + args: vec![ + "replica_nodes".to_string(), + file.path().to_str().unwrap().to_string(), + position.to_string(), + hash.to_string(), + ], + }; + + let options = ProposeOptions { + title: Some(format!("Proposal to {} firewall rule", change_type)), + motivation: Some(format!("Proposal to {} firewall rule", change_type)), + summary: Some(format!("Proposal to {} firewall rule", change_type)), + }; + admin_wrapper.propose_run_mapped(cmd, options, simulate)? + } + Ok(()) + } + + submit_proposals(self, "add", added_entries, simulate)?; + submit_proposals(self, "update", updated_entries, simulate)?; + submit_proposals(self, "removed", removed_entries, simulate)?; Ok(()) } } diff --git a/rs/cli/src/main.rs b/rs/cli/src/main.rs index e9d456477..92e2b984b 100644 --- a/rs/cli/src/main.rs +++ b/rs/cli/src/main.rs @@ -218,7 +218,7 @@ async fn main() -> Result<(), anyhow::Error> { } }.await?; - ic_admin.propose_run(ic_admin::ProposeCommand::UpdateElectedVersions { + ic_admin.propose_run_mapped(ic_admin::ProposeCommand::UpdateElectedVersions { release_artifact: update_version.release_artifact.clone(), args: cli::Cli::get_update_cmd_args(&update_version) }, @@ -291,7 +291,7 @@ async fn main() -> Result<(), anyhow::Error> { cli::Commands::Firewall => { let ic_admin: IcAdminWrapper = cli::Cli::from_opts(&cli_opts, true).await?.into(); - ic_admin.update_replica_nodes_firewall(cli_opts.network).await + ic_admin.update_replica_nodes_firewall(cli_opts.network, cli_opts.simulate).await } } }) diff --git a/rs/cli/src/runner.rs b/rs/cli/src/runner.rs index 248e4bf1f..df0a318ae 100644 --- a/rs/cli/src/runner.rs +++ b/rs/cli/src/runner.rs @@ -106,7 +106,7 @@ impl Runner { .expect("Should get a replica version"), ); - self.ic_admin.propose_run( + self.ic_admin.propose_run_mapped( ic_admin::ProposeCommand::CreateSubnet { node_ids: subnet_creation_data.added, replica_version, @@ -163,7 +163,7 @@ impl Runner { } self.ic_admin - .propose_run( + .propose_run_mapped( ic_admin::ProposeCommand::ChangeSubnetMembership { subnet_id, node_ids_add: change.added.clone(), @@ -430,7 +430,7 @@ impl Runner { } println!("{}", table); - self.ic_admin.propose_run( + self.ic_admin.propose_run_mapped( ic_admin::ProposeCommand::RemoveNodes { nodes: node_removals.iter().map(|n| n.node.principal).collect(), }, From 43ed079eeeb860595b7f9444719c8161ee17fff9 Mon Sep 17 00:00:00 2001 From: nikolamilosa Date: Thu, 29 Feb 2024 16:11:28 +0100 Subject: [PATCH 3/4] removing unnecessary wrapper --- rs/cli/src/ic_admin.rs | 21 +++++++-------------- rs/cli/src/main.rs | 5 +++-- rs/cli/src/runner.rs | 15 +++++++++------ 3 files changed, 19 insertions(+), 22 deletions(-) diff --git a/rs/cli/src/ic_admin.rs b/rs/cli/src/ic_admin.rs index 25191419d..3e0621eab 100644 --- a/rs/cli/src/ic_admin.rs +++ b/rs/cli/src/ic_admin.rs @@ -88,15 +88,6 @@ impl IcAdminWrapper { .yellow(), ); } - pub(crate) fn propose_run_mapped( - &self, - cmd: ProposeCommand, - opts: ProposeOptions, - simulate: bool, - ) -> anyhow::Result<()> { - self.propose_run(cmd, opts, simulate)?; - return Ok(()); - } pub(crate) fn propose_run( &self, @@ -330,7 +321,8 @@ impl IcAdminWrapper { args: args.iter().skip(1).cloned().collect::>(), }; let simulate = simulate || cmd.args().contains(&String::from("--dry-run")); - self.propose_run_mapped(cmd, Default::default(), simulate) + self.propose_run(cmd, Default::default(), simulate)?; + Ok(()) } fn get_s3_cdn_image_url(version: &String, s3_subdir: &String) -> String { @@ -615,7 +607,8 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa title: Some("Update all unassigned nodes".to_string()), }; - self.propose_run_mapped(command, options, simulate) + self.propose_run(command, options, simulate)?; + Ok(()) } pub async fn update_replica_nodes_firewall(&self, network: Network, simulate: bool) -> Result<(), Error> { @@ -656,7 +649,7 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa let edited: BTreeMap; loop { info!("Spawning edit window..."); - let edited_string = edit::edit_with_builder(pretty.clone(), &with_suffix)?; + let edited_string = edit::edit_with_builder(pretty.clone(), with_suffix)?; match serde_json::from_str(&edited_string) { Ok(ruleset) => { edited = ruleset; @@ -733,7 +726,7 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa let cmd = ProposeCommand::Raw { command: format!("{}-firewall-rules", change_type), - args: vec![ + args: [ "--test", "replica_nodes", file.path().to_str().unwrap(), @@ -784,7 +777,7 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa motivation: Some(format!("Proposal to {} firewall rule", change_type)), summary: Some(format!("Proposal to {} firewall rule", change_type)), }; - admin_wrapper.propose_run_mapped(cmd, options, simulate)? + admin_wrapper.propose_run(cmd, options, simulate)?; } Ok(()) } diff --git a/rs/cli/src/main.rs b/rs/cli/src/main.rs index 92e2b984b..28e350576 100644 --- a/rs/cli/src/main.rs +++ b/rs/cli/src/main.rs @@ -218,7 +218,7 @@ async fn main() -> Result<(), anyhow::Error> { } }.await?; - ic_admin.propose_run_mapped(ic_admin::ProposeCommand::UpdateElectedVersions { + ic_admin.propose_run(ic_admin::ProposeCommand::UpdateElectedVersions { release_artifact: update_version.release_artifact.clone(), args: cli::Cli::get_update_cmd_args(&update_version) }, @@ -226,7 +226,8 @@ async fn main() -> Result<(), anyhow::Error> { title: Some(update_version.title), summary: Some(update_version.summary.clone()), motivation: None, - }, simulate) + }, simulate)?; + Ok(()) } } }, diff --git a/rs/cli/src/runner.rs b/rs/cli/src/runner.rs index df0a318ae..7a915c5af 100644 --- a/rs/cli/src/runner.rs +++ b/rs/cli/src/runner.rs @@ -106,7 +106,7 @@ impl Runner { .expect("Should get a replica version"), ); - self.ic_admin.propose_run_mapped( + self.ic_admin.propose_run( ic_admin::ProposeCommand::CreateSubnet { node_ids: subnet_creation_data.added, replica_version, @@ -117,7 +117,8 @@ impl Runner { motivation: Some(motivation.clone()), }, simulate, - ) + )?; + Ok(()) } pub async fn membership_replace( @@ -163,7 +164,7 @@ impl Runner { } self.ic_admin - .propose_run_mapped( + .propose_run( ic_admin::ProposeCommand::ChangeSubnetMembership { subnet_id, node_ids_add: change.added.clone(), @@ -172,7 +173,8 @@ impl Runner { options, simulate, ) - .map_err(|e| anyhow::anyhow!(e)) + .map_err(|e| anyhow::anyhow!(e))?; + Ok(()) } pub async fn new_with_network_url(ic_admin: ic_admin::IcAdminWrapper, backend_port: u16) -> anyhow::Result { @@ -430,7 +432,7 @@ impl Runner { } println!("{}", table); - self.ic_admin.propose_run_mapped( + self.ic_admin.propose_run( ic_admin::ProposeCommand::RemoveNodes { nodes: node_removals.iter().map(|n| n.node.principal).collect(), }, @@ -440,6 +442,7 @@ impl Runner { motivation: node_remove_response.motivation.into(), }, simulate, - ) + )?; + Ok(()) } } From bc5bb4d885c77908f7a9a22c8356c09bbf6e5f36 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Thu, 29 Feb 2024 15:20:51 +0000 Subject: [PATCH 4/4] Github Action: Bazel Repin --- Cargo.Bazel.lock | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.Bazel.lock b/Cargo.Bazel.lock index c1656aef1..ae02db263 100644 --- a/Cargo.Bazel.lock +++ b/Cargo.Bazel.lock @@ -1,5 +1,5 @@ { - "checksum": "872ac0e7ad6530e912d6f269ea87a37b5f210f23b0528c8c8e9d194d980e5c0a", + "checksum": "35baebb9ebb55e7994363dfedd074554d53eca611f932763d41e6846a346f08e", "crates": { "actix-codec 0.5.2": { "name": "actix-codec", @@ -11677,6 +11677,10 @@ "id": "tabular 0.2.0", "target": "tabular" }, + { + "id": "tempfile 3.10.0", + "target": "tempfile" + }, { "id": "tokio 1.36.0", "target": "tokio" @@ -11690,10 +11694,6 @@ }, "deps_dev": { "common": [ - { - "id": "tempfile 3.10.0", - "target": "tempfile" - }, { "id": "wiremock 0.6.0", "target": "wiremock"