From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 44B628E6A for ; Wed, 23 Aug 2023 13:37:31 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 24AFA163CB for ; Wed, 23 Aug 2023 13:37:31 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 23 Aug 2023 13:37:29 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id ACF2343601 for ; Wed, 23 Aug 2023 13:37:29 +0200 (CEST) Date: Wed, 23 Aug 2023 13:37:27 +0200 From: Wolfgang Bumiller To: Hannes Laimer Cc: pbs-devel@lists.proxmox.com Message-ID: References: <20230808121344.199500-1-h.laimer@proxmox.com> <20230808121344.199500-2-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230808121344.199500-2-h.laimer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.101 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [remote.rs, job.store, pull.rs, tasks.rs, jobs.rs, sync.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 1/6] api2: make Remote for SyncJob optional X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Aug 2023 11:37:31 -0000 On Tue, Aug 08, 2023 at 02:13:39PM +0200, Hannes Laimer wrote: > Signed-off-by: Hannes Laimer > --- > 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, > #[serde(skip_serializing_if = "Option::is_none")] > pub owner: Option, > - pub remote: String, > + #[serde(skip_serializing_if = "Option::is_none")] > + /// None implies local sync. > + pub remote: Option, > pub remote_store: String, > #[serde(skip_serializing_if = "Option::is_none")] > pub remote_ns: Option, > 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) -> Result<(), Error> > > let job_list: Vec = 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 #[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@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@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@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@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 { > 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, > - remote: String, > + remote: Option, > remote_store: String, > remote_ns: Option, > remove_vanished: Option, > @@ -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