[pbs-devel] [PATCH proxmox-backup v3 1/6] api2: make Remote for SyncJob optional
Wolfgang Bumiller
w.bumiller at proxmox.com
Wed Aug 23 13:37:27 CEST 2023
On Tue, Aug 08, 2023 at 02:13:39PM +0200, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer at proxmox.com>
> ---
> pbs-api-types/src/jobs.rs | 5 ++-
> src/api2/config/remote.rs | 2 +-
> src/api2/config/sync.rs | 41 +++++++++++++-------
> src/api2/node/tasks.rs | 4 +-
> src/api2/pull.rs | 62 ++++++++++++++++++++++---------
> src/server/email_notifications.rs | 18 +++++----
> 6 files changed, 90 insertions(+), 42 deletions(-)
>
> diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
> index 23e19b7b..85fdbe41 100644
> --- a/pbs-api-types/src/jobs.rs
> +++ b/pbs-api-types/src/jobs.rs
> @@ -467,6 +467,7 @@ pub const TRANSFER_LAST_SCHEMA: Schema =
> },
> remote: {
> schema: REMOTE_ID_SCHEMA,
> + optional: true,
> },
> "remote-store": {
> schema: DATASTORE_SCHEMA,
> @@ -515,7 +516,9 @@ 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")]
> + /// None implies local sync.
> + 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 76dd3b89..307cf3cd 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 01e5f2ce..21634bd5 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.
> @@ -275,6 +287,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;
> }
> @@ -334,7 +349,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;
> @@ -503,7 +518,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(),
> @@ -538,11 +553,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));
>
> @@ -555,10 +570,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,
> @@ -567,7 +582,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 866361c6..12ce70f6 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 here comes from a capture group in `SYNC_JOB_WORKER_ID_REGEX`
where the remote is not actually optional here.
> (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 daeba7cf..664ecce5 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -8,7 +8,7 @@ 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,
> + GROUP_FILTER_LIST_SCHEMA, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP,
> PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
> TRANSFER_LAST_SCHEMA,
> };
> @@ -22,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> {
> @@ -39,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(
> @@ -65,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.as_deref().unwrap_or("local"),
> &sync_job.remote_store,
> sync_job.remote_ns.clone().unwrap_or_default(),
> sync_job
> @@ -91,7 +101,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()),
This was still left unanswered in the last revision:
Can I create a remote and name it `localhost`?
This and the corresponding SYNC_JOB_WORKER_ID_REGEX need to be adapted
accordingly.
Also this patch still uses both "localhost" vs "local"?
I'm starting to wonder whether we should even expose the regexes this
way or instead have a structural type implementing FromStr and Display
instead so it's harder to use wrongly and have both directions in the
same place.
> sync_job.remote_store,
> sync_job.store,
> sync_job.ns.clone().unwrap_or_default(),
> @@ -124,11 +134,28 @@ pub fn do_sync_job(
> worker,
> "sync datastore '{}' from '{}/{}'",
> sync_job.store,
> - sync_job.remote,
> + sync_job.remote.clone().unwrap_or("local".to_string()),
> 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 {
> + if let (Some(target_ns), Some(source_ns)) = (sync_job.ns, sync_job.remote_ns) {
> + if target_ns.path().starts_with(source_ns.path())
> + && sync_job.store == sync_job.remote_store
> + && sync_job.max_depth.map_or(true, |sync_depth| {
> + target_ns.depth() + sync_depth > MAX_NAMESPACE_DEPTH
> + }) {
> + task_log!(
> + worker,
> + "Can't sync namespace into one of its sub-namespaces, would exceed maximum namespace depth, skipping"
> + );
> + }
> + } else {
> + pull_store(&worker, &client, pull_params).await?;
> + }
> + }
>
> task_log!(worker, "sync job '{}' end", &job_id);
>
> @@ -180,6 +207,7 @@ pub fn do_sync_job(
> },
> remote: {
> schema: REMOTE_ID_SCHEMA,
> + optional: true,
> },
> "remote-store": {
> schema: DATASTORE_SCHEMA,
> @@ -224,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>,
> @@ -248,7 +276,7 @@ async fn pull(
> &auth_id,
> &store,
> ns_str.as_deref(),
> - &remote,
> + remote.as_deref(),
> &remote_store,
> delete,
> )?;
> @@ -256,7 +284,7 @@ async fn pull(
> let pull_params = PullParameters::new(
> &store,
> ns,
> - &remote,
> + remote.as_deref().unwrap_or("local"),
> &remote_store,
> remote_ns.unwrap_or_default(),
> auth_id.clone(),
> @@ -280,7 +308,7 @@ async fn pull(
> worker,
> "pull datastore '{}' from '{}/{}'",
> store,
> - remote,
> + remote.as_deref().unwrap_or("localhost"),
And I don't think messages should be formatted with slashes if we
normally use this for remote + datastore.
> remote_store,
> );
>
> @@ -299,4 +327,4 @@ async fn pull(
> Ok(upid_str)
> }
>
> -pub const ROUTER: Router = Router::new().post(&API_METHOD_PULL);
> +pub const ROUTER: Router = Router::new().post(&API_METHOD_PULL);
> \ No newline at end of file
> diff --git a/src/server/email_notifications.rs b/src/server/email_notifications.rs
> index ea1476d7..18881782 100644
> --- a/src/server/email_notifications.rs
> +++ b/src/server/email_notifications.rs
> @@ -484,15 +484,17 @@ pub fn send_sync_status(
> }
> };
>
> + let tmp_src_string;
> + let source_str = if let Some(remote) = &job.remote {
> + tmp_src_string = format!("Sync remote '{}'", remote);
> + &tmp_src_string
> + } else {
> + "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.39.2
More information about the pbs-devel
mailing list