Re: [pbs-devel] [PATCH vma-to-pbs v2 2/2] add support for notes and logs

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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 <f.schauer@proxmox.com>
> ---
> 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>
>            Key password file
> +      [--notes-file <NOTES_FILE>]
> +          File containing a comment/notes
> +      [--log-file <LOG_FILE>]
> +          Log file
>    -h, --help
>            Print help
>    -V, --version
> @@ -93,6 +97,8 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>      let encrypt = args.contains(["-e", "--encrypt"]);
>      let password_file: Option<OsString> = args.opt_value_from_str("--password-file")?;
>      let key_password_file: Option<OsString> = args.opt_value_from_str("--key-password-file")?;
> +    let notes_file: Option<OsString> = args.opt_value_from_str("--notes-file")?;
> +    let log_file_path: Option<OsString> = 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<BackupVmaToPbsArgs, Error> {
>          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<BackupVmaToPbsArgs, Error> {
>          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<String>,
> +    pub log_file_path: Option<OsString>,
>  }
>  
>  #[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<Vec<u8>, 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<dyn BufRead> = 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