From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 75F9F1FF164 for ; Fri, 22 Nov 2024 13:26:48 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 071A011F96; Fri, 22 Nov 2024 13:26:57 +0100 (CET) From: Christian Ebner To: pbs-devel@lists.proxmox.com Date: Fri, 22 Nov 2024 13:26:04 +0100 Message-Id: <20241122122604.335901-1-c.ebner@proxmox.com> X-Mailer: git-send-email 2.39.5 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.030 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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] [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" 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 Result { 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 { - 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