[pbs-devel] [PATCH proxmox-backup v8 03/45] api: config: implement endpoints to manipulate and list s3 configs

[Christian Ebner]

On 7/18/25 9:32 AM, Lukas Wagner wrote:
> Reviewed-by: Lukas Wagner <l.wagner at proxmox.com>
> 
> 
> On  2025-07-15 14:52, Christian Ebner wrote:
>> +/// Update an s3 client configuration.
>> +#[allow(clippy::too_many_arguments)]
>> +pub fn update_s3_client_config(
>> +    id: String,
>> +    update: S3ClientConfigUpdater,
>> +    update_secrets: S3ClientSecretsConfigUpdater,
>> +    delete: Option<Vec<DeletableProperty>>,
>> +    digest: Option<String>,
>> +    _rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<(), Error> {
>> +    let _lock = s3::lock_config()?;
>> +    let (mut config, expected_digest) = s3::config()?;
>> +    let (mut secrets, secrets_digest) = s3::secrets_config()?;
>> +    let expected_digest = digest_with_secrets(&expected_digest, &secrets_digest);
>> +
>> +    // Secrets are not included in digest concurrent changes therefore not detected.
>> +    if let Some(ref digest) = digest {
>> +        let digest = <[u8; 32]>::from_hex(digest)?;
>> +        crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
>> +    }
>> +
>> +    let mut data: S3ClientConfig = config.lookup("s3client", &id)?;
>> +
>> +    if let Some(delete) = delete {
>> +        for delete_prop in delete {
>> +            match delete_prop {
>> +                DeletableProperty::Port => {
>> +                    data.port = None;
>> +                }
>> +                DeletableProperty::Region => {
>> +                    data.region = None;
>> +                }
>> +                DeletableProperty::Fingerprint => {
>> +                    data.fingerprint = None;
>> +                }
>> +                DeletableProperty::PathStyle => {
>> +                    data.path_style = None;
>> +                }
>> +            }
>> +        }
>> +    }
> 
> Some time ago I've found that it is quite useful to
> destructure the updater like I did in proxmox-notify [1].
> This ensures that you don't forget to update the
> API handler after adding a new field to the config struct.
> Not a must, just a suggestion, since I like this pattern quite a bit :)

If fine by you, I'll keep this for now and do this as a followup, 
including the same changes to the sync jobs and other configs were we 
have this pattern. Would like to focus on the other comments first, as 
these seem more pressing.

> [1] https://git.proxmox.com/?p=proxmox.git;a=blob;f=proxmox-notify/src/api/webhook.rs;h=9d904d0bf57f9f789bb6723e1d8ca710fcf0cb96;hb=HEAD#l175
> 
>> +
>> +    if let Some(endpoint) = update.endpoint {
>> +        data.endpoint = endpoint;
>> +    }
>> +    if let Some(port) = update.port {
>> +        data.port = Some(port);
>> +    }
>> +    if let Some(region) = update.region {
>> +        data.region = Some(region);
>> +    }
>> +    if let Some(access_key) = update.access_key {
>> +        data.access_key = access_key;
>> +    }
>> +    if let Some(fingerprint) = update.fingerprint {
>> +        data.fingerprint = Some(fingerprint);
>> +    }
>> +    if let Some(path_style) = update.path_style {
>> +        data.path_style = Some(path_style);
>> +    }
>> +
>> +    let mut secrets_data: S3ClientSecretsConfig = secrets.lookup("s3secrets", &id)?;
>> +    if let Some(secret_key) = update_secrets.secret_key {
>> +        secrets_data.secret_key = secret_key;
>> +    }
>> +
>> +    config.set_data(&id, "s3client", &data)?;
>> +    secrets.set_data(&id, "s3secrets", &secrets_data)?;
>> +    s3::save_config(&config, &secrets)?;
>> +
>> +    Ok(())
>> +}
> 
>