diff --git a/rs/cli/src/cli.rs b/rs/cli/src/cli.rs index c80444abb..3eb58c243 100644 --- a/rs/cli/src/cli.rs +++ b/rs/cli/src/cli.rs @@ -145,7 +145,14 @@ pub(crate) enum Commands { }, /// Firewall rules - Firewall, + Firewall { + #[clap(long, default_value = Some("Proposal to modify firewall rules"))] + title: Option, + #[clap(long, default_value = None)] + summary: Option, + #[clap(long, default_value = None)] + motivation: Option, + }, } pub(crate) mod subnet { diff --git a/rs/cli/src/ic_admin.rs b/rs/cli/src/ic_admin.rs index f7d53be2a..2719ce164 100644 --- a/rs/cli/src/ic_admin.rs +++ b/rs/cli/src/ic_admin.rs @@ -18,14 +18,16 @@ use log::{error, info, warn}; use prost::Message; use regex::Regex; use reqwest::StatusCode; +use serde::Serialize; use sha2::{Digest, Sha256}; use std::collections::BTreeMap; +use std::fmt; use std::fs::File; 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 std::{fmt::Display, path::Path, process::Command}; use strum::Display; use tempfile::NamedTempFile; @@ -35,6 +37,105 @@ use crate::{cli, defaults}; const MAX_SUMMARY_CHAR_COUNT: usize = 14000; +#[derive(Clone, Serialize, PartialEq)] +enum FirewallRuleModificationType { + Addition, + Update, + Removal, +} + +impl Display for FirewallRuleModificationType { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::Addition => write!(f, "add"), + Self::Update => write!(f, "update"), + Self::Removal => write!(f, "remove"), + } + } +} + +#[derive(Clone, Serialize)] +struct FirewallRuleModification { + change_type: FirewallRuleModificationType, + rule_being_modified: FirewallRule, + position: usize, +} + +impl FirewallRuleModification { + fn addition(position: usize, rule: FirewallRule) -> Self { + Self { + change_type: FirewallRuleModificationType::Addition, + rule_being_modified: rule, + position, + } + } + fn update(position: usize, rule: FirewallRule) -> Self { + Self { + change_type: FirewallRuleModificationType::Update, + rule_being_modified: rule, + position, + } + } + fn removal(position: usize, rule: FirewallRule) -> Self { + Self { + change_type: FirewallRuleModificationType::Removal, + rule_being_modified: rule, + position, + } + } +} + +struct FirewallRuleModifications { + raw: Vec, +} + +impl FirewallRuleModifications { + fn new() -> Self { + FirewallRuleModifications { raw: vec![] } + } + + fn addition(&mut self, position: usize, rule: FirewallRule) { + self.raw.push(FirewallRuleModification::addition(position, rule)) + } + + fn update(&mut self, position: usize, rule: FirewallRule) { + self.raw.push(FirewallRuleModification::update(position, rule)) + } + + fn removal(&mut self, position: usize, rule: FirewallRule) { + self.raw.push(FirewallRuleModification::removal(position, rule)) + } + + fn reverse_sorted(&self) -> Vec { + let mut sorted = self.raw.to_vec(); + sorted.sort_by(|first, second| first.position.partial_cmp(&second.position).unwrap()); + sorted.reverse(); + sorted + } + + fn reverse_sorted_and_batched(&self) -> Vec<(FirewallRuleModificationType, Vec)> { + let mut batches: Vec<(FirewallRuleModificationType, Vec)> = vec![]; + let mut current_batch: Vec = vec![]; + let mut modtype: Option = None; + for modif in self.reverse_sorted().iter() { + if modtype.is_none() { + modtype = Some(modif.clone().change_type); + } + if modtype.clone().unwrap() == modif.change_type { + current_batch.push(modif.clone()) + } else { + batches.push((current_batch[0].clone().change_type, current_batch)); + current_batch = vec![]; + modtype = Some(modif.clone().change_type); + } + } + if !current_batch.is_empty() { + batches.push((current_batch[0].clone().change_type, current_batch)) + } + batches + } +} + #[derive(Clone)] pub struct IcAdminWrapper { ic_admin: Option, @@ -611,7 +712,12 @@ must be identical, and must match the SHA256 from the payload of the NNS proposa Ok(()) } - pub async fn update_replica_nodes_firewall(&self, network: Network, simulate: bool) -> Result<(), Error> { + pub async fn update_replica_nodes_firewall( + &self, + network: Network, + propose_options: ProposeOptions, + 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))?; @@ -679,112 +785,116 @@ 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(); - 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 mut mods = FirewallRuleModifications::new(); + for (pos, rule) in added_entries.into_iter() { + mods.addition(pos, rule.clone()); + } + for (pos, rule) in updated_entries.into_iter() { + mods.update(pos, rule.clone()); + } + for (pos, rule) in removed_entries.into_iter() { + mods.removal(pos, rule.clone()); } - 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() { + let reverse_sorted = mods.reverse_sorted_and_batched(); + if reverse_sorted.is_empty() { info!("No modifications should be made"); return Ok(()); } - - if added_entries.len() + updated_entries.len() + removed_entries.len() > 1 { + let diff = serde_json::to_string_pretty(&reverse_sorted).unwrap(); + info!("Pretty printing diff:\n{}", diff); + /*if reverse_sorted.len() > 1 { return Err(anyhow::anyhow!( - "Cannot apply more than 1 change at a time due to hash changes" + "Cannot currently 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( + fn submit_proposal( admin_wrapper: &IcAdminWrapper, - change_type: &str, - rules: BTreeMap, + modifications: Vec, + propose_options: ProposeOptions, 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: [ - "--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![ + let positions = modifications.iter().map(|modif| modif.position).join(","); + let change_type = modifications[0].clone().change_type; + + let mut file = NamedTempFile::new().map_err(|e| anyhow::anyhow!("Couldn't create temp file: {:?}", e))?; + + let test_args = match change_type { + FirewallRuleModificationType::Removal => vec![ + "--test".to_string(), + "replica_nodes".to_string(), + positions.to_string(), + "none".to_string(), + ], + _ => { + let rules = modifications + .iter() + .map(|modif| modif.clone().rule_being_modified) + .collect::>(); + let serialized = serde_json::to_string(&rules).unwrap(); + file.write_all(serialized.as_bytes()) + .map_err(|e| anyhow::anyhow!("Couldn't write to tempfile: {:?}", e))?; + vec![ + "--test".to_string(), "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(cmd, options, simulate)?; - } + positions.to_string(), + "none".to_string(), + ] + } + }; + + let cmd = ProposeCommand::Raw { + command: format!("{}-firewall-rules", change_type), + args: test_args.clone(), + }; + + let output = admin_wrapper + .propose_run(cmd, propose_options.clone(), 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 '{}': {}", positions, hash); + + let mut final_args = test_args.clone(); + // Remove --test from head of args. + let _ = final_args.remove(0); + // Add the real hash to args. + let last = final_args.last_mut().unwrap(); + *last = hash.to_string(); + + let cmd = ProposeCommand::Raw { + command: format!("{}-firewall-rules", change_type), + args: final_args, + }; + + admin_wrapper.propose_run(cmd, propose_options.clone(), simulate)?; + Ok(()) } - submit_proposals(self, "add", added_entries, simulate)?; - submit_proposals(self, "update", updated_entries, simulate)?; - submit_proposals(self, "removed", removed_entries, simulate)?; + for (_, mods) in reverse_sorted.into_iter() { + submit_proposal(self, mods, propose_options.clone(), simulate)?; + break; // no more than one rule mod implemented currenty -- FIXME + } Ok(()) } } diff --git a/rs/cli/src/main.rs b/rs/cli/src/main.rs index ed0901104..314a45d81 100644 --- a/rs/cli/src/main.rs +++ b/rs/cli/src/main.rs @@ -293,9 +293,13 @@ async fn main() -> Result<(), anyhow::Error> { registry_dump::dump_registry(path, cli_opts.network, version).await } - cli::Commands::Firewall => { + cli::Commands::Firewall{title, summary, motivation} => { let ic_admin: IcAdminWrapper = cli::Cli::from_opts(&cli_opts, true).await?.into(); - ic_admin.update_replica_nodes_firewall(cli_opts.network, cli_opts.simulate).await + ic_admin.update_replica_nodes_firewall(cli_opts.network, ic_admin::ProposeOptions{ + title: title.clone(), + summary: summary.clone(), + motivation: motivation.clone(), + }, cli_opts.simulate).await } } })