[pbs-devel] [PATCH proxmox-backup v2 1/5] api2: make Remote for SyncJob optional

Fabian Grünbichler f.gruenbichler at proxmox.com
Tue Feb 28 12:35:56 CET 2023


On February 23, 2023 1:55 pm, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer at proxmox.com>
> ---
>  pbs-api-types/src/jobs.rs         |  4 +-
>  src/api2/config/remote.rs         |  2 +-
>  src/api2/config/sync.rs           | 41 +++++++++++++------
>  src/api2/node/tasks.rs            |  4 +-
>  src/api2/pull.rs                  | 68 +++++++++++++++++++++++--------
>  src/server/email_notifications.rs | 16 ++++----
>  6 files changed, 93 insertions(+), 42 deletions(-)
> 
> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
> index cf7618c4..68db6cb8 100644
> --- a/pbs-api-types/src/jobs.rs
> +++ b/pbs-api-types/src/jobs.rs
> @@ -462,6 +462,7 @@ pub const GROUP_FILTER_LIST_SCHEMA: Schema =
>          },
>          remote: {
>              schema: REMOTE_ID_SCHEMA,
> +            optional: true,

could probably benefit from a description specifying that not specifying a
remote implies syncing from the local PBS system

>          },
>          "remote-store": {
>              schema: DATASTORE_SCHEMA,
> @@ -506,7 +507,8 @@ pub struct SyncJobConfig {
>      pub ns: Option<BackupNamespace>,
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub owner: Option<Authid>,
> -    pub remote: String,
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub remote: Option<String>,
>      pub remote_store: String,
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub remote_ns: Option<BackupNamespace>,
> diff --git a/src/api2/config/remote.rs b/src/api2/config/remote.rs
> index 2f02d121..aa74bdc0 100644
> --- a/src/api2/config/remote.rs
> +++ b/src/api2/config/remote.rs
> @@ -268,7 +268,7 @@ pub fn delete_remote(name: String, digest: Option<String>) -> Result<(), Error>
>  
>      let job_list: Vec<SyncJobConfig> = sync_jobs.convert_to_typed_array("sync")?;
>      for job in job_list {
> -        if job.remote == name {
> +        if job.remote.map_or(false, |id| id == name) {
>              param_bail!(
>                  "name",
>                  "remote '{}' is used by sync job '{}' (datastore '{}')",
> diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
> index bd7373df..4c5d06e2 100644
> --- a/src/api2/config/sync.rs
> +++ b/src/api2/config/sync.rs
> @@ -8,8 +8,8 @@ use proxmox_schema::{api, param_bail};
>  
>  use pbs_api_types::{
>      Authid, SyncJobConfig, SyncJobConfigUpdater, JOB_ID_SCHEMA, PRIV_DATASTORE_AUDIT,
> -    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_AUDIT,
> -    PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA,
> +    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ,
> +    PRIV_REMOTE_AUDIT, PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA,
>  };
>  use pbs_config::sync;
>  
> @@ -25,8 +25,13 @@ pub fn check_sync_job_read_access(
>          return false;
>      }
>  
> -    let remote_privs = user_info.lookup_privs(auth_id, &["remote", &job.remote]);
> -    remote_privs & PRIV_REMOTE_AUDIT != 0
> +    if let Some(remote) = &job.remote {
> +        let remote_privs = user_info.lookup_privs(auth_id, &["remote", remote]);
> +        remote_privs & PRIV_REMOTE_AUDIT != 0
> +    } else {
> +        let source_ds_privs = user_info.lookup_privs(auth_id, &["datastore", &job.remote_store]);
> +        source_ds_privs & PRIV_DATASTORE_AUDIT != 0
> +    }
>  }
>  
>  /// checks whether user can run the corresponding pull job
> @@ -63,8 +68,13 @@ pub fn check_sync_job_modify_access(
>          return false;
>      }
>  
> -    let remote_privs = user_info.lookup_privs(auth_id, &["remote", &job.remote, &job.remote_store]);
> -    remote_privs & PRIV_REMOTE_READ != 0
> +    if let Some(remote) = &job.remote {
> +        let remote_privs = user_info.lookup_privs(auth_id, &["remote", remote, &job.remote_store]);
> +        remote_privs & PRIV_REMOTE_READ != 0
> +    } else {
> +        let source_ds_privs = user_info.lookup_privs(auth_id, &["datastore", &job.remote_store]);
> +        source_ds_privs & PRIV_DATASTORE_READ != 0
> +    }
>  }
>  
>  #[api(
> @@ -191,6 +201,8 @@ pub fn read_sync_job(id: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Sync
>  #[serde(rename_all = "kebab-case")]
>  /// Deletable property name
>  pub enum DeletableProperty {
> +    /// Delete the remote property(-> meaning local).
> +    Remote,
>      /// Delete the owner property.
>      Owner,
>      /// Delete the comment property.
> @@ -273,6 +285,9 @@ pub fn update_sync_job(
>      if let Some(delete) = delete {
>          for delete_prop in delete {
>              match delete_prop {
> +                DeletableProperty::Remote => {
> +                    data.remote = None;
> +                }
>                  DeletableProperty::Owner => {
>                      data.owner = None;
>                  }
> @@ -329,7 +344,7 @@ pub fn update_sync_job(
>          data.ns = Some(ns);
>      }
>      if let Some(remote) = update.remote {
> -        data.remote = remote;
> +        data.remote = Some(remote);
>      }
>      if let Some(remote_store) = update.remote_store {
>          data.remote_store = remote_store;
> @@ -495,7 +510,7 @@ acl:1:/remote/remote1/remotestore1:write at pbs:RemoteSyncOperator
>  
>      let mut job = SyncJobConfig {
>          id: "regular".to_string(),
> -        remote: "remote0".to_string(),
> +        remote: Some("remote0".to_string()),
>          remote_store: "remotestore1".to_string(),
>          remote_ns: None,
>          store: "localstore0".to_string(),
> @@ -529,11 +544,11 @@ acl:1:/remote/remote1/remotestore1:write at pbs:RemoteSyncOperator
>      assert!(!check_sync_job_read_access(&user_info, &read_auth_id, &job));
>  
>      // reading without proper read permissions on local end must fail
> -    job.remote = "remote1".to_string();
> +    job.remote = Some("remote1".to_string());
>      assert!(!check_sync_job_read_access(&user_info, &read_auth_id, &job));
>  
>      // reading without proper read permissions on remote end must fail
> -    job.remote = "remote0".to_string();
> +    job.remote = Some("remote0".to_string());
>      job.store = "localstore1".to_string();
>      assert!(!check_sync_job_read_access(&user_info, &read_auth_id, &job));
>  
> @@ -546,10 +561,10 @@ acl:1:/remote/remote1/remotestore1:write at pbs:RemoteSyncOperator
>      ));
>  
>      // writing without proper write permissions on local end must fail
> -    job.remote = "remote1".to_string();
> +    job.remote = Some("remote1".to_string());
>  
>      // writing without proper write permissions on remote end must fail
> -    job.remote = "remote0".to_string();
> +    job.remote = Some("remote0".to_string());
>      job.store = "localstore1".to_string();
>      assert!(!check_sync_job_modify_access(
>          &user_info,
> @@ -558,7 +573,7 @@ acl:1:/remote/remote1/remotestore1:write at pbs:RemoteSyncOperator
>      ));
>  
>      // reset remote to one where users have access
> -    job.remote = "remote1".to_string();
> +    job.remote = Some("remote1".to_string());
>  
>      // user with read permission can only read, but not modify/run
>      assert!(check_sync_job_read_access(&user_info, &read_auth_id, &job));
> diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
> index d386f805..780cb6d1 100644
> --- a/src/api2/node/tasks.rs
> +++ b/src/api2/node/tasks.rs
> @@ -75,14 +75,14 @@ fn check_job_privs(auth_id: &Authid, user_info: &CachedUserInfo, upid: &UPID) ->
>                  let local_store = captures.get(3);
>                  let local_ns = captures.get(4).map(|m| m.as_str());
>  
> -                if let (Some(remote), Some(remote_store), Some(local_store)) =
> +                if let (remote, Some(remote_store), Some(local_store)) =
>                      (remote, remote_store, local_store)
>                  {
>                      return check_pull_privs(
>                          auth_id,
>                          local_store.as_str(),
>                          local_ns,
> -                        remote.as_str(),
> +                        remote.map(|remote| remote.as_str()),
>                          remote_store.as_str(),
>                          false,
>                      );
> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> index b2473ec8..bb8f6fe1 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -9,7 +9,8 @@ use proxmox_sys::task_log;
>  use pbs_api_types::{
>      Authid, BackupNamespace, GroupFilter, RateLimitConfig, SyncJobConfig, DATASTORE_SCHEMA,
>      GROUP_FILTER_LIST_SCHEMA, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP,
> -    PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
> +    PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA,
> +    REMOVE_VANISHED_BACKUPS_SCHEMA,
>  };
>  use pbs_config::CachedUserInfo;
>  use proxmox_rest_server::WorkerTask;
> @@ -21,7 +22,7 @@ pub fn check_pull_privs(
>      auth_id: &Authid,
>      store: &str,
>      ns: Option<&str>,
> -    remote: &str,
> +    remote: Option<&str>,
>      remote_store: &str,
>      delete: bool,
>  ) -> Result<(), Error> {
> @@ -38,12 +39,22 @@ pub fn check_pull_privs(
>          PRIV_DATASTORE_BACKUP,
>          false,
>      )?;
> -    user_info.check_privs(
> -        auth_id,
> -        &["remote", remote, remote_store],
> -        PRIV_REMOTE_READ,
> -        false,
> -    )?;
> +
> +    if let Some(remote) = remote {
> +        user_info.check_privs(
> +            auth_id,
> +            &["remote", remote, remote_store],
> +            PRIV_REMOTE_READ,
> +            false,
> +        )?;
> +    } else {
> +        user_info.check_privs(
> +            auth_id,
> +            &["datastore", remote_store],
> +            PRIV_DATASTORE_BACKUP,
> +            false,
> +        )?;
> +    }
>  
>      if delete {
>          user_info.check_privs(
> @@ -64,7 +75,7 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
>          PullParameters::new(
>              &sync_job.store,
>              sync_job.ns.clone().unwrap_or_default(),
> -            &sync_job.remote,
> +            sync_job.remote.clone().as_deref(),
>              &sync_job.remote_store,
>              sync_job.remote_ns.clone().unwrap_or_default(),
>              sync_job
> @@ -75,7 +86,6 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
>              sync_job.remove_vanished,
>              sync_job.max_depth,
>              sync_job.group_filter.clone(),
> -            sync_job.limit.clone(),

not too happy about the limit being dropped here.. but see comment on other patch!

>          )
>      }
>  }
> @@ -89,7 +99,7 @@ pub fn do_sync_job(
>  ) -> Result<String, Error> {
>      let job_id = format!(
>          "{}:{}:{}:{}:{}",
> -        sync_job.remote,
> +        sync_job.remote.clone().unwrap_or("localhost".to_string()),

doesn't this break the task access checks? probably need to adapt those and
either use some other format for local jobs, or use some placeholder that cannot
also be a remote name..

>          sync_job.remote_store,
>          sync_job.store,
>          sync_job.ns.clone().unwrap_or_default(),
> @@ -122,11 +132,34 @@ pub fn do_sync_job(
>                      worker,
>                      "sync datastore '{}' from '{}/{}'",
>                      sync_job.store,
> -                    sync_job.remote,
> +                    sync_job.remote.clone().unwrap_or("local".to_string()),

nit: "local" or "localhost" (or possibly even another format entirely, e.g.
"local sync of datastore '..' from '..')

>                      sync_job.remote_store,
>                  );
>  
> -                pull_store(&worker, &client, pull_params).await?;
> +                if sync_job.remote.is_some() {
> +                    pull_store(&worker, &client, pull_params).await?;
> +                } else {
> +                    match (sync_job.ns, sync_job.remote_ns) {
> +                        (Some(target_ns), Some(source_ns))
> +                            if target_ns.path().starts_with(source_ns.path())
> +                                && sync_job.store == sync_job.remote_store =>
> +                        {
> +                            task_log!(
> +                                worker,
> +                                "Can't sync namespace into one of its sub-namespaces, skipping"
> +                            );
> +                        }
> +                        (_, None) if sync_job.store == sync_job.remote_store => {
> +                            task_log!(
> +                                worker,
> +                                "Can't sync root namespace into same datastore, skipping"
> +                            );
> +                        }
> +                        _ => {
> +                            pull_store(&worker, pull_params).await?;
> +                        }

I think this block is mostly not needed - it's perfectly fine to sync from a
namespace into a sub-namespace or from the root namespace into some other
(non-root) namespace.

there are only two things that are forbidden:
- syncing from one store+namespace combo into the exact same store+namespace
combo locally
- combinations which exceed the max namespace depth (same as for remote syncing,
so if possible handle at the same place)

> +                    }
> +                }
>  
>                  task_log!(worker, "sync job '{}' end", &job_id);
>  
> @@ -178,6 +211,7 @@ pub fn do_sync_job(
>              },
>              remote: {
>                  schema: REMOTE_ID_SCHEMA,
> +                optional: true,

same here - description should mention what no remote means

>              },
>              "remote-store": {
>                  schema: DATASTORE_SCHEMA,
> @@ -218,7 +252,7 @@ The delete flag additionally requires the Datastore.Prune privilege on '/datasto
>  async fn pull(
>      store: String,
>      ns: Option<BackupNamespace>,
> -    remote: String,
> +    remote: Option<String>,
>      remote_store: String,
>      remote_ns: Option<BackupNamespace>,
>      remove_vanished: Option<bool>,
> @@ -241,7 +275,7 @@ async fn pull(
>          &auth_id,
>          &store,
>          ns_str.as_deref(),
> -        &remote,
> +        remote.as_deref(),
>          &remote_store,
>          delete,
>      )?;
> @@ -249,7 +283,7 @@ async fn pull(
>      let pull_params = PullParameters::new(
>          &store,
>          ns,
> -        &remote,
> +        remote.as_deref(),
>          &remote_store,
>          remote_ns.unwrap_or_default(),
>          auth_id.clone(),
> @@ -272,7 +306,7 @@ async fn pull(
>                  worker,
>                  "pull datastore '{}' from '{}/{}'",
>                  store,
> -                remote,
> +                remote.as_deref().unwrap_or("localhost"),

same as above, the local part might use a different formatting or at least a
consistent one ;)

>                  remote_store,
>              );
>  
> diff --git a/src/server/email_notifications.rs b/src/server/email_notifications.rs
> index b3298cf9..31a46b0f 100644
> --- a/src/server/email_notifications.rs
> +++ b/src/server/email_notifications.rs
> @@ -486,15 +486,15 @@ pub fn send_sync_status(
>          }
>      };
>  
> +    let source_str = if let Some(remote) = job.remote.clone() {
> +        format!("Sync remote '{}'", remote)
> +    } else {
> +        format!("Sync local")
> +    };
> +
>      let subject = match result {
> -        Ok(()) => format!(
> -            "Sync remote '{}' datastore '{}' successful",
> -            job.remote, job.remote_store,
> -        ),
> -        Err(_) => format!(
> -            "Sync remote '{}' datastore '{}' failed",
> -            job.remote, job.remote_store,
> -        ),
> +        Ok(()) => format!("{} datastore '{}' successful", source_str, job.remote_store,),
> +        Err(_) => format!("{} datastore '{}' failed", source_str, job.remote_store,),
>      };
>  
>      send_job_status_mail(email, &subject, &text)?;
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 





More information about the pbs-devel mailing list