[pbs-devel] [PATCH proxmox-backup 1/3] pull: filter local removal candidates by owner

[pbs-devel] [PATCH proxmox-backup 1/3] pull: filter local removal candidates by owner

[Fabian Grünbichler]

else this might remove groups which are not part of the pull scope. note
that setting/using remove_vanished already checks the required privs
earlier.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/server/pull.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 097bd5cb..c0831417 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -28,7 +28,7 @@ use pbs_datastore::index::IndexFile;
 use pbs_datastore::manifest::{
     archive_type, ArchiveType, BackupManifest, FileInfo, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME,
 };
-use pbs_datastore::{DataStore, StoreProgress};
+use pbs_datastore::{check_backup_owner, DataStore, StoreProgress};
 use pbs_tools::sha::sha256;
 use proxmox_rest_server::WorkerTask;
 
@@ -801,6 +801,10 @@ pub async fn pull_store(
                 if new_groups.contains(local_group.as_ref()) {
                     continue;
                 }
+                let owner = params.store.get_owner(&local_group.group())?;
+                if check_backup_owner(&owner, &params.owner).is_err() {
+                    continue;
+                }
                 if let Some(ref group_filter) = &params.group_filter {
                     if !apply_filters(local_group.as_ref(), group_filter) {
                         continue;
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/3] pull: remove unnecessary `pub` visibility

[Fabian Grünbichler]

pull_store is the entrypoint used by other code, the rest does not need
to be visible at all.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/server/pull.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index c0831417..674c7826 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -469,7 +469,7 @@ async fn pull_snapshot(
     Ok(())
 }
 
-pub async fn pull_snapshot_from(
+async fn pull_snapshot_from(
     worker: &WorkerTask,
     reader: Arc<BackupReader>,
     tgt_store: Arc<DataStore>,
@@ -556,7 +556,7 @@ impl std::fmt::Display for SkipInfo {
     }
 }
 
-pub async fn pull_group(
+async fn pull_group(
     worker: &WorkerTask,
     client: &HttpClient,
     params: &PullParameters,
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 3/3] pull: add some comments

[Fabian Grünbichler]

and remove already fixed fixmes.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/server/pull.rs | 62 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 58 insertions(+), 4 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 674c7826..ab94b6e1 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -34,21 +34,29 @@ use proxmox_rest_server::WorkerTask;
 
 use crate::tools::parallel_handler::ParallelHandler;
 
-// fixme: implement filters
-// fixme: delete vanished groups
-// Todo: correctly lock backup groups
-
+/// Parameters for a pull operation.
 pub struct PullParameters {
+    /// Remote that is pulled from
     remote: Remote,
+    /// Full specification of remote datastore
     source: BackupRepository,
+    /// Local store that is pulled into
     store: Arc<DataStore>,
+    /// Owner of synced groups (needs to match local owner of pre-existing groups)
     owner: Authid,
+    /// Whether to remove groups which exist locally, but not on the remote end
     remove_vanished: bool,
+    /// Filters for reducing the pull scope
     group_filter: Option<Vec<GroupFilter>>,
+    /// Rate limits for all transfers from `remote`
     limit: RateLimitConfig,
 }
 
 impl PullParameters {
+    /// Creates a new instance of `PullParameters`.
+    /// 
+    /// `remote` will be dereferenced via [pbs_api_types::RemoteConfig], and combined into a 
+    /// [BackupRepository] with `remote_store`.
     pub fn new(
         store: &str,
         remote: &str,
@@ -83,6 +91,7 @@ impl PullParameters {
         })
     }
 
+    /// Creates a new [HttpClient] for accessing the [Remote] that is pulled from.
     pub async fn client(&self) -> Result<HttpClient, Error> {
         crate::api2::config::remote::remote_client(&self.remote, Some(self.limit.clone())).await
     }
@@ -218,6 +227,14 @@ fn verify_archive(info: &FileInfo, csum: &[u8; 32], size: u64) -> Result<(), Err
     Ok(())
 }
 
+/// Pulls a single file referenced by a manifest.
+///
+/// Pulling an archive consists of the following steps:
+/// - Create tmp file for archive
+/// - Download archive file into tmp file
+/// - Verify tmp file checksum
+/// - if archive is an index, pull referenced chunks
+/// - Rename tmp file into real path
 async fn pull_single_archive(
     worker: &WorkerTask,
     reader: &BackupReader,
@@ -317,6 +334,15 @@ async fn try_client_log_download(
     Ok(())
 }
 
+/// Actual implementation of pulling a snapshot.
+///
+/// Pulling a snapshot consists of the following steps:
+/// - (Re)download the manifest
+/// -- if it matches, only download log and treat snapshot as already synced
+/// - Iterate over referenced files
+/// -- if file already exists, verify contents
+/// -- if not, pull it from the remote
+/// - Download log if not already existing
 async fn pull_snapshot(
     worker: &WorkerTask,
     reader: Arc<BackupReader>,
@@ -469,6 +495,8 @@ async fn pull_snapshot(
     Ok(())
 }
 
+/// Pulls a `snapshot` into `tgt_store`, differentiating between new snapshots (removed on error)
+/// and existing ones (kept even on error).
 async fn pull_snapshot_from(
     worker: &WorkerTask,
     reader: Arc<BackupReader>,
@@ -556,6 +584,19 @@ impl std::fmt::Display for SkipInfo {
     }
 }
 
+/// Pulls a group according to `params`.
+///
+/// Pulling a group consists of the following steps:
+/// - Query the list of snapshots available for this group on the remote, sort by snapshot time
+/// - Get last snapshot timestamp on local datastore
+/// - Iterate over list of snapshots
+/// -- Recreate client/BackupReader
+/// -- pull snapshot, unless it's not finished yet or older than last local snapshot
+/// - (remove_vanished) list all local snapshots, remove those that don't exist on remote
+///
+/// Permission checks:
+/// - remote snapshot access is checked by remote (twice: query and opening the backup reader)
+/// - local group owner is already checked by pull_store
 async fn pull_group(
     worker: &WorkerTask,
     client: &HttpClient,
@@ -695,6 +736,19 @@ async fn pull_group(
     Ok(())
 }
 
+/// Pulls a store according to `params`.
+///
+/// Pulling a store consists of the following steps:
+/// - Query list of groups on the remote
+/// - Filter list according to configured group filters
+/// - Iterate list and attempt to pull each group in turn
+/// - (remove_vanished) remove groups with matching owner and matching the configured group filters which are
+///   not or no longer available on the remote
+///
+/// Permission checks:
+/// - access to local datastore and remote entry need to be checked at call site
+/// - remote groups are filtered by remote
+/// - owner check for vanished groups done here
 pub async fn pull_store(
     worker: &WorkerTask,
     client: &HttpClient,
-- 
2.30.2

[pbs-devel] applied-series: [PATCH proxmox-backup 1/3] pull: filter local removal candidates by owner

[Thomas Lamprecht]

Am 4/29/22 um 11:17 schrieb Fabian Grünbichler:
> else this might remove groups which are not part of the pull scope. note
> that setting/using remove_vanished already checks the required privs
> earlier.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  src/server/pull.rs | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
>

applied series, thanks!