From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 6A66E1FF164 for ; Fri, 22 Nov 2024 14:14:39 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2022112CF8; Fri, 22 Nov 2024 14:14:48 +0100 (CET) MIME-Version: 1.0 In-Reply-To: <20241122122604.335901-1-c.ebner@proxmox.com> References: <20241122122604.335901-1-c.ebner@proxmox.com> From: Fabian =?utf-8?q?Gr=C3=BCnbichler?= To: Christian Ebner , pbs-devel@lists.proxmox.com Date: Fri, 22 Nov 2024 14:14:08 +0100 Message-ID: <173228124842.2118190.6703201927866768772@yuna.proxmox.com> User-Agent: alot/0.10 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 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 Subject: [pbs-devel] applied: [PATCH v2 proxmox-backup] server: push: add error context to api calls and priv checks 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: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" with cargo fmt + a missing `format!` in one context folded in Quoting Christian Ebner (2024-11-22 13:26:04) > Add an anyhow context to errors and display the full error context > in the log output. Further, make it clear which errors stem from api > calls by explicitly mentioning this in the context message. > > This also fixes incorrect error handling by placing the error context > on the api result instead of the serde deserialization error for > cases this was handled incorrectly. > > Signed-off-by: Christian Ebner > --- > changes since version 1: > - fix incorrect api result error handling > - use anyhow context > - show full error context in log output > > src/server/push.rs | 88 +++++++++++++++++++++++++++++----------------- > 1 file changed, 56 insertions(+), 32 deletions(-) > > diff --git a/src/server/push.rs b/src/server/push.rs > index 4c489531c..1914cad75 100644 > --- a/src/server/push.rs > +++ b/src/server/push.rs > @@ -3,7 +3,7 @@ > use std::collections::HashSet; > use std::sync::{Arc, Mutex}; > > -use anyhow::{bail, format_err, Error}; > +use anyhow::{bail, Context, Error}; > use futures::stream::{self, StreamExt, TryStreamExt}; > use tokio::sync::mpsc; > use tokio_stream::wrappers::ReceiverStream; > @@ -180,7 +180,12 @@ fn check_ns_remote_datastore_privs( > // Fetch the list of namespaces found on target > async fn fetch_target_namespaces(params: &PushParameters) -> Result, Error> { > let api_path = params.target.datastore_api_path("namespace"); > - let mut result = params.target.client.get(&api_path, None).await?; > + let mut result = params > + .target > + .client > + .get(&api_path, None) > + .await > + .context("Fetching remote namespaces failed, remote returned error")?; > let namespaces: Vec = serde_json::from_value(result["data"].take())?; > let mut namespaces: Vec = namespaces > .into_iter() > @@ -201,7 +206,7 @@ async fn remove_target_namespace( > } > > check_ns_remote_datastore_privs(params, target_namespace, PRIV_REMOTE_DATASTORE_MODIFY) > - .map_err(|err| format_err!("Pruning remote datastore namespaces not allowed - {err}"))?; > + .context("Pruning remote datastore namespaces not allowed")?; > > let api_path = params.target.datastore_api_path("namespace"); > > @@ -214,13 +219,17 @@ async fn remove_target_namespace( > args["error-on-protected"] = serde_json::to_value(false)?; > } > > - let mut result = params.target.client.delete(&api_path, Some(args)).await?; > + let mut result = params > + .target > + .client > + .delete(&api_path, Some(args)) > + .await > + .context(format!( > + "Failed to remove remote namespace {target_namespace}, remote returned error" > + ))?; > > if params.target.supports_prune_delete_stats { > - let data = result["data"].take(); > - serde_json::from_value(data).map_err(|err| { > - format_err!("removing target namespace {target_namespace} failed - {err}") > - }) > + Ok(serde_json::from_value(result["data"].take())?) > } else { > Ok(BackupGroupDeleteStats::default()) > } > @@ -235,7 +244,13 @@ async fn fetch_target_groups( > let api_path = params.target.datastore_api_path("groups"); > let args = Some(serde_json::json!({ "ns": target_namespace.name() })); > > - let mut result = params.target.client.get(&api_path, args).await?; > + let mut result = params > + .target > + .client > + .get(&api_path, args) > + .await > + .context("Failed to fetch remote groups, remote returned error")?; > + > let groups: Vec = serde_json::from_value(result["data"].take())?; > > let (mut owned, not_owned) = groups.into_iter().fold( > @@ -262,7 +277,7 @@ async fn remove_target_group( > backup_group: &BackupGroup, > ) -> Result { > check_ns_remote_datastore_privs(params, target_namespace, PRIV_REMOTE_DATASTORE_PRUNE) > - .map_err(|err| format_err!("Pruning remote datastore contents not allowed - {err}"))?; > + .context("Pruning remote datastore contents not allowed")?; > > let api_path = params.target.datastore_api_path("groups"); > > @@ -273,12 +288,11 @@ async fn remove_target_group( > args["error-on-protected"] = serde_json::to_value(false)?; > } > > - let mut result = params.target.client.delete(&api_path, Some(args)).await?; > + let mut result = params.target.client.delete(&api_path, Some(args)).await > + .context(format!("Failed to remove remote group {backup_group}, remote returned error"))?; > > if params.target.supports_prune_delete_stats { > - let data = result["data"].take(); > - serde_json::from_value(data) > - .map_err(|err| format_err!("removing target group {backup_group} failed - {err}")) > + Ok(serde_json::from_value(result["data"].take())?) > } else { > Ok(BackupGroupDeleteStats::default()) > } > @@ -295,7 +309,7 @@ async fn check_or_create_target_namespace( > // Sub-namespaces have to be created by creating parent components first. > > check_ns_remote_datastore_privs(params, target_namespace, PRIV_REMOTE_DATASTORE_MODIFY) > - .map_err(|err| format_err!("Creating remote namespace not allowed - {err}"))?; > + .context("Creating remote namespace not allowed")?; > > let mut parent = BackupNamespace::root(); > for component in target_namespace.components() { > @@ -310,12 +324,12 @@ async fn check_or_create_target_namespace( > if !parent.is_root() { > args["parent"] = serde_json::to_value(parent.clone())?; > } > - match params.target.client.post(&api_path, Some(args)).await { > - Ok(_) => info!("Successfully created new namespace {current} on remote"), > - Err(err) => { > - bail!("Remote creation of namespace {current} failed, remote returned: {err}") > - } > - } > + > + params.target.client.post(&api_path, Some(args)).await > + .context("Creation of remote namespace {current} failed, remote returned error")?; > + > + info!("Successfully created new namespace {current} on remote"); > + > existing_target_namespaces.push(current.clone()); > parent = current; > } > @@ -377,7 +391,7 @@ pub(crate) async fn push_store(mut params: PushParameters) -> Result ) > .await > { > - warn!("Encountered error: {err}"); > + warn!("Encountered error: {err:#}"); > warn!("Failed to sync {source_store_and_ns} into {target_store_and_ns}!"); > errors = true; > continue; > @@ -404,7 +418,7 @@ pub(crate) async fn push_store(mut params: PushParameters) -> Result } > Err(err) => { > errors = true; > - info!("Encountered errors: {err}"); > + info!("Encountered errors: {err:#}"); > info!("Failed to sync {source_store_and_ns} into {target_store_and_ns}!"); > } > } > @@ -453,7 +467,7 @@ pub(crate) async fn push_store(mut params: PushParameters) -> Result } > } > Err(err) => { > - warn!("Encountered errors: {err}"); > + warn!("Encountered errors: {err:#}"); > warn!("Failed to remove vanished namespace {target_namespace} from remote!"); > continue; > } > @@ -483,7 +497,7 @@ pub(crate) async fn push_namespace( > let target_namespace = params.map_to_target(namespace)?; > // Check if user is allowed to perform backups on remote datastore > check_ns_remote_datastore_privs(params, &target_namespace, PRIV_REMOTE_DATASTORE_BACKUP) > - .map_err(|err| format_err!("Pushing to remote namespace not allowed - {err}"))?; > + .context("Pushing to remote namespace not allowed")?; > > let mut list: Vec = params > .source > @@ -529,7 +543,7 @@ pub(crate) async fn push_namespace( > match push_group(params, namespace, &group, &mut progress).await { > Ok(sync_stats) => stats.add(sync_stats), > Err(err) => { > - warn!("Encountered errors: {err}"); > + warn!("Encountered errors: {err:#}"); > warn!("Failed to push group {group} to remote!"); > errors = true; > } > @@ -562,7 +576,7 @@ pub(crate) async fn push_namespace( > })); > } > Err(err) => { > - warn!("Encountered errors: {err}"); > + warn!("Encountered errors: {err:#}"); > warn!("Failed to remove vanished group {target_group} from remote!"); > errors = true; > continue; > @@ -584,7 +598,12 @@ async fn fetch_target_snapshots( > if !target_namespace.is_root() { > args["ns"] = serde_json::to_value(target_namespace)?; > } > - let mut result = params.target.client.get(&api_path, Some(args)).await?; > + let mut result = params > + .target > + .client > + .get(&api_path, Some(args)) > + .await > + .context("Failed to fetch remote snapshots, remote returned error")?; > let snapshots: Vec = serde_json::from_value(result["data"].take())?; > > Ok(snapshots) > @@ -596,14 +615,19 @@ async fn forget_target_snapshot( > snapshot: &BackupDir, > ) -> Result<(), Error> { > check_ns_remote_datastore_privs(params, target_namespace, PRIV_REMOTE_DATASTORE_PRUNE) > - .map_err(|err| format_err!("Pruning remote datastore contents not allowed - {err}"))?; > + .context("Pruning remote datastore contents not allowed")?; > > let api_path = params.target.datastore_api_path("snapshots"); > let mut args = serde_json::to_value(snapshot)?; > if !target_namespace.is_root() { > args["ns"] = serde_json::to_value(target_namespace)?; > } > - params.target.client.delete(&api_path, Some(args)).await?; > + params > + .target > + .client > + .delete(&api_path, Some(args)) > + .await > + .context("Failed to remove remote snapshot, remote returned error")?; > > Ok(()) > } > @@ -709,7 +733,7 @@ pub(crate) async fn push_group( > ); > } > Err(err) => { > - warn!("Encountered errors: {err}"); > + warn!("Encountered errors: {err:#}"); > warn!( > "Failed to remove vanished snapshot {name} from remote!", > name = snapshot.backup > @@ -754,7 +778,7 @@ pub(crate) async fn push_snapshot( > Ok((manifest, _raw_size)) => manifest, > Err(err) => { > // No manifest in snapshot or failed to read, warn and skip > - log::warn!("Encountered errors: {err}"); > + log::warn!("Encountered errors: {err:#}"); > log::warn!("Failed to load manifest for '{snapshot}'!"); > return Ok(stats); > } > -- > 2.39.5 > > > > _______________________________________________ > 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