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

[Lukas Wagner]

On  2025-07-18 10:40, Christian Ebner wrote:
> 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.

Sure, no problem for more.
> 
>> [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(())
>>> +}
>>
>>
>