[pbs-devel] [PATCH proxmox-backup 0/4] fix #4256: remove datastore from configs after deletion

[pbs-devel] [PATCH proxmox-backup 0/4] fix #4256: remove datastore from configs after deletion

[Hannes Laimer]

Deleting a datastore did not result in it being removed from
all configs(acl and prune were missing), this fixes that and adds
a checkbox for keeping the configs when deleting a datastore.

https://bugzilla.proxmox.com/show_bug.cgi?id=4256

Hannes Laimer (4):
  fix #4256: api2: remove prune jobs on datastore delete
  pbs-config: add delete_node for ACL-tree
  fix #4256: api2: remove datastore ACL-node on removal of datastore
  ui: add 'keep configs' checkbox to datastore removal window

 pbs-config/src/acl.rs        | 61 ++++++++++++++++++++++++++++++++++++
 src/api2/config/datastore.rs | 12 ++++++-
 www/datastore/OptionView.js  |  5 +++
 3 files changed, 77 insertions(+), 1 deletion(-)

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 1/4] fix #4256: api2: remove prune jobs on datastore delete

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/config/datastore.rs | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index b6679f2f..27489590 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -18,7 +18,10 @@ use pbs_api_types::{
 use pbs_config::BackupLockGuard;
 use pbs_datastore::chunk_store::ChunkStore;
 
-use crate::api2::admin::{sync::list_sync_jobs, verify::list_verification_jobs};
+use crate::api2::admin::{
+    prune::list_prune_jobs, sync::list_sync_jobs, verify::list_verification_jobs,
+};
+use crate::api2::config::prune::delete_prune_job;
 use crate::api2::config::sync::delete_sync_job;
 use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_backup_jobs};
 use crate::api2::config::verify::delete_verification_job;
@@ -433,6 +436,9 @@ pub async fn delete_datastore(
         for job in list_sync_jobs(Some(name.clone()), Value::Null, rpcenv)? {
             delete_sync_job(job.config.id, None, rpcenv)?
         }
+        for job in list_prune_jobs(Some(name.clone()), Value::Null, rpcenv)? {
+            delete_prune_job(job.config.id, None, rpcenv)?
+        }
 
         let tape_jobs = list_tape_backup_jobs(Value::Null, rpcenv)?;
         for job_config in tape_jobs
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/4] pbs-config: add delete_node for ACL-tree

[Hannes Laimer]

... needed for the deletion of datastore ACL-nodes when the datastore
is removed.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-config/src/acl.rs | 61 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/pbs-config/src/acl.rs b/pbs-config/src/acl.rs
index 928a4121..9defc3c6 100644
--- a/pbs-config/src/acl.rs
+++ b/pbs-config/src/acl.rs
@@ -396,6 +396,21 @@ impl AclTree {
         node.delete_user_role(auth_id, role);
     }
 
+    /// Deletes the [AclTreeNode] at the specified patth
+    ///
+    /// Never fails, deletes a node iff the specified path exists.
+    pub fn delete_node(&mut self, path: &str) {
+        let mut path = split_acl_path(path);
+        let last = path.pop();
+        let parent = match self.get_node_mut(&path) {
+            Some(n) => n,
+            None => return,
+        };
+        if let Some(name) = last {
+            parent.children.remove(name);
+        }
+    }
+
     /// Inserts the specified `role` into the `group` ACL on `path`.
     ///
     /// The [AclTreeNode] representing `path` will be created and inserted into the tree if
@@ -947,4 +962,50 @@ acl:1:/storage/store1:user1@pbs:DatastoreBackup
 
         Ok(())
     }
