Re: [pbs-devel] [PATCH proxmox v3 3/4] s3 client: implement list buckets method

["Lukas Wagner" <l.wagner@proxmox.com>]

On Thu Jul 31, 2025 at 12:48 PM CEST, Christian Ebner wrote:
> Extends the client by the list buckets method which allows to
> fetch the buckets owned by the user with who's access key the
> request is signed.
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> changes since version 2:
> - no changes
>
> changes since version 1:
> - fixed parsing issue
>
>  proxmox-s3-client/src/client.rs          | 14 +++++-
>  proxmox-s3-client/src/response_reader.rs | 56 ++++++++++++++++++++++++
>  2 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/proxmox-s3-client/src/client.rs b/proxmox-s3-client/src/client.rs
> index 91188f1a..a2c62811 100644
> --- a/proxmox-s3-client/src/client.rs
> +++ b/proxmox-s3-client/src/client.rs
> @@ -28,7 +28,7 @@ use crate::aws_sign_v4::{aws_sign_v4_signature, aws_sign_v4_uri_encode};
>  use crate::object_key::S3ObjectKey;
>  use crate::response_reader::{
>      CopyObjectResponse, DeleteObjectsResponse, GetObjectResponse, HeadObjectResponse,
> -    ListObjectsV2Response, PutObjectResponse, ResponseReader,
> +    ListBucketsResponse, ListObjectsV2Response, PutObjectResponse, ResponseReader,
>  };
>  
>  const S3_HTTP_CONNECT_TIMEOUT: Duration = Duration::from_secs(10);
> @@ -319,6 +319,18 @@ impl S3Client {
>          Ok(())
>      }
>  
> +    /// List all buckets owned by the user authenticated via the access key.
> +    /// See reference docs: https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListBuckets.html
> +    pub async fn list_buckets(&self) -> Result<ListBucketsResponse, Error> {
> +        let request = Request::builder()
> +            .method(Method::GET)
> +            .uri(self.build_uri("/", &[])?)
> +            .body(Body::empty())?;
> +        let response = self.send(request).await?;
> +        let response_reader = ResponseReader::new(response);
> +        response_reader.list_buckets_response().await
> +    }
> +
>      /// Fetch metadata from an object without returning the object itself.
>      /// See reference docs: https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html
>      pub async fn head_object(
> diff --git a/proxmox-s3-client/src/response_reader.rs b/proxmox-s3-client/src/response_reader.rs
> index a7c71639..f9d7f1ac 100644
> --- a/proxmox-s3-client/src/response_reader.rs
> +++ b/proxmox-s3-client/src/response_reader.rs
> @@ -153,6 +153,38 @@ pub struct CopyObjectResult {
>      pub last_modified: LastModifiedTimestamp,
>  }
>  
> +/// Subset of the list buckets response
> +/// https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListBuckets.html#API_ListBuckets_ResponseElements
> +#[derive(Deserialize, Debug)]
> +#[serde(rename_all = "PascalCase")]
> +pub struct ListBucketsResponse {
> +    pub buckets: Vec<Bucket>,

Missing doc-comments for `pbu` struct member.

> +}
> +/// Subset of the list buckets response
> +/// https://docs.aws.amazon.com/AmazonS3/latest/API/API_ListBuckets.html#API_ListBuckets_ResponseElements
> +#[derive(Deserialize, Debug)]
> +#[serde(rename_all = "PascalCase")]
> +pub struct ListAllMyBucketsResult {
> +    pub buckets: Option<Buckets>,
> +}
> +
> +/// Subset used to deserialize the list buckets response
> +#[derive(Deserialize, Debug)]
> +#[serde(rename_all = "PascalCase")]
> +pub struct Buckets {
> +    bucket: Vec<Bucket>,
> +}
> +
> +/// Subset used to deserialize the list buckets response
> +#[derive(Deserialize, Debug)]
> +#[serde(rename_all = "PascalCase")]
> +pub struct Bucket {
> +    pub name: String,
> +    pub bucket_arn: Option<String>,
> +    pub bucket_region: Option<String>,
> +    pub creation_date: LastModifiedTimestamp,

Missing doc-comments for `pub` struct member.

> +}
> +
>  impl ResponseReader {
>      pub(crate) fn new(response: Response<Incoming>) -> Self {
>          Self { response }
> @@ -339,6 +371,30 @@ impl ResponseReader {
>          })
>      }
>  

Missing doc comment for this function here.

> +    pub(crate) async fn list_buckets_response(self) -> Result<ListBucketsResponse, Error> {
> +        let (parts, body) = self.response.into_parts();
> +        let body = body.collect().await?.to_bytes();
> +
> +        match parts.status {
> +            StatusCode::OK => (),
> +            status_code => {
> +                Self::log_error_response_utf8(body);
> +                bail!("unexpected status code {status_code}")
> +            }
> +        };

This 'match' reads a bit weird to me, how about:

if !matches!(parts.status, Status::OK) {
    ...
    bail!(...)
}

> +
> +        let body = String::from_utf8(body.to_vec())?;
> +
> +        let list_buckets_result: ListAllMyBucketsResult =
> +            serde_xml_rs::from_str(&body).context("failed to parse response body")?;
> +
> +        let buckets = match list_buckets_result.buckets {
> +            Some(buckets) => buckets.bucket,
> +            None => Vec::new(),
> +        };

This could be 

let buckets = list_buckets_results.buckets.map(|b| b.bucket).unwrap_or_default();

and be a bit shorter, but no hard feelings.

> +        Ok(ListBucketsResponse { buckets })
> +    }
> +
>      fn log_error_response_utf8(body: Bytes) {
>          if let Ok(body) = String::from_utf8(body.to_vec()) {
>              if !body.is_empty() {

Otherwise it 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