From 382823b228c2d2fc7f0165f8e865a089bb194add Mon Sep 17 00:00:00 2001 From: Till Wegmueller Date: Sun, 22 Mar 2026 15:30:02 +0100 Subject: [PATCH] Improve DX: confirmations, progress, validate, completions Batch 1 of developer experience improvements: - Destroy requires typing zone name to confirm (or --yes/-y for scripts) - Non-interactive (non-TTY) mode refuses destructive ops without --yes - Create/destroy print numbered step-by-step progress - Partial failure during create prints cleanup guidance (orphaned VNICs etc) - New `zmgr validate` checks KDL syntax, referential integrity, gateway in network, range in network, duplicate IPs, pool utilization warnings - New `zmgr completions ` generates bash/zsh/fish completions - Added --version flag - Dry-run notes that IP allocation is tentative Co-Authored-By: Claude Opus 4.6 (1M context) --- Cargo.lock | 67 +++++++++++ Cargo.toml | 3 + src/main.rs | 291 +++++++++++++++++++++++++++++++++++++++++------- src/validate.rs | 241 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 560 insertions(+), 42 deletions(-) create mode 100644 src/validate.rs diff --git a/Cargo.lock b/Cargo.lock index d9f1596..374aa1d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -169,6 +169,15 @@ dependencies = [ "strsim", ] +[[package]] +name = "clap_complete" +version = "4.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "19c9f1dde76b736e3681f28cec9d5a61299cbaae0fce80a68e43724ad56031eb" +dependencies = [ + "clap", +] + [[package]] name = "clap_derive" version = "4.6.0" @@ -269,6 +278,12 @@ version = "1.70.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a6cb138bb79a146c1bd460005623e142ef0181e3d0219cb493e02f7d08a35695" +[[package]] +name = "itoa" +version = "1.0.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" + [[package]] name = "js-sys" version = "0.3.91" @@ -496,6 +511,49 @@ version = "1.0.22" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b39cdef0fa800fc44525c84ccb54a029961a8215f9619753635a9c0d2538d46d" +[[package]] +name = "serde" +version = "1.0.228" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a8e94ea7f378bd32cbbd37198a4a91436180c5bb472411e48b5ec2e2124ae9e" +dependencies = [ + "serde_core", + "serde_derive", +] + +[[package]] +name = "serde_core" +version = "1.0.228" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41d385c7d4ca58e59fc732af25c3983b67ac852c1a25000afe1175de458b67ad" +dependencies = [ + "serde_derive", +] + +[[package]] +name = "serde_derive" +version = "1.0.228" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d540f220d3187173da220f885ab66608367b6574e925011a9353e4badda91d79" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "serde_json" +version = "1.0.149" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "83fc039473c5595ace860d8c4fafa220ff474b3fc6bfdb4293327f1a37e94d86" +dependencies = [ + "itoa", + "memchr", + "serde", + "serde_core", + "zmij", +] + [[package]] name = "shlex" version = "1.3.0" @@ -812,8 +870,17 @@ version = "0.1.0" dependencies = [ "chrono", "clap", + "clap_complete", "ipnet", "kdl", "miette", + "serde", + "serde_json", "thiserror", ] + +[[package]] +name = "zmij" +version = "1.0.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b8848ee67ecc8aedbaf3e4122217aff892639231befc6a1b58d29fff4c2cabaa" diff --git a/Cargo.toml b/Cargo.toml index 1c6fa86..b0717a6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,10 @@ edition = "2024" [dependencies] chrono = "0.4.44" clap = { version = "4.6.0", features = ["derive"] } +clap_complete = "4.6.0" ipnet = "2.12.0" kdl = "6.5.0" miette = { version = "7.6.0", features = ["fancy"] } +serde = { version = "1.0.228", features = ["derive"] } +serde_json = "1.0.149" thiserror = "2.0.18" diff --git a/src/main.rs b/src/main.rs index 0a8716f..8f0f507 100644 --- a/src/main.rs +++ b/src/main.rs @@ -10,13 +10,16 @@ mod kdl_util; mod pool; mod publisher; mod template; +mod validate; mod zone; use std::collections::HashSet; +use std::io::{IsTerminal, Write}; use std::net::Ipv4Addr; use std::path::Path; -use clap::{Parser, Subcommand}; +use clap::{CommandFactory, Parser, Subcommand}; +use clap_complete::Shell; use crate::config::Config; use crate::error::{Result, ZmgrError}; @@ -26,7 +29,7 @@ use crate::template::Template; use crate::zone::{Zone, ZoneNet}; #[derive(Parser)] -#[command(name = "zmgr", about = "illumos zone manager")] +#[command(name = "zmgr", version, about = "illumos zone manager")] struct Cli { /// Registry path (default: /etc/zmgr) #[arg(long, default_value = "/etc/zmgr")] @@ -36,6 +39,10 @@ struct Cli { #[arg(long, short = 'n')] dry_run: bool, + /// Skip confirmation prompts (for scripting) + #[arg(long, short = 'y')] + yes: bool, + #[command(subcommand)] command: Commands, } @@ -73,6 +80,8 @@ enum Commands { /// Only import this specific zone name: Option, }, + /// Validate registry configuration + Validate, /// Manage templates Template { #[command(subcommand)] @@ -88,6 +97,11 @@ enum Commands { #[command(subcommand)] action: PublisherAction, }, + /// Generate shell completions + Completions { + /// Shell to generate completions for + shell: Shell, + }, } #[derive(Subcommand)] @@ -129,16 +143,18 @@ fn main() -> miette::Result<()> { let registry = std::path::PathBuf::from(&cli.registry); let dry_run = cli.dry_run; + let yes = cli.yes; match cli.command { Commands::Init { force } => cmd_init(®istry, force), Commands::Create { name, template } => { cmd_create(®istry, &name, template.as_deref(), dry_run) } - Commands::Destroy { name } => cmd_destroy(®istry, &name, dry_run), + Commands::Destroy { name } => cmd_destroy(®istry, &name, dry_run, yes), Commands::List => cmd_list(®istry), Commands::Status { name } => cmd_status(®istry, name.as_deref()), Commands::Import { name } => cmd_import(®istry, name.as_deref()), + Commands::Validate => cmd_validate(®istry), Commands::Template { action } => match action { TemplateAction::List => cmd_template_list(®istry), TemplateAction::Show { name } => cmd_template_show(®istry, &name), @@ -152,9 +168,53 @@ fn main() -> miette::Result<()> { PublisherAction::Add { name, origin } => cmd_publisher_add(®istry, &name, &origin), PublisherAction::Remove { name } => cmd_publisher_remove(®istry, &name), }, + Commands::Completions { shell } => { + clap_complete::generate( + shell, + &mut Cli::command(), + "zmgr", + &mut std::io::stdout(), + ); + Ok(()) + } } } +// --- Confirmation helpers --- + +/// Ask the user to type the zone name to confirm a destructive operation. +/// Returns Ok(true) if confirmed, Ok(false) if declined. +fn confirm_destroy(name: &str, zone: &Zone) -> Result { + if !std::io::stdin().is_terminal() { + eprintln!("error: destructive operation requires --yes (-y) in non-interactive mode"); + return Ok(false); + } + + println!("Zone '{name}' will be permanently destroyed:"); + for net in &zone.nets { + println!(" net \"{}\": {} via {}", net.name, net.address, net.vnic); + } + println!(); + println!(" - zoneadm halt + uninstall"); + println!(" - zonecfg delete"); + for net in &zone.nets { + println!(" - dladm delete-vnic {}", net.vnic); + } + println!(" - remove registry entry"); + println!(); + print!("Type zone name to confirm: "); + std::io::stdout().flush().map_err(ZmgrError::Io)?; + + let mut input = String::new(); + std::io::stdin() + .read_line(&mut input) + .map_err(ZmgrError::Io)?; + + Ok(input.trim() == name) +} + +// --- Commands --- + fn cmd_init(registry: &Path, force: bool) -> Result<()> { if registry.join("config.kdl").exists() && !force { return Err(ZmgrError::AlreadyInitialized { @@ -214,13 +274,17 @@ fn cmd_create( let gateway = pool.gateway.to_string(); let stub = pool.stub.clone(); - zone_nets.push((tnet, pool, ZoneNet { - name: tnet.name.clone(), - address, - gateway, - vnic, - stub, - })); + zone_nets.push(( + tnet, + pool, + ZoneNet { + name: tnet.name.clone(), + address, + gateway, + vnic, + stub, + }, + )); } // Build zonecfg commands @@ -282,23 +346,72 @@ fn cmd_create( println!(); println!("Registry entry that would be written:"); println!(" {}/zones/{name}.kdl", registry.display()); + println!(); + println!("Note: IP allocation is tentative. Actual address may differ if"); + println!("other zones are created first."); return Ok(()); } + // --- Execute with step-by-step progress --- + + let total_steps = zone_nets.len() + 3 + if tmpl.autoboot { 1 } else { 0 }; + let mut step = 0; + + println!("Creating zone '{name}' (template: {tmpl_name})..."); + + // Track created VNICs for rollback + let mut created_vnics: Vec = Vec::new(); + // Create VNICs for (_, _, znet) in &zone_nets { - exec::dladm_create_vnic(&znet.vnic, &znet.stub)?; + step += 1; + print!(" [{step}/{total_steps}] Creating VNIC {} on {}... ", znet.vnic, znet.stub); + std::io::stdout().flush().map_err(ZmgrError::Io)?; + match exec::dladm_create_vnic(&znet.vnic, &znet.stub) { + Ok(()) => { + println!("done"); + created_vnics.push(znet.vnic.clone()); + } + Err(e) => { + println!("FAILED"); + print_partial_failure(name, &created_vnics, false, false); + return Err(e); + } + } } - exec::zonecfg_create(name, &zonecfg_cmds)?; - exec::zoneadm_install(name)?; + // Configure zone + step += 1; + print!(" [{step}/{total_steps}] Configuring zone via zonecfg... "); + std::io::stdout().flush().map_err(ZmgrError::Io)?; + match exec::zonecfg_create(name, &zonecfg_cmds) { + Ok(()) => println!("done"), + Err(e) => { + println!("FAILED"); + print_partial_failure(name, &created_vnics, false, false); + return Err(e); + } + } + + // Install zone + step += 1; + print!(" [{step}/{total_steps}] Installing zone (this may take a while)... "); + std::io::stdout().flush().map_err(ZmgrError::Io)?; + match exec::zoneadm_install(name) { + Ok(()) => println!("done"), + Err(e) => { + println!("FAILED"); + print_partial_failure(name, &created_vnics, true, false); + return Err(e); + } + } // Write registry entry + step += 1; + print!(" [{step}/{total_steps}] Writing registry entry... "); + std::io::stdout().flush().map_err(ZmgrError::Io)?; let today = chrono::Local::now().format("%Y-%m-%d").to_string(); - let nets: Vec = zone_nets - .into_iter() - .map(|(_, _, znet)| znet) - .collect(); + let nets: Vec = zone_nets.into_iter().map(|(_, _, znet)| znet).collect(); let zone = Zone { name: name.to_string(), template: tmpl_name.to_string(), @@ -306,26 +419,61 @@ fn cmd_create( created: today, }; zone.write(registry)?; + println!("done"); - println!("Created zone '{name}'"); - println!(" template: {tmpl_name}"); - println!(" brand: {}", tmpl.brand); - for net in &zone.nets { - println!(" net \"{}\": {} via {} ({})", net.name, net.address, net.vnic, net.gateway); + // Boot if autoboot + if tmpl.autoboot { + step += 1; + print!(" [{step}/{total_steps}] Booting zone... "); + std::io::stdout().flush().map_err(ZmgrError::Io)?; + exec::zoneadm_boot(name)?; + println!("done"); } - if tmpl.autoboot { - exec::zoneadm_boot(name)?; - println!(" status: booted"); - } else { - println!(" status: installed (autoboot=false)"); - println!(" hint: run `zoneadm -z {name} boot` to start"); + println!(); + println!("Zone '{name}' created successfully."); + for net in &zone.nets { + println!( + " net \"{}\": {} via {} (gw {})", + net.name, net.address, net.vnic, net.gateway + ); } Ok(()) } -fn cmd_destroy(registry: &Path, name: &str, dry_run: bool) -> Result<()> { +/// Print cleanup guidance after a partial failure during create. +fn print_partial_failure( + name: &str, + created_vnics: &[String], + zone_configured: bool, + zone_installed: bool, +) { + eprintln!(); + eprintln!("Partial state left behind:"); + if zone_installed { + eprintln!(" - zone '{name}' is installed"); + } + if zone_configured { + eprintln!(" - zone '{name}' is configured in zonecfg"); + } + for vnic in created_vnics { + eprintln!(" - VNIC '{vnic}' was created"); + } + eprintln!(); + eprintln!("To clean up manually:"); + if zone_installed { + eprintln!(" zoneadm -z {name} uninstall -F"); + } + if zone_configured { + eprintln!(" echo 'delete -F' | zonecfg -z {name}"); + } + for vnic in created_vnics { + eprintln!(" dladm delete-vnic {vnic}"); + } +} + +fn cmd_destroy(registry: &Path, name: &str, dry_run: bool, yes: bool) -> Result<()> { config::ensure_initialized(registry)?; let zone = Zone::load(registry, name)?; @@ -335,7 +483,7 @@ fn cmd_destroy(registry: &Path, name: &str, dry_run: bool) -> Result<()> { println!(); println!(" template: {}", zone.template); for net in &zone.nets { - println!(" net \"{}\": {} via {}", net.name, net.address, net.vnic); + println!(" net \"{}\": {} via {}", net.name, net.address, net.vnic); } println!(); println!("Commands that would be executed:"); @@ -352,30 +500,77 @@ fn cmd_destroy(registry: &Path, name: &str, dry_run: bool) -> Result<()> { return Ok(()); } - // Halt if running (ignore errors — zone may already be halted) - let _ = exec::zoneadm_halt(name); + // Require confirmation unless --yes + if !yes { + if !confirm_destroy(name, &zone)? { + println!("Aborted."); + return Ok(()); + } + println!(); + } + + let vnic_count = zone.nets.len(); + let total_steps = 3 + vnic_count + 1; // halt+uninstall+zonecfg + vnics + registry + let mut step = 0; + + println!("Destroying zone '{name}'..."); + + // Halt + step += 1; + print!(" [{step}/{total_steps}] Halting zone... "); + std::io::stdout().flush().map_err(ZmgrError::Io)?; + match exec::zoneadm_halt(name) { + Ok(()) => println!("done"), + Err(_) => println!("skipped (not running)"), + } // Uninstall - if let Err(e) = exec::zoneadm_uninstall(name) { - eprintln!("warning: zoneadm uninstall: {e}"); + step += 1; + print!(" [{step}/{total_steps}] Uninstalling zone... "); + std::io::stdout().flush().map_err(ZmgrError::Io)?; + match exec::zoneadm_uninstall(name) { + Ok(()) => println!("done"), + Err(e) => { + println!("warning"); + eprintln!(" {e}"); + } } // Delete zonecfg - if let Err(e) = exec::zonecfg_delete(name) { - eprintln!("warning: zonecfg delete: {e}"); + step += 1; + print!(" [{step}/{total_steps}] Deleting zone configuration... "); + std::io::stdout().flush().map_err(ZmgrError::Io)?; + match exec::zonecfg_delete(name) { + Ok(()) => println!("done"), + Err(e) => { + println!("warning"); + eprintln!(" {e}"); + } } - // Delete all VNICs + // Delete VNICs for net in &zone.nets { - if let Err(e) = exec::dladm_delete_vnic(&net.vnic) { - eprintln!("warning: dladm delete-vnic {}: {e}", net.vnic); + step += 1; + print!(" [{step}/{total_steps}] Deleting VNIC {}... ", net.vnic); + std::io::stdout().flush().map_err(ZmgrError::Io)?; + match exec::dladm_delete_vnic(&net.vnic) { + Ok(()) => println!("done"), + Err(e) => { + println!("warning"); + eprintln!(" {e}"); + } } } // Remove from registry + step += 1; + print!(" [{step}/{total_steps}] Removing registry entry... "); + std::io::stdout().flush().map_err(ZmgrError::Io)?; Zone::delete(registry, name)?; + println!("done"); - println!("Destroyed zone '{name}'"); + println!(); + println!("Zone '{name}' destroyed."); Ok(()) } @@ -469,7 +664,8 @@ fn cmd_status(registry: &Path, name: Option<&str>) -> Result<()> { let addrs: Vec<&str> = r.nets.iter().map(|n| n.address.as_str()).collect(); println!( "{:<20} {:<10} {:<12} {:<10} {}", - r.name, r.template, state, "yes", addrs.join(", ") + r.name, r.template, state, "yes", + addrs.join(", ") ); seen.insert(r.name.clone()); } @@ -507,6 +703,18 @@ fn cmd_import(registry: &Path, name: Option<&str>) -> Result<()> { Ok(()) } +fn cmd_validate(registry: &Path) -> Result<()> { + config::ensure_initialized(registry)?; + + println!("Validating registry at {}...", registry.display()); + let ok = validate::run(registry)?; + + if !ok { + std::process::exit(1); + } + Ok(()) +} + fn cmd_template_list(registry: &Path) -> Result<()> { config::ensure_initialized(registry)?; @@ -587,7 +795,6 @@ fn cmd_pool_show(registry: &Path, name: &str) -> Result<()> { println!(" stub: {}", pool.stub); println!(" addresses: {}", pool.source_description()); - // Find allocations: zone nets whose IP falls in this pool let mut allocations = Vec::new(); for z in &zones { for net in &z.nets { diff --git a/src/validate.rs b/src/validate.rs new file mode 100644 index 0000000..116c7cd --- /dev/null +++ b/src/validate.rs @@ -0,0 +1,241 @@ +use std::collections::HashMap; +use std::net::Ipv4Addr; +use std::path::Path; + +use crate::config::Config; +use crate::error::Result; +use crate::pool::Pool; +use crate::template::Template; +use crate::zone::Zone; + +struct ValidationResult { + errors: Vec, + warnings: Vec, +} + +impl ValidationResult { + fn new() -> Self { + Self { + errors: Vec::new(), + warnings: Vec::new(), + } + } + + fn error(&mut self, msg: impl Into) { + self.errors.push(msg.into()); + } + + fn warn(&mut self, msg: impl Into) { + self.warnings.push(msg.into()); + } +} + +/// Run all validation checks and print results. Returns true if no errors. +pub fn run(registry: &Path) -> Result { + let mut result = ValidationResult::new(); + + // 1. Config + print!(" config.kdl "); + match Config::load(registry) { + Ok(cfg) => { + println!("{:.<30} ok", ""); + validate_config(&cfg, registry, &mut result); + } + Err(e) => { + println!("{:.<30} ERROR", ""); + result.error(format!("config.kdl: {e}")); + } + } + + // 2. Templates + let template_names = Template::list(registry).unwrap_or_default(); + let mut templates: HashMap = HashMap::new(); + for name in &template_names { + let label = format!("templates/{name}.kdl "); + print!(" {label}"); + match Template::load(registry, name) { + Ok(tmpl) => { + println!("{:.<30} ok", ""); + templates.insert(name.clone(), tmpl); + } + Err(e) => { + println!("{:.<30} ERROR", ""); + result.error(format!("templates/{name}.kdl: {e}")); + } + } + } + + // 3. Pools + let pool_names = Pool::list(registry).unwrap_or_default(); + let mut pools: HashMap = HashMap::new(); + for name in &pool_names { + let label = format!("pools/{name}.kdl "); + print!(" {label}"); + match Pool::load(registry, name) { + Ok(pool) => { + println!("{:.<30} ok", ""); + pools.insert(name.clone(), pool); + } + Err(e) => { + println!("{:.<30} ERROR", ""); + result.error(format!("pools/{name}.kdl: {e}")); + } + } + } + + // 4. Zones + let zones = Zone::list(registry).unwrap_or_default(); + let zones_dir = registry.join("zones"); + if zones_dir.exists() { + let count = zones.len(); + print!(" zones/ ({count} zones) "); + println!("{:.<25} ok", ""); + } + + // --- Cross-file validation --- + + // Template → Pool referential integrity + for (tname, tmpl) in &templates { + for net in &tmpl.nets { + if !pools.contains_key(&net.pool) { + result.error(format!( + "templates/{tname}.kdl: net '{}' references pool '{}' which does not exist", + net.name, net.pool + )); + } + } + } + + // Config → Template referential integrity + if let Ok(cfg) = Config::load(registry) { + if !templates.contains_key(&cfg.default_template) && !template_names.is_empty() { + result.warn(format!( + "config.kdl: default-template '{}' does not exist", + cfg.default_template + )); + } + } + + // Pool sanity checks + for (pname, pool) in &pools { + // Gateway in network + if !pool.network.contains(&pool.gateway) { + result.error(format!( + "pools/{pname}.kdl: gateway {} is not in network {}", + pool.gateway, pool.network + )); + } + + // Range/addresses within network + match &pool.source { + crate::pool::AddressSource::Range { start, end } => { + if !pool.network.contains(start) { + result.error(format!( + "pools/{pname}.kdl: range-start {start} is not in network {}", + pool.network + )); + } + if !pool.network.contains(end) { + result.error(format!( + "pools/{pname}.kdl: range-end {end} is not in network {}", + pool.network + )); + } + if u32::from(*start) > u32::from(*end) { + result.error(format!( + "pools/{pname}.kdl: range-start {start} is after range-end {end}" + )); + } + } + crate::pool::AddressSource::List(addrs) => { + for addr in addrs { + if !pool.network.contains(addr) { + result.error(format!( + "pools/{pname}.kdl: address {addr} is not in network {}", + pool.network + )); + } + } + } + } + + // Pool utilization warning + let used: usize = zones + .iter() + .flat_map(|z| &z.nets) + .filter_map(|n| { + n.address + .split('/') + .next() + .and_then(|s| s.parse::().ok()) + }) + .filter(|ip| pool.network.contains(ip)) + .count(); + let total = pool.total_addresses() as usize; + if total > 0 { + let pct = (used * 100) / total; + if pct >= 90 { + result.warn(format!( + "pools/{pname}.kdl: {used}/{total} addresses allocated ({pct}%)" + )); + } + } + } + + // Duplicate IP check across all zones + let mut ip_owners: HashMap> = HashMap::new(); + for z in &zones { + for net in &z.nets { + if let Some(ip) = net + .address + .split('/') + .next() + .and_then(|s| s.parse::().ok()) + { + ip_owners + .entry(ip) + .or_default() + .push(format!("{}:{}", z.name, net.name)); + } + } + } + for (ip, owners) in &ip_owners { + if owners.len() > 1 { + result.error(format!( + "zones: duplicate IP {ip} allocated to: {}", + owners.join(", ") + )); + } + } + + // Print summary + println!(); + if result.warnings.is_empty() && result.errors.is_empty() { + println!("No issues found."); + } else { + for w in &result.warnings { + println!(" warning: {w}"); + } + for e in &result.errors { + println!(" error: {e}"); + } + println!(); + println!( + "{} error(s), {} warning(s)", + result.errors.len(), + result.warnings.len() + ); + } + + Ok(result.errors.is_empty()) +} + +fn validate_config(cfg: &Config, _registry: &Path, result: &mut ValidationResult) { + // Check zonepath prefix is absolute + if !cfg.zonepath_prefix.starts_with('/') { + result.warn(format!( + "config.kdl: zonepath-prefix '{}' is not an absolute path", + cfg.zonepath_prefix + )); + } +}