+
+    #[test]
+    fn test_delete_node() -> Result<(), Error> {
+        let mut tree = AclTree::new();
+
+        let user1: Authid = "user1@pbs".parse()?;
+
+        tree.insert_user_role("/storage", &user1, "NoAccess", true);
+        tree.insert_user_role("/storage/a", &user1, "NoAccess", true);
+        tree.insert_user_role("/storage/b", &user1, "NoAccess", true);
+        tree.insert_user_role("/storage/b/a", &user1, "NoAccess", true);
+        tree.insert_user_role("/storage/b/b", &user1, "NoAccess", true);
+        tree.insert_user_role("/datastore/c", &user1, "NoAccess", true);
+        tree.insert_user_role("/datastore/d", &user1, "NoAccess", true);
+
+        assert!(tree.find_node("/storage/b/a").is_some());
+        tree.delete_node("/storage/b/a");
+        assert!(tree.find_node("/storage/b/a").is_none());
+
+        assert!(tree.find_node("/storage/b/b").is_some());
+        assert!(tree.find_node("/storage/b").is_some());
+        tree.delete_node("/storage/b");
+        assert!(tree.find_node("/storage/b/b").is_none());
+        assert!(tree.find_node("/storage/b").is_none());
+
+        assert!(tree.find_node("/storage").is_some());
+        assert!(tree.find_node("/storage/a").is_some());
+        tree.delete_node("/storage");
+        assert!(tree.find_node("/storage").is_none());
+        assert!(tree.find_node("/storage/a").is_none());
+
+        assert!(tree.find_node("/datastore/c").is_some());
+        tree.delete_node("/datastore/c");
+        assert!(tree.find_node("/datastore/c").is_none());
+
+        assert!(tree.find_node("/datastore/d").is_some());
+        tree.delete_node("/datastore/d");
+        assert!(tree.find_node("/datastore/d").is_none());
+
+        // '/' should not be deletable
+        assert!(tree.find_node("/").is_some());
+        tree.delete_node("/");
+        assert!(tree.find_node("/").is_some());
+
+        Ok(())
+    }
 }
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 3/4] fix #4256: api2: remove datastore ACL-node on removal of datastore

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/config/datastore.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 27489590..677650b9 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -440,6 +440,10 @@ pub async fn delete_datastore(
             delete_prune_job(job.config.id, None, rpcenv)?
         }
 
+        let (mut tree, _digest) = pbs_config::acl::config()?;
+        tree.delete_node(&format!("/datastore/{}", name));
+        pbs_config::acl::save_config(&tree)?;
+
         let tape_jobs = list_tape_backup_jobs(Value::Null, rpcenv)?;
         for job_config in tape_jobs
             .into_iter()
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 4/4] ui: add 'keep configs' checkbox to datastore removal window

[Hannes Laimer]

... since the API already accepts a boolean for that.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/datastore/OptionView.js | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 448cd4b3..d20c5d8a 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -45,6 +45,11 @@ Ext.define('PBS.window.SafeDatastoreDestroy', {
 	bind: {
 	    value: '{destroyData}',
 	},
+    }, {
+	xtype: 'proxmoxcheckbox',
+	name: 'keep-job-configs',
+	boxLabel: gettext("Keep configured jobs and permissions"),
+	defaultValue: false,
     }, {
 	xtype: 'component',
 	reference: 'noteCmp',
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 4/4] ui: add 'keep configs' checkbox to datastore removal window

[Wolfgang Bumiller]

On Mon, Dec 19, 2022 at 03:13:26PM +0100, Hannes Laimer wrote:
> ... since the API already accepts a boolean for that.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  www/datastore/OptionView.js | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
> index 448cd4b3..d20c5d8a 100644
> --- a/www/datastore/OptionView.js
> +++ b/www/datastore/OptionView.js
> @@ -45,6 +45,11 @@ Ext.define('PBS.window.SafeDatastoreDestroy', {
>  	bind: {
>  	    value: '{destroyData}',
>  	},
> +    }, {
> +	xtype: 'proxmoxcheckbox',
> +	name: 'keep-job-configs',
> +	boxLabel: gettext("Keep configured jobs and permissions"),
> +	defaultValue: false,

^ This never gets passed along the API call.
We handle 'destroy-data' in `getParams`, so this probably needs to be
added there as well, but shouldn't this work automatically for these
simple cases? @Dominik

>      }, {
>  	xtype: 'component',
>  	reference: 'noteCmp',
> -- 
> 2.30.2

[pbs-devel] partially-applied: [PATCH proxmox-backup 0/4] fix #4256: remove datastore from configs after deletion

[Wolfgang Bumiller]

applied the backend patches, but the UI patch doesn't doesn't pass along
the parameter

On Mon, Dec 19, 2022 at 03:13:22PM +0100, Hannes Laimer wrote:
> Deleting a datastore did not result in it being removed from
> all configs(acl and prune were missing), this fixes that and adds
> a checkbox for keeping the configs when deleting a datastore.
> 
> https://bugzilla.proxmox.com/show_bug.cgi?id=4256
> 
> Hannes Laimer (4):
>   fix #4256: api2: remove prune jobs on datastore delete
>   pbs-config: add delete_node for ACL-tree
>   fix #4256: api2: remove datastore ACL-node on removal of datastore
>   ui: add 'keep configs' checkbox to datastore removal window
> 
>  pbs-config/src/acl.rs        | 61 ++++++++++++++++++++++++++++++++++++
>  src/api2/config/datastore.rs | 12 ++++++-
>  www/datastore/OptionView.js  |  5 +++
>  3 files changed, 77 insertions(+), 1 deletion(-)
> 
> -- 
> 2.30.2