[pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

[pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

[Gabriel Goller]

Added the command `proxmox-backup-client group forget <group>` so
that we can forget (delete) whole groups with all the containing
snapshots. As this is quite dangerous, we added a prompt, so the
user has to confirm the operation.


Changes since v3:
 * Removed localization

--Version 2 has been skipped--

Changes since v1:
 * Added localization



proxmox:

Gabriel Goller (1):
  router: cli: added `ask_for_confirmation` helper

 proxmox-router/Cargo.toml     |  1 +
 proxmox-router/src/cli/mod.rs | 46 ++++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletion(-)


proxmox-backup:

Gabriel Goller (1):
  close #4763: client: added command to forget backup group

 proxmox-backup-client/src/group.rs | 69 ++++++++++++++++++++++++++++++
 proxmox-backup-client/src/main.rs  |  3 ++
 2 files changed, 72 insertions(+)
 create mode 100644 proxmox-backup-client/src/group.rs


Summary over all repositories:
  4 files changed, 118 insertions(+), 1 deletions(-)

-- 
murpp v0.4.0

[pbs-devel] [PATCH v4 proxmox 1/2] router: cli: added `ask_for_confirmation` helper

[Gabriel Goller]

Added `ask_for_confirmation` helper that outputs a prompt and
lets the user confirm or deny it.
Implemented to close #4763.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-router/Cargo.toml     |  1 +
 proxmox-router/src/cli/mod.rs | 46 ++++++++++++++++++++++++++++++++++-
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/proxmox-router/Cargo.toml b/proxmox-router/Cargo.toml
index c53ccd0..cd9140d 100644
--- a/proxmox-router/Cargo.toml
+++ b/proxmox-router/Cargo.toml
@@ -19,6 +19,7 @@ percent-encoding.workspace = true
 serde_json.workspace = true
 serde.workspace = true
 unicode-width ="0.1.8"
+regex.workspace = true
 
 # cli:
 tokio = { workspace = true, features = [], optional = true }
diff --git a/proxmox-router/src/cli/mod.rs b/proxmox-router/src/cli/mod.rs
index 208df4a..2628538 100644
--- a/proxmox-router/src/cli/mod.rs
+++ b/proxmox-router/src/cli/mod.rs
@@ -12,7 +12,10 @@
 //! - Ability to create interactive commands (using ``rustyline``)
 //! - Supports complex/nested commands
 
-use std::collections::HashMap;
+use std::{
+    collections::HashMap,
+    io::{self, Write},
+};
 
 use crate::ApiMethod;
 
@@ -62,6 +65,47 @@ pub fn init_cli_logger(env_var_name: &str, default_log_level: &str) {
     .init();
 }
 
+pub enum DefaultAnswer {
+    Yes,
+    No,
+}
+
+/// Prints a prompt to ask for confirmation
+pub fn ask_for_confirmation(query: String, default: DefaultAnswer) -> Result<bool, io::Error> {
+    let yesnoprompt: (char, char) = match default {
+        DefaultAnswer::Yes => ('Y', 'n'),
+        DefaultAnswer::No => ('y', 'N'),
+    };
+    print!("{query} [{}/{}]: ", yesnoprompt.0, yesnoprompt.1);
+
+    io::stdout().flush()?;
+    let stdin = io::stdin();
+    let mut line = String::new();
+    stdin.read_line(&mut line)?;
+
+    use regex::Regex;
+    match default {
+        DefaultAnswer::Yes => {
+            // unwrap is okay, because this regex is correct
+            let no_regex: Regex = Regex::new("^[nN]").unwrap();
+            if no_regex.is_match(line.trim()) {
+                Ok(false)
+            } else {
+                Ok(true)
+            }
+        }
+        DefaultAnswer::No => {
+            // unwrap is okay, because this regex is coorrect
+            let yes_regex: Regex = Regex::new("^[yY]").unwrap();
+            if yes_regex.is_match(line.trim()) {
+                Ok(true)
+            } else {
+                Ok(false)
+            }
+        }
+    }
+}
+
 /// Define a simple CLI command.
 pub struct CliCommand {
     /// The Schema definition.
-- 
2.39.2

[pbs-devel] [PATCH v4 proxmox-backup 2/2] close #4763: client: added command to forget backup group

[Gabriel Goller]

Added the command `proxmox-backup-client group forget <group>` so
that we can forget (delete) whole groups with all the containing
snapshots.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-backup-client/src/group.rs | 69 ++++++++++++++++++++++++++++++
 proxmox-backup-client/src/main.rs  |  3 ++
 2 files changed, 72 insertions(+)
 create mode 100644 proxmox-backup-client/src/group.rs

diff --git a/proxmox-backup-client/src/group.rs b/proxmox-backup-client/src/group.rs
new file mode 100644
index 00000000..0e4e735f
--- /dev/null
+++ b/proxmox-backup-client/src/group.rs
@@ -0,0 +1,69 @@
+use anyhow::Error;
+use serde_json::Value;
+
+use proxmox_router::cli::{ask_for_confirmation, CliCommand, CliCommandMap, DefaultAnswer};
+use proxmox_schema::api;
+
+use crate::{
+    complete_backup_group, complete_namespace, complete_repository, merge_group_into,
+    REPO_URL_SCHEMA,
+};
+use pbs_api_types::{BackupGroup, BackupNamespace};
+use pbs_client::tools::{connect, extract_repository_from_value};
+
+pub fn group_mgmt_cli() -> CliCommandMap {
+    CliCommandMap::new().insert(
+        "forget",
+        CliCommand::new(&API_METHOD_FORGET_GROUP)
+            .arg_param(&["group"])
+            .completion_cb("ns", complete_namespace)
+            .completion_cb("repository", complete_repository)
+            .completion_cb("group", complete_backup_group),
+    )
+}
+
+#[api(
+    input: {
+        properties: {
+            group: {
+                type: String,
+                description: "Backup group",
+            },
+            repository: {
+                schema: REPO_URL_SCHEMA,
+                optional: true,
+            },
+            ns: {
+                type: BackupNamespace,
+                optional: true,
+            },
+        }
+    }
+)]
+/// Forget (remove) backup snapshots.
+async fn forget_group(group: String, param: Value) -> Result<Value, Error> {
+    let backup_group: BackupGroup = group.parse()?;
+    let repo = extract_repository_from_value(&param)?;
+    let client = connect(&repo)?;
+
+    let mut api_params = param;
+    merge_group_into(api_params.as_object_mut().unwrap(), backup_group.clone());
+
+    let path = format!("api2/json/admin/datastore/{}/snapshots", repo.store());
+    let result = client.get(&path, Some(api_params.clone())).await?;
+    let snapshots = result["data"].as_array().unwrap().len();
+
+    if ask_for_confirmation(
+        format!(
+            "Delete group \"{}\" with {} snapshot(s)?",
+            backup_group, snapshots
+        ),
+        DefaultAnswer::No,
+    )? {
+        let path = format!("api2/json/admin/datastore/{}/groups", repo.store());
+        let _ = client.delete(&path, Some(api_params)).await?;
+
+        println!("Successfully deleted group!");
+    }
+    Ok(Value::Null)
+}
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index e5caf87d..73d39d51 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -72,6 +72,8 @@ mod catalog;
 pub use catalog::*;
 mod snapshot;
 pub use snapshot::*;
+mod group;
+pub use group::*;
 pub mod key;
 pub mod namespace;
 
@@ -1783,6 +1785,7 @@ fn main() {
         .insert("benchmark", benchmark_cmd_def)
         .insert("change-owner", change_owner_cmd_def)
         .insert("namespace", namespace::cli_map())
+        .insert("group", group_mgmt_cli())
         .alias(&["files"], &["snapshot", "files"])
         .alias(&["forget"], &["snapshot", "forget"])
         .alias(&["upload-log"], &["snapshot", "upload-log"])
-- 
2.39.2

Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

[Gabriel Goller]

bump, this should still apply on master.

Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

[Christian Ebner]

Hi,
thanks for tackling this issue.

The group forget command is something I missed quite a lot when doing 
testing on PBS with accumulating snapshots in a group an not wanting to 
fallback to the UI (I actually always went the route of deleting the 
snapshot folder).

Tested the following:
- Created a few backup snapshots, creating thereby a new backup group
- Tried to delete the group while backup is running
   result was as expected: delete failed, missing lock error
- Tried to delete the group while a restore is running
   result was as expected: delete failed, missing lock (although 
different from the previous one)
- Tried to remove without any other task running
   result was as expected: group deleted, including the group folder in 
the datastore
- Tried to delete empty group
   result was as expected: group deleted, including the group folder in 
the datastore

Two further things I noticed:

The confirmation dialog also allows me to type in `yolo` or any string 
starting with `yY` and nevertheless accepts this as valid confirmation 
input (similar for negation). Should we limit this further?

When one tries to delete a non existing group, the dialog asks me for 
confirmation, failing however afterwards with an error message, leaking 
also the datastore path to the client. While the former is not an issue 
and the intention is to be able to remove empty groups, the latter is 
not okay in my opinion.
So either check if the group even exists before asking for confirmation, 
or map the error to not leak the datastore path.

For the rest consider this:

Tested-by: Christian Ebner <c.ebner@proxmox.com>



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

[Christian Ebner]

On 4/17/24 16:15, Christian Ebner wrote:
> Hi,
> thanks for tackling this issue.
> 
> The group forget command is something I missed quite a lot when doing 
> testing on PBS with accumulating snapshots in a group an not wanting to 
> fallback to the UI (I actually always went the route of deleting the 
> snapshot folder).
> 
> Tested the following:
> - Created a few backup snapshots, creating thereby a new backup group
> - Tried to delete the group while backup is running
>    result was as expected: delete failed, missing lock error
> - Tried to delete the group while a restore is running
>    result was as expected: delete failed, missing lock (although 
> different from the previous one)
> - Tried to remove without any other task running
>    result was as expected: group deleted, including the group folder in 
> the datastore
> - Tried to delete empty group
>    result was as expected: group deleted, including the group folder in 
> the datastore
> 
> Two further things I noticed:
> 
> The confirmation dialog also allows me to type in `yolo` or any string 
> starting with `yY` and nevertheless accepts this as valid confirmation 
> input (similar for negation). Should we limit this further?
> 
> When one tries to delete a non existing group, the dialog asks me for 
> confirmation, failing however afterwards with an error message, leaking 
> also the datastore path to the client. While the former is not an issue 
> and the intention is to be able to remove empty groups, the latter is 
> not okay in my opinion.
> So either check if the group even exists before asking for confirmation, 
> or map the error to not leak the datastore path.

Forgot to add that the same applies for the datastore locking, the 
client should not need to know the paths there either, this should 
concern the server only.

> 
> For the rest consider this:
> 
> Tested-by: Christian Ebner <c.ebner@proxmox.com>
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

[Gabriel Goller]

On Wed Apr 17, 2024 at 4:15 PM CEST, Christian Ebner wrote:
> Hi,
> thanks for tackling this issue.
>
> The group forget command is something I missed quite a lot when doing 
> testing on PBS with accumulating snapshots in a group an not wanting to 
> fallback to the UI (I actually always went the route of deleting the 
> snapshot folder).
>
> Tested the following:
> - Created a few backup snapshots, creating thereby a new backup group
> - Tried to delete the group while backup is running
>    result was as expected: delete failed, missing lock error
> - Tried to delete the group while a restore is running
>    result was as expected: delete failed, missing lock (although 
> different from the previous one)
> - Tried to remove without any other task running
>    result was as expected: group deleted, including the group folder in 
> the datastore
> - Tried to delete empty group
>    result was as expected: group deleted, including the group folder in 
> the datastore
>
> Two further things I noticed:
>
> The confirmation dialog also allows me to type in `yolo` or any string 
> starting with `yY` and nevertheless accepts this as valid confirmation 
> input (similar for negation). Should we limit this further?

It seems 'apt' also matches just on the beginning character, but
nevertheless we can do this, wouldn't hurt restricting it more

I think adding a '$' on the regex should do the trick:

    let no_regex: Regex = Regex::new("^[nN]$").unwrap();


> When one tries to delete a non existing group, the dialog asks me for 
> confirmation, failing however afterwards with an error message, leaking 
> also the datastore path to the client. While the former is not an issue 
> and the intention is to be able to remove empty groups, the latter is 
> not okay in my opinion.
> So either check if the group even exists before asking for confirmation, 
> or map the error to not leak the datastore path.

The thing is that we don't differentiate between an empty group or a
nonexistent group — at least when using the api. This means that even
the list-groups api call will **not** return a group if it doesn't contain
any snapshots, but deleting it will succeed (because it still exists).

What we can do is obviously ignore the error message and simply return a
generic "failed to remove group" or "group not found" to avoid leaking
stuff. Although debugging a issue will be much harder with these vague 
error messages. 

IMO leaking the datastore is not a big issue, as the client can also
list task logs and the datastore path is going to be visible in there as
well.

> For the rest consider this:
>
> Tested-by: Christian Ebner <c.ebner@proxmox.com>

Thanks for testing this!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

[Christian Ebner]

On 4/18/24 11:13, Gabriel Goller wrote:
> On Wed Apr 17, 2024 at 4:15 PM CEST, Christian Ebner wrote:
>> Hi,
>> thanks for tackling this issue.
>>
>> The group forget command is something I missed quite a lot when doing
>> testing on PBS with accumulating snapshots in a group an not wanting to
>> fallback to the UI (I actually always went the route of deleting the
>> snapshot folder).
>>
>> Tested the following:
>> - Created a few backup snapshots, creating thereby a new backup group
>> - Tried to delete the group while backup is running
>>     result was as expected: delete failed, missing lock error
>> - Tried to delete the group while a restore is running
>>     result was as expected: delete failed, missing lock (although
>> different from the previous one)
>> - Tried to remove without any other task running
>>     result was as expected: group deleted, including the group folder in
>> the datastore
>> - Tried to delete empty group
>>     result was as expected: group deleted, including the group folder in
>> the datastore
>>
>> Two further things I noticed:
>>
>> The confirmation dialog also allows me to type in `yolo` or any string
>> starting with `yY` and nevertheless accepts this as valid confirmation
>> input (similar for negation). Should we limit this further?
> 
> It seems 'apt' also matches just on the beginning character, but
> nevertheless we can do this, wouldn't hurt restricting it more
> 
> I think adding a '$' on the regex should do the trick:
> 
>      let no_regex: Regex = Regex::new("^[nN]$").unwrap();
> 

Yeah, however allowing more might be a feature (probably this should 
also allow at least `yes` and `no`). Feel free to decide, I just wanted 
to mention this, in case it was not intentional.

> 
>> When one tries to delete a non existing group, the dialog asks me for
>> confirmation, failing however afterwards with an error message, leaking
>> also the datastore path to the client. While the former is not an issue
>> and the intention is to be able to remove empty groups, the latter is
>> not okay in my opinion.
>> So either check if the group even exists before asking for confirmation,
>> or map the error to not leak the datastore path.
> 
> The thing is that we don't differentiate between an empty group or a
> nonexistent group — at least when using the api. This means that even
> the list-groups api call will **not** return a group if it doesn't contain
> any snapshots, but deleting it will succeed (because it still exists) >
> What we can do is obviously ignore the error message and simply return a
> generic "failed to remove group" or "group not found" to avoid leaking
> stuff. Although debugging a issue will be much harder with these vague
> error messages.
> 
> IMO leaking the datastore is not a big issue, as the client can also
> list task logs and the datastore path is going to be visible in there as
> well.

It is not a big issue, agreed, but nevertheless an information leak. But 
you are right, the task log does contain that information as well, so 
there is at least no additional information here. Given that, this is 
fine I guess.

> 
>> For the rest consider this:
>>
>> Tested-by: Christian Ebner <c.ebner@proxmox.com>
> 
> Thanks for testing this!



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

[Gabriel Goller]

On Thu Apr 18, 2024 at 12:24 PM CEST, Christian Ebner wrote:
> >> When one tries to delete a non existing group, the dialog asks me for
> >> confirmation, failing however afterwards with an error message, leaking
> >> also the datastore path to the client. While the former is not an issue
> >> and the intention is to be able to remove empty groups, the latter is
> >> not okay in my opinion.
> >> So either check if the group even exists before asking for confirmation,
> >> or map the error to not leak the datastore path.
> > 
> > The thing is that we don't differentiate between an empty group or a
> > nonexistent group — at least when using the api. This means that even
> > the list-groups api call will **not** return a group if it doesn't contain
> > any snapshots, but deleting it will succeed (because it still exists) >
> > What we can do is obviously ignore the error message and simply return a
> > generic "failed to remove group" or "group not found" to avoid leaking
> > stuff. Although debugging a issue will be much harder with these vague
> > error messages.
> > 
> > IMO leaking the datastore is not a big issue, as the client can also
> > list task logs and the datastore path is going to be visible in there as
> > well.
>
> It is not a big issue, agreed, but nevertheless an information leak. But 
> you are right, the task log does contain that information as well, so 
> there is at least no additional information here. Given that, this is 
> fine I guess.

As this particular error (group not found) is very common, I added a
branch that matches this error and rephrases it without the datastore
path.

Submitted a new version!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel