From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
	by lore.proxmox.com (Postfix) with ESMTPS id 5F5FF1FF15C
	for <inbox@lore.proxmox.com>; Fri, 16 May 2025 13:54:54 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id 0340EC085;
	Fri, 16 May 2025 13:55:19 +0200 (CEST)
From: Christian Ebner <c.ebner@proxmox.com>
To: pbs-devel@lists.proxmox.com
Date: Fri, 16 May 2025 13:54:29 +0200
Message-Id: <20250516115429.208563-6-c.ebner@proxmox.com>
X-Mailer: git-send-email 2.39.5
In-Reply-To: <20250516115429.208563-1-c.ebner@proxmox.com>
References: <20250516115429.208563-1-c.ebner@proxmox.com>
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.029 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
Subject: [pbs-devel] [PATCH proxmox-backup 5/5] sync: conditionally
 pre-filter source list by verified or encrypted state
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox Backup Server development discussion
 <pbs-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>

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@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@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



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