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

[Filip Schauer <f.schauer@proxmox.com>]

Superseded by:
https://lists.proxmox.com/pipermail/pbs-devel/2024-July/010159.html

On 10/07/2024 14:18, Fabian Grünbichler wrote:
> 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
>
>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel