From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 629361FF2C6 for ; Wed, 10 Jul 2024 14:18:35 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2A3FB8CCC; Wed, 10 Jul 2024 14:18:57 +0200 (CEST) Date: Wed, 10 Jul 2024 14:18:19 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20240710092008.25271-1-f.schauer@proxmox.com> <20240710092008.25271-3-f.schauer@proxmox.com> In-Reply-To: <20240710092008.25271-3-f.schauer@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1720613235.4f6jsbkwyr.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.050 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [vma2pbs.rs, main.rs, proxmox.com] Subject: Re: [pbs-devel] [PATCH vma-to-pbs v2 2/2] add support for notes and logs X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On July 10, 2024 11:20 am, Filip Schauer wrote: > Allow the user to specify a notes file and a log file to associate with > the backup > > Signed-off-by: Filip Schauer > --- > Use the proxmox-backup crates from the proxmox-backup-qemu submodule, > because a seperate proxmox-backup submodule leads to "package collision > in the lockfile" > > Cargo.toml | 8 +++ > src/main.rs | 16 ++++++ > src/vma2pbs.rs | 132 +++++++++++++++++++++++++++++++++++++++++++++---- > 3 files changed, 146 insertions(+), 10 deletions(-) > > diff --git a/Cargo.toml b/Cargo.toml > index 0111362..c62b5e0 100644 > --- a/Cargo.toml > +++ b/Cargo.toml > @@ -7,14 +7,22 @@ edition = "2021" > [dependencies] > anyhow = "1.0" > bincode = "1.3" > +hyper = "0.14.5" > pico-args = "0.4" > md5 = "0.7.0" > scopeguard = "1.1.0" > serde = "1.0" > +serde_json = "1.0" > serde-big-array = "0.4.1" > > +proxmox-async = "0.4" > proxmox-io = "1.0.1" > proxmox-sys = "0.5.0" > proxmox-time = "2" > > +pbs-api-types = { path = "submodules/proxmox-backup-qemu/submodules/proxmox-backup/pbs-api-types" } > +pbs-client = { path = "submodules/proxmox-backup-qemu/submodules/proxmox-backup/pbs-client" } > +pbs-datastore = { path = "submodules/proxmox-backup-qemu/submodules/proxmox-backup/pbs-datastore" } > +pbs-key-config = { path = "submodules/proxmox-backup-qemu/submodules/proxmox-backup/pbs-key-config" } > +pbs-tools = { path = "submodules/proxmox-backup-qemu/submodules/proxmox-backup/pbs-tools" } > proxmox-backup-qemu = { path = "submodules/proxmox-backup-qemu" } > diff --git a/src/main.rs b/src/main.rs > index 2653d3e..de789c1 100644 > --- a/src/main.rs > +++ b/src/main.rs > @@ -37,6 +37,10 @@ Options: > Password file > --key-password-file > Key password file > + [--notes-file ] > + File containing a comment/notes > + [--log-file ] > + Log file > -h, --help > Print help > -V, --version > @@ -93,6 +97,8 @@ fn parse_args() -> Result { > let encrypt = args.contains(["-e", "--encrypt"]); > let password_file: Option = args.opt_value_from_str("--password-file")?; > let key_password_file: Option = args.opt_value_from_str("--key-password-file")?; > + let notes_file: Option = args.opt_value_from_str("--notes-file")?; > + let log_file_path: Option = args.opt_value_from_str("--log-file")?; > > match (encrypt, keyfile.is_some()) { > (true, false) => bail!("--encrypt requires a --keyfile!"), > @@ -170,6 +176,14 @@ fn parse_args() -> Result { > None > }; > > + let notes = if let Some(notes_file) = notes_file { > + let notes = std::fs::read_to_string(notes_file).context("Could not read notes file")?; > + > + Some(notes) > + } else { > + None > + }; > + > let options = BackupVmaToPbsArgs { > vma_file_path: vma_file_path.cloned(), > pbs_repository, > @@ -183,6 +197,8 @@ fn parse_args() -> Result { > fingerprint, > compress, > encrypt, > + notes, > + log_file_path, > }; > > Ok(options) > diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs > index 199cf50..eaad02e 100644 > --- a/src/vma2pbs.rs > +++ b/src/vma2pbs.rs > @@ -8,6 +8,12 @@ use std::ptr; > use std::time::SystemTime; > > use anyhow::{anyhow, bail, Error}; > +use pbs_api_types::{BackupDir, BackupNamespace, BackupType}; > +use pbs_client::{tools::connect, BackupRepository, HttpClient}; > +use pbs_datastore::DataBlob; > +use pbs_key_config::decrypt_key; > +use pbs_tools::crypt_config::CryptConfig; > +use proxmox_async::runtime::block_on; > use proxmox_backup_qemu::{ > capi_types::ProxmoxBackupHandle, proxmox_backup_add_config, proxmox_backup_close_image, > proxmox_backup_connect, proxmox_backup_disconnect, proxmox_backup_finish, > @@ -16,6 +22,7 @@ use proxmox_backup_qemu::{ > }; > use proxmox_time::epoch_to_rfc3339; > use scopeguard::defer; > +use serde_json::Value; > > use crate::vma::VmaReader; > > @@ -34,6 +41,8 @@ pub struct BackupVmaToPbsArgs { > pub fingerprint: String, > pub compress: bool, > pub encrypt: bool, > + pub notes: Option, > + pub log_file_path: Option, > } > > #[derive(Copy, Clone)] > @@ -52,7 +61,7 @@ fn handle_pbs_error(pbs_err: *mut c_char, function_name: &str) -> Result<(), Err > bail!("{function_name} failed: {pbs_err_str}"); > } > > -fn create_pbs_backup_task(args: BackupVmaToPbsArgs) -> Result<*mut ProxmoxBackupHandle, Error> { > +fn create_pbs_backup_task(args: &BackupVmaToPbsArgs) -> Result<*mut ProxmoxBackupHandle, Error> { this > println!("PBS repository: {}", args.pbs_repository); > if let Some(ns) = &args.namespace { > println!("PBS namespace: {}", ns); > @@ -65,22 +74,31 @@ fn create_pbs_backup_task(args: BackupVmaToPbsArgs) -> Result<*mut ProxmoxBackup > > let mut pbs_err: *mut c_char = ptr::null_mut(); > > - let pbs_repository_cstr = CString::new(args.pbs_repository)?; > - let ns_cstr = CString::new(args.namespace.unwrap_or("".to_string()))?; > - let backup_id_cstr = CString::new(args.backup_id)?; > - let pbs_password_cstr = CString::new(args.pbs_password)?; > - let fingerprint_cstr = CString::new(args.fingerprint)?; > - let keyfile_cstr = args.keyfile.map(|v| CString::new(v).unwrap()); > + let pbs_repository_cstr = CString::new(args.pbs_repository.as_str())?; > + let ns_cstr = CString::new(args.namespace.as_deref().unwrap_or(""))?; > + let backup_id_cstr = CString::new(args.backup_id.as_str())?; > + let pbs_password_cstr = CString::new(args.pbs_password.as_str())?; > + let fingerprint_cstr = CString::new(args.fingerprint.as_str())?; > + let keyfile_cstr = args > + .keyfile > + .as_ref() > + .map(|v| CString::new(v.as_str()).unwrap()); and these > let keyfile_ptr = keyfile_cstr > .as_ref() > .map(|v| v.as_ptr()) > .unwrap_or(ptr::null()); > - let key_password_cstr = args.key_password.map(|v| CString::new(v).unwrap()); > + let key_password_cstr = args > + .key_password > + .as_ref() > + .map(|v| CString::new(v.as_str()).unwrap()); and this > let key_password_ptr = key_password_cstr > .as_ref() > .map(|v| v.as_ptr()) > .unwrap_or(ptr::null()); > - let master_keyfile_cstr = args.master_keyfile.map(|v| CString::new(v).unwrap()); > + let master_keyfile_cstr = args > + .master_keyfile > + .as_ref() > + .map(|v| CString::new(v.as_str()).unwrap()); and this seem kinda unrelated? some sort of clippy/format fix patch that should be split out maybe? ah, it's because you need to pass the args to another helper further below.. that could be split into it's own patch to clearl separate the interface change and the rest.. > let master_keyfile_ptr = master_keyfile_cstr > .as_ref() > .map(|v| v.as_ptr()) > @@ -343,6 +361,98 @@ where > Ok(()) > } > > +fn upload_log_and_notes(args: &BackupVmaToPbsArgs) -> Result<(), Error> { > + if args.log_file_path.is_none() || args.notes.is_none() { > + return Ok(()); > + } > + > + let repo: BackupRepository = args.pbs_repository.parse()?; > + std::env::set_var("PBS_PASSWORD", &args.pbs_password); > + let client = connect(&repo)?; why not just create the client directly (HttpCLient::new()), then you don't need to set an env variable here that might leak the password.. > + let backup_dir = BackupDir::from((BackupType::Vm, args.backup_id.clone(), args.backup_time)); > + > + let namespace = match &args.namespace { > + Some(namespace) => BackupNamespace::new(namespace)?, > + None => BackupNamespace::root(), > + }; > + > + let mut request_args = serde_json::to_value(backup_dir)?; > + if !namespace.is_root() { > + request_args["ns"] = serde_json::to_value(namespace)?; > + } everything up to here could be a helper to get a client.. > + > + upload_log(&client, args, &repo, request_args.clone())?; > + upload_notes(&client, args, &repo, request_args)?; > + > + Ok(()) > +} > + > +fn upload_log( > + client: &HttpClient, > + args: &BackupVmaToPbsArgs, > + repo: &BackupRepository, > + request_args: Value, > +) -> Result<(), Error> { > + if let Some(log_file_path) = &args.log_file_path { > + let path = format!( > + "api2/json/admin/datastore/{}/upload-backup-log", > + repo.store() > + ); > + > + let data = std::fs::read(log_file_path)?; > + > + let blob = if args.encrypt { > + let crypt_config = match &args.keyfile { > + None => None, > + Some(keyfile) => { > + let key = std::fs::read(keyfile)?; > + let (key, _created, _) = decrypt_key(&key, &|| -> Result, Error> { > + match &args.key_password { > + Some(key_password) => Ok(key_password.clone().into_bytes()), > + None => bail!("no key password provided"), > + } > + })?; > + let crypt_config = CryptConfig::new(key)?; > + Some(crypt_config) > + } > + }; > + > + DataBlob::encode(&data, crypt_config.as_ref(), args.compress)? > + } else { > + // fixme: howto sign log? > + DataBlob::encode(&data, None, args.compress)? > + }; > + > + let body = hyper::Body::from(blob.into_inner()); > + > + block_on(async { > + client > + .upload("application/octet-stream", body, &path, Some(request_args)) > + .await > + .unwrap(); > + }); > + } > + > + Ok(()) > +} > + > +fn upload_notes( > + client: &HttpClient, > + args: &BackupVmaToPbsArgs, > + repo: &BackupRepository, > + mut request_args: Value, > +) -> Result<(), Error> { > + if let Some(notes) = &args.notes { > + request_args["notes"] = Value::from(notes.as_str()); > + let path = format!("api2/json/admin/datastore/{}/notes", repo.store()); > + block_on(async { > + client.put(&path, Some(request_args)).await.unwrap(); > + }); > + } > + > + Ok(()) > +} > + > pub fn backup_vma_to_pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> { > let vma_file: Box = match &args.vma_file_path { > Some(vma_file_path) => match File::open(vma_file_path) { > @@ -353,7 +463,7 @@ pub fn backup_vma_to_pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> { > }; > let vma_reader = VmaReader::new(vma_file)?; > > - let pbs = create_pbs_backup_task(args)?; > + let pbs = create_pbs_backup_task(&args)?; > > defer! { > proxmox_backup_disconnect(pbs); > @@ -377,6 +487,8 @@ pub fn backup_vma_to_pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> { > handle_pbs_error(pbs_err, "proxmox_backup_finish")?; > } > couldn't we just check here if we need to upload logs or notes, and then do something like: let (client, request_args) = client_helper(&args)?; if (args.notes_file.is_some()) { set_notes(&client, ..)?; } if (args.log_file_path.is_some()) { upload_log(&client, ..)?; } > + upload_log_and_notes(&args)?; > + > let transfer_duration = SystemTime::now().duration_since(start_transfer_time)?; > let total_seconds = transfer_duration.as_secs(); > let minutes = total_seconds / 60; > -- > 2.39.2 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel