[pbs-devel] [PATCH proxmox 0/3] s3 client: improve error logging for invalid requests

[pbs-devel] [PATCH proxmox 0/3] s3 client: improve error logging for invalid requests

[Christian Ebner]

This patches improve error logging by refactoring the logging logic
into a common helper and also covering responses with response status
BAD_REQUEST. This has not been covered previously and helps to
diagnose the reason for the bad request. In example, a malformed
request might return the following response body:

```
<?xml version="1.0" encoding="UTF-8"?>
<Error>
    <Code>MalformedXML</Code>
    <Message>The XML you provided was not well formed or did not validate against our published schema.</Message>
</Error>
```

Christian Ebner (3):
  s3 client: fix minor whitespace issue
  s3 client: refactor error response body into dedicated helper
  s3 client: log error response body for invalid request status codes

 proxmox-s3-client/src/api_types.rs       |  1 -
 proxmox-s3-client/src/response_reader.rs | 56 ++++++++++--------------
 2 files changed, 23 insertions(+), 34 deletions(-)

-- 
2.47.2



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

[pbs-devel] [PATCH proxmox 1/3] s3 client: fix minor whitespace issue

[Christian Ebner]

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-s3-client/src/api_types.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/proxmox-s3-client/src/api_types.rs b/proxmox-s3-client/src/api_types.rs
index e05ad0f9..2234f121 100644
--- a/proxmox-s3-client/src/api_types.rs
+++ b/proxmox-s3-client/src/api_types.rs
@@ -171,7 +171,6 @@ pub struct S3ClientConf {
     pub secret_key: String,
 }
 
-
 #[api(
     properties: {
         id: {
-- 
2.47.2



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

[pbs-devel] [PATCH proxmox 2/3] s3 client: refactor error response body into dedicated helper

[Christian Ebner]

Places body utf8 encoding and logging into a common place instead
of having it scattered over various places in the response status
checks.

No functional changes intended.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-s3-client/src/response_reader.rs | 46 ++++++++----------------
 1 file changed, 15 insertions(+), 31 deletions(-)

diff --git a/proxmox-s3-client/src/response_reader.rs b/proxmox-s3-client/src/response_reader.rs
index 41e8c6d8..7aed4201 100644
--- a/proxmox-s3-client/src/response_reader.rs
+++ b/proxmox-s3-client/src/response_reader.rs
@@ -2,7 +2,7 @@ use std::str::FromStr;
 
 use anyhow::{anyhow, bail, Context, Error};
 use http_body_util::BodyExt;
-use hyper::body::Incoming;
+use hyper::body::{Bytes, Incoming};
 use hyper::header::HeaderName;
 use hyper::http::header;
 use hyper::http::StatusCode;
@@ -166,11 +166,7 @@ impl ResponseReader {
             StatusCode::OK => (),
             StatusCode::NOT_FOUND => bail!("bucket does not exist"),
             status_code => {
-                if let Ok(body) = String::from_utf8(body.to_vec()) {
-                    if !body.is_empty() {
-                        tracing::error!("{body}");
-                    }
-                }
+                Self::log_error_response_utf8(body);
                 bail!("unexpected status code {status_code}")
             }
         }
@@ -193,11 +189,7 @@ impl ResponseReader {
             StatusCode::OK => (),
             StatusCode::NOT_FOUND => return Ok(None),
             status_code => {
-                if let Ok(body) = String::from_utf8(body.to_vec()) {
-                    if !body.is_empty() {
-                        tracing::error!("{body}");
-                    }
-                }
+                Self::log_error_response_utf8(body);
                 bail!("unexpected status code {status_code}")
             }
         }
@@ -229,11 +221,7 @@ impl ResponseReader {
             StatusCode::FORBIDDEN => bail!("object is archived and inaccessible until restored"),
             status_code => {
                 let body = content.collect().await?.to_bytes();
-                if let Ok(body) = String::from_utf8(body.to_vec()) {
-                    if !body.is_empty() {
-                        tracing::error!("{body}");
-                    }
-                }
+                Self::log_error_response_utf8(body);
                 bail!("unexpected status code {status_code}")
             }
         }
@@ -264,11 +252,7 @@ impl ResponseReader {
             StatusCode::CONFLICT => return Ok(PutObjectResponse::NeedsRetry),
             StatusCode::BAD_REQUEST => bail!("invalid request"),
             status_code => {
-                if let Ok(body) = String::from_utf8(body.to_vec()) {
-                    if !body.is_empty() {
-                        tracing::error!("{body}");
-                    }
-                }
+                Self::log_error_response_utf8(body);
                 bail!("unexpected status code {status_code}")
             }
         };
@@ -301,11 +285,7 @@ impl ResponseReader {
             StatusCode::OK => (),
             StatusCode::BAD_REQUEST => bail!("invalid request"),
             status_code => {
-                if let Ok(body) = String::from_utf8(body.to_vec()) {
-                    if !body.is_empty() {
-                        tracing::error!("{body}");
-                    }
-                }
+                Self::log_error_response_utf8(body);
                 bail!("unexpected status code {status_code}")
             }
         };
@@ -327,11 +307,7 @@ impl ResponseReader {
             StatusCode::NOT_FOUND => bail!("object not found"),
             StatusCode::FORBIDDEN => bail!("the source object is not in the active tier"),
             status_code => {
-                if let Ok(body) = String::from_utf8(body.to_vec()) {
-                    if !body.is_empty() {
-                        tracing::error!("{body}");
-                    }
-                }
+                Self::log_error_response_utf8(body);
                 bail!("unexpected status code {status_code}")
             }
         }
@@ -357,6 +333,14 @@ impl ResponseReader {
         })
     }
 
+    fn log_error_response_utf8(body: Bytes) {
+        if let Ok(body) = String::from_utf8(body.to_vec()) {
+            if !body.is_empty() {
+                tracing::error!("{body}");
+            }
+        }
+    }
+
     fn parse_header<T: FromStr>(name: HeaderName, headers: &HeaderMap) -> Result<T, Error>
     where
         <T as FromStr>::Err: Send + Sync + 'static,
-- 
2.47.2



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

[pbs-devel] [PATCH proxmox 3/3] s3 client: log error response body for invalid request status codes

[Christian Ebner]

While the response body is already logged via `tracing` in case of
unexpected status codes, this is not the case for invalid request
response codes.

Log the error response body for these cases as well, as they might
contain further information to help debug issues. For example,
an incorrectly composed delete_objects call might return the
following error in the response body (reformatted from single line
to multi line for better readability in the commit message):

```
<?xml version="1.0" encoding="UTF-8"?>
<Error>
    <Code>MalformedXML</Code>
    <Message>The XML you provided was not well formed or did not validate against our published schema.</Message>
</Error>
```

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-s3-client/src/response_reader.rs | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/proxmox-s3-client/src/response_reader.rs b/proxmox-s3-client/src/response_reader.rs
index 7aed4201..d8ca3b19 100644
--- a/proxmox-s3-client/src/response_reader.rs
+++ b/proxmox-s3-client/src/response_reader.rs
@@ -250,7 +250,10 @@ impl ResponseReader {
             StatusCode::OK => (),
             StatusCode::PRECONDITION_FAILED => return Ok(PutObjectResponse::PreconditionFailed),
             StatusCode::CONFLICT => return Ok(PutObjectResponse::NeedsRetry),
-            StatusCode::BAD_REQUEST => bail!("invalid request"),
+            StatusCode::BAD_REQUEST => {
+                Self::log_error_response_utf8(body);
+                bail!("invalid request");
+            }
             status_code => {
                 Self::log_error_response_utf8(body);
                 bail!("unexpected status code {status_code}")
@@ -283,7 +286,10 @@ impl ResponseReader {
 
         match parts.status {
             StatusCode::OK => (),
-            StatusCode::BAD_REQUEST => bail!("invalid request"),
+            StatusCode::BAD_REQUEST => {
+                Self::log_error_response_utf8(body);
+                bail!("invalid request");
+            }
             status_code => {
                 Self::log_error_response_utf8(body);
                 bail!("unexpected status code {status_code}")
-- 
2.47.2



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

Re: [pbs-devel] [PATCH proxmox 0/3] s3 client: improve error logging for invalid requests

[Lukas Wagner]

On Wed Jul 23, 2025 at 12:25 PM CEST, Christian Ebner wrote:
> This patches improve error logging by refactoring the logging logic
> into a common helper and also covering responses with response status
> BAD_REQUEST. This has not been covered previously and helps to
> diagnose the reason for the bad request. In example, a malformed
> request might return the following response body:
>
> ```
> <?xml version="1.0" encoding="UTF-8"?>
> <Error>
>     <Code>MalformedXML</Code>
>     <Message>The XML you provided was not well formed or did not validate against our published schema.</Message>
> </Error>
> ```
>
> Christian Ebner (3):
>   s3 client: fix minor whitespace issue
>   s3 client: refactor error response body into dedicated helper
>   s3 client: log error response body for invalid request status codes
>
>  proxmox-s3-client/src/api_types.rs       |  1 -
>  proxmox-s3-client/src/response_reader.rs | 56 ++++++++++--------------
>  2 files changed, 23 insertions(+), 34 deletions(-)

Looks good to me:

Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>


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

[pbs-devel] applied: [PATCH proxmox 0/3] s3 client: improve error logging for invalid requests

[Thomas Lamprecht]

On Wed, 23 Jul 2025 12:25:58 +0200, Christian Ebner wrote:
> This patches improve error logging by refactoring the logging logic
> into a common helper and also covering responses with response status
> BAD_REQUEST. This has not been covered previously and helps to
> diagnose the reason for the bad request. In example, a malformed
> request might return the following response body:
> 
> ```
> <?xml version="1.0" encoding="UTF-8"?>
> <Error>
>     <Code>MalformedXML</Code>
>     <Message>The XML you provided was not well formed or did not validate against our published schema.</Message>
> </Error>
> ```
> 
> [...]

Applied, thanks!

[1/3] s3 client: fix minor whitespace issue
      commit: 808b3725b63f82caf5fb49678593f7cf0aad1549
[2/3] s3 client: refactor error response body into dedicated helper
      commit: d63fe256beb4c961ea1258cb114c12b2cc4a3b0e
[3/3] s3 client: log error response body for invalid request status codes
      commit: 093dfeb8206aa3db405e27be92411794bcfc8780


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