[pbs-devel] [PATCH proxmox-backup 5/5] sync: conditionally pre-filter source list by verified or encrypted state

[Christian Ebner]

Commit 40ccd1ac ("fix #6072: server: sync encrypted or verified
snapshots only") introduced flags for sync jobs to include encrypted
or verified snapshots only. The chosen approach to check the snapshot
state right before sync only does however lead to possibly unexpected
results if the jobs parameters also include a transfer last setting,
as it is applied beforehand.

In order to improve this behaviour, already pre-filter the snapshot
source list if either the `verified-only` or the `encrypted-only`
flags are set.

The state has to be re-checked once again for consistency reasons,
when the snapshot is read locked and about to be synced. Snapshots
are not included if the state would match after the list generation
and will be excluded if the state no longer matches.

This is a breaking change in the filter logic.

Reported-by: Stefan Hanreich <s.hanreich at proxmox.com>
Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
---
 src/server/pull.rs | 11 ++++++++---
 src/server/push.rs | 31 +++++++++++++++++++++----------
 src/server/sync.rs | 34 +++++++++++++++++++++++++++++++---
 3 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 53de74f19..49f1d7a66 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -28,8 +28,9 @@ use pbs_datastore::{check_backup_owner, DataStore, StoreProgress};
 use pbs_tools::sha::sha256;
 
 use super::sync::{
-    check_namespace_depth_limit, ignore_not_verified_or_encrypted, LocalSource, RemoteSource,
-    RemovedVanishedStats, SkipInfo, SkipReason, SyncSource, SyncSourceReader, SyncStats,
+    check_namespace_depth_limit, exclude_not_verified_or_encrypted,
+    ignore_not_verified_or_encrypted, LocalSource, RemoteSource, RemovedVanishedStats, SkipInfo,
+    SkipReason, SyncSource, SyncSourceReader, SyncStats,
 };
 use crate::backup::{check_ns_modification_privs, check_ns_privs};
 use crate::tools::parallel_handler::ParallelHandler;
@@ -561,8 +562,12 @@ async fn pull_group(
     let list: Vec<(BackupDir, bool)> = raw_list
         .into_iter()
         .filter_map(|item| {
-            let dir = item.backup;
+            if exclude_not_verified_or_encrypted(&item, params.verified_only, params.encrypted_only)
+            {
+                return None;
+            }
 
+            let dir = item.backup;
             source_snapshots.insert(dir.time);
             // If resync_corrupt is set, check if the corresponding local snapshot failed to
             // verification
diff --git a/src/server/push.rs b/src/server/push.rs
index fdfed1b5a..d9a7aea8a 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -26,8 +26,9 @@ use pbs_datastore::read_chunk::AsyncReadChunk;
 use pbs_datastore::{DataStore, StoreProgress};
 
 use super::sync::{
-    check_namespace_depth_limit, ignore_not_verified_or_encrypted, LocalSource,
-    RemovedVanishedStats, SkipInfo, SkipReason, SyncSource, SyncStats,
+    check_namespace_depth_limit, exclude_not_verified_or_encrypted,
+    ignore_not_verified_or_encrypted, LocalSource, RemovedVanishedStats, SkipInfo, SkipReason,
+    SyncSource, SyncStats,
 };
 use crate::api2::config::remote;
 
@@ -682,12 +683,6 @@ pub(crate) async fn push_group(
         info!("Group '{group}' contains no snapshots to sync to remote");
     }
 
-    let total_snapshots = snapshots.len();
-    let cutoff = params
-        .transfer_last
-        .map(|count| total_snapshots.saturating_sub(count))
-        .unwrap_or_default();
-
     let target_namespace = params.map_to_target(namespace)?;
     let mut target_snapshots = fetch_target_snapshots(params, &target_namespace, group).await?;
     target_snapshots.sort_unstable_by_key(|a| a.backup.time);
@@ -698,11 +693,27 @@ pub(crate) async fn push_group(
         .unwrap_or(i64::MIN);
 
     let mut source_snapshots = HashSet::new();
+    let snapshots: Vec<BackupDir> = snapshots
+        .into_iter()
+        .filter_map(|item| {
+            if exclude_not_verified_or_encrypted(&item, params.verified_only, params.encrypted_only)
+            {
+                return None;
+            }
+            Some(item.backup)
+        })
+        .collect();
+
+    let total_snapshots = snapshots.len();
+    let cutoff = params
+        .transfer_last
+        .map(|count| total_snapshots.saturating_sub(count))
+        .unwrap_or_default();
+
     let snapshots: Vec<BackupDir> = snapshots
         .into_iter()
         .enumerate()
-        .filter_map(|(pos, item)| {
-            let snapshot = item.backup;
+        .filter_map(|(pos, snapshot)| {
             source_snapshots.insert(snapshot.time);
             if last_snapshot_time >= snapshot.time {
                 already_synced_skip_info.update(snapshot.time);
diff --git a/src/server/sync.rs b/src/server/sync.rs
index 155a0cb4b..8c0569672 100644
--- a/src/server/sync.rs
+++ b/src/server/sync.rs
@@ -20,8 +20,8 @@ use proxmox_router::HttpError;
 
 use pbs_api_types::{
     Authid, BackupDir, BackupGroup, BackupNamespace, CryptMode, GroupListItem, SnapshotListItem,
-    SyncDirection, SyncJobConfig, VerifyState, CLIENT_LOG_BLOB_NAME, MAX_NAMESPACE_DEPTH,
-    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_READ,
+    SyncDirection, SyncJobConfig, VerifyState, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME,
+    MAX_NAMESPACE_DEPTH, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_READ,
 };
 use pbs_client::{BackupReader, BackupRepository, HttpClient, RemoteChunkReader};
 use pbs_datastore::data_blob::DataBlob;
@@ -761,10 +761,38 @@ pub(super) fn ignore_not_verified_or_encrypted(
             .iter()
             .all(|file| file.chunk_crypt_mode() == CryptMode::Encrypt)
         {
-            info!("Snapshot {snapshot} not encrypted but encrypted-only set, snapshot skipped");
             return true;
         }
     }
 
     false
 }
+
+pub(super) fn exclude_not_verified_or_encrypted(
+    item: &SnapshotListItem,
+    verified_only: bool,
+    encrypted_only: bool,
+) -> bool {
+    if verified_only {
+        match &item.verification {
+            Some(state) if state.state == VerifyState::Ok => (),
+            _ => return true,
+        }
+    }
+
+    if encrypted_only
+        && !item.files.iter().all(|content| {
+            if content.filename == MANIFEST_BLOB_NAME.as_ref() {
+                content.crypt_mode == Some(CryptMode::SignOnly)
+            } else if content.filename == CLIENT_LOG_BLOB_NAME.as_ref() {
+                true
+            } else {
+                content.crypt_mode == Some(CryptMode::Encrypt)
+            }
+        })
+    {
+        return true;
+    }
+
+    false
+}
-- 
2.39.5