Re: [PATCH proxmox-backup v2 18/27] fix #7251: api: push: encrypt snapshots using configured encryption key

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 10.04.26 um 18:54 schrieb Christian Ebner:
> diff --git a/src/server/push.rs b/src/server/push.rs
> @@ -886,6 +886,27 @@ pub(crate) async fn push_snapshot(
>
> +    if params.crypt_config.is_some() {
> +        let mut contains_unencrypted_file = false;
> +        // Check if snapshot is fully encrypted or not encrypted at all:
> +        // refuse progress otherwise to upload partially unencrypted contents or mix encryption key.
> +        if source_manifest.files().iter().all(|file| {
> +            if file.chunk_crypt_mode() == CryptMode::None {
> +                contains_unencrypted_file = true;
> +                true
> +            } else {
> +                false
> +            }
> +        }) {

nit: this is hard to follow - .all() returning true here means "every file is
unencrypted" because the closure returns true for None and false for encrypted.
Maybe just spell out the three states, e.g. something like:

  let all_none = files.iter().all(|f| f.chunk_crypt_mode() == CryptMode::None);
  let any_none = files.iter().any(|f| f.chunk_crypt_mode() == CryptMode::None);
  if all_none { encrypt } else if any_none { warn partial, skip } else { push as-is }

> +        } else if contains_unencrypted_file {
> +            warn!("Encountered partially encrypted snapshot, refuse to re-encrypt and skip");

nit: I'd probably include the snapshot name here, otherwise the admin cannot
tell which snapshot was skipped in a multi-snapshot sync run as e.g. getting
that from context, e.g. log lines above might feel like guess work to some
admins, especially if we do parallel syncing. OTOH. if we add a prefix for
all log message on parallel syncing, this nit might not matter anymore.

btw: still has log::warn!/log::info! on lines 873-874 and log::info! on line
931 while everything else uses tracing macros.