[pbs-devel] [PATCH proxmox{, -backup} 0/4] fix #7078: Add quirk for providers not supporting deleteObjects

[pbs-devel] [PATCH proxmox{, -backup} 0/4] fix #7078: Add quirk for providers not supporting deleteObjects

[Christian Ebner]

These patches provide a workaround to users which would like to
use the S3 datastore backend with object store API's not implementing
the deleteObjects API method, such as e.g. Google Cloud Storage.

When the quirk is set via the S3 client options, delete objects calls
are performed as individual delete object calls instead.

Patches therefore restructure the s3-client code such that the delete
objects response can be generated and the deletion is performed by
looping over the delete list, performing single object deletions.

On the Proxmox Backup Server side the new, additional quirk is exposed
by reworking the advanced section in the s3 endpoint edit window
to use a fieldset of checkboxes instead of the dropdown selector,
with the intend to improve usability.

Link to the bugtracker issue:
https://bugzilla.proxmox.com/show_bug.cgi?id=7078

proxmox:

Christian Ebner (3):
  s3-client: factor out optional response header parsing
  s3-client: parse and return headers for delete object response
  s3-client: extend provider quirks by delete objects via delete object

 proxmox-s3-client/src/api_types.rs       |  2 +
 proxmox-s3-client/src/client.rs          | 38 +++++++++++--
 proxmox-s3-client/src/response_reader.rs | 68 +++++++++++++++---------
 3 files changed, 80 insertions(+), 28 deletions(-)


proxmox-backup:

Christian Ebner (1):
  fix #7078: ui: exponse DeleteObjects via DeleteObject provider quirk

 www/window/S3ClientEdit.js | 57 ++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 14 deletions(-)


Summary over all repositories:
  4 files changed, 123 insertions(+), 42 deletions(-)

-- 
Generated by git-murpp 0.8.1


_______________________________________________
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: factor out optional response header parsing

[Christian Ebner]

Introduce an associated function to parse optional headers, currently
only present for the date header. Further, reduce code duplication
by using the same function also for the parsing of required headers.

Will be used to parse optional headers set in the delete object
response so they can be mapped to look like a delete objects response
when adding a provider quirk to perform delete objects via individual
delete object calls.

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

diff --git a/proxmox-s3-client/src/response_reader.rs b/proxmox-s3-client/src/response_reader.rs
index e03b3bb0..7066c33b 100644
--- a/proxmox-s3-client/src/response_reader.rs
+++ b/proxmox-s3-client/src/response_reader.rs
@@ -251,7 +251,7 @@ impl ResponseReader {
 
         let body = String::from_utf8(body.to_vec())?;
 
-        let date = Self::parse_optional_date_header(&parts.headers)?;
+        let date = Self::parse_optional_header(header::DATE, &parts.headers)?;
 
         let response: ListObjectsV2ResponseBody =
             serde_xml_rs::from_str(&body).context("failed to parse response body")?;
@@ -282,7 +282,7 @@ impl ResponseReader {
         let content_length: u64 = Self::parse_header(header::CONTENT_LENGTH, &parts.headers)?;
         let content_type = Self::parse_header(header::CONTENT_TYPE, &parts.headers)?;
         let e_tag = Self::parse_header(header::ETAG, &parts.headers)?;
-        let date = Self::parse_optional_date_header(&parts.headers)?;
+        let date = Self::parse_optional_header(header::DATE, &parts.headers)?;
         let last_modified = Self::parse_header(header::LAST_MODIFIED, &parts.headers)?;
 
         Ok(Some(HeadObjectResponse {
@@ -314,7 +314,7 @@ impl ResponseReader {
         let content_length: u64 = Self::parse_header(header::CONTENT_LENGTH, &parts.headers)?;
         let content_type = Self::parse_header(header::CONTENT_TYPE, &parts.headers)?;
         let e_tag = Self::parse_header(header::ETAG, &parts.headers)?;
-        let date = Self::parse_optional_date_header(&parts.headers)?;
+        let date = Self::parse_optional_header(header::DATE, &parts.headers)?;
         let last_modified = Self::parse_header(header::LAST_MODIFIED, &parts.headers)?;
 
         Ok(Some(GetObjectResponse {
@@ -477,30 +477,30 @@ impl ResponseReader {
         <T as FromStr>::Err: Send + Sync + 'static,
         Result<T, <T as FromStr>::Err>: Context<T, <T as FromStr>::Err>,
     {
-        let header_value = headers
-            .get(&name)
+        let value = Self::parse_optional_header(name.clone(), headers)?
             .ok_or_else(|| anyhow!("missing header '{name}'"))?;
-        let header_str = header_value
-            .to_str()
-            .with_context(|| format!("non UTF-8 header '{name}'"))?;
-        let value = header_str
-            .parse()
-            .with_context(|| format!("failed to parse header '{name}'"))?;
         Ok(value)
     }
 
-    fn parse_optional_date_header(headers: &HeaderMap) -> Result<Option<HttpDate>, Error> {
-        let header_value = match headers.get(header::DATE) {
+    fn parse_optional_header<T: FromStr>(
+        name: HeaderName,
+        headers: &HeaderMap,
+    ) -> Result<Option<T>, Error>
+    where
+        <T as FromStr>::Err: Send + Sync + 'static,
+        Result<T, <T as FromStr>::Err>: Context<T, <T as FromStr>::Err>,
+    {
+        let header_value = match headers.get(&name) {
             Some(value) => value,
             None => return Ok(None),
         };
         let header_str = header_value
             .to_str()
-            .with_context(|| format!("non UTF-8 header '{}'", header::DATE))?;
-        let date: HttpDate = header_str
+            .with_context(|| format!("non UTF-8 header '{name}'"))?;
+        let value = header_str
             .parse()
-            .with_context(|| format!("failed to parse header '{}'", header::DATE))?;
-        Ok(Some(date))
+            .with_context(|| format!("failed to parse header '{name}'"))?;
+        Ok(Some(value))
     }
 }
 
@@ -615,7 +615,8 @@ fn test_optional_date_header_parsing() {
 
     let expected_date = "Wed, 12 Oct 2009 17:50:00 GMT";
     header_map.insert(header::DATE, expected_date.parse().unwrap());
-    let parsed_date = ResponseReader::parse_optional_date_header(&header_map).unwrap();
+    let parsed_date: Option<HttpDate> =
+        ResponseReader::parse_optional_header(header::DATE, &header_map).unwrap();
     assert!(parsed_date.is_some());
     assert_eq!(
         parsed_date.unwrap(),
@@ -625,10 +626,12 @@ fn test_optional_date_header_parsing() {
     header_map.clear();
     let invalid_date_format = "2019-11-10";
     header_map.insert(header::DATE, invalid_date_format.parse().unwrap());
-    assert!(ResponseReader::parse_optional_date_header(&header_map).is_err());
+    assert!(ResponseReader::parse_optional_header::<HttpDate>(header::DATE, &header_map).is_err());
 
     header_map.clear();
-    assert!(ResponseReader::parse_optional_date_header(&header_map)
-        .unwrap()
-        .is_none());
+    assert!(
+        ResponseReader::parse_optional_header::<HttpDate>(header::DATE, &header_map)
+            .unwrap()
+            .is_none()
+    );
 }
-- 
2.47.3



_______________________________________________
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: parse and return headers for delete object response

[Christian Ebner]

Mimic the response elements from the list objects parsing in the
delete object response.

In preparation for being able to perform delete objects via
individual delete object api calls, if the respective provider quirk
is set in the client options.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-s3-client/src/client.rs          |  9 +++++----
 proxmox-s3-client/src/response_reader.rs | 21 +++++++++++++++++++--
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/proxmox-s3-client/src/client.rs b/proxmox-s3-client/src/client.rs
index 83176b39..3d0af5d6 100644
--- a/proxmox-s3-client/src/client.rs
+++ b/proxmox-s3-client/src/client.rs
@@ -29,8 +29,9 @@ use crate::aws_sign_v4::AWS_SIGN_V4_DATETIME_FORMAT;
 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,
-    ListBucketsResponse, ListObjectsV2Response, PutObjectResponse, ResponseReader,
+    CopyObjectResponse, DeleteObjectsResponse, DeletedObject, GetObjectResponse,
+    HeadObjectResponse, ListBucketsResponse, ListObjectsV2Response, PutObjectResponse,
+    ResponseReader,
 };
 
 /// Default timeout for s3 api requests.
@@ -544,7 +545,7 @@ impl S3Client {
 
     /// Removes an object from a bucket.
     /// See reference docs: https://docs.aws.amazon.com/AmazonS3/latest/API/API_DeleteObject.html
-    pub async fn delete_object(&self, object_key: S3ObjectKey) -> Result<(), Error> {
+    pub async fn delete_object(&self, object_key: S3ObjectKey) -> Result<DeletedObject, Error> {
         let object_key = object_key.to_full_key(&self.options.common_prefix);
         let request = Request::builder()
             .method(Method::DELETE)
@@ -553,7 +554,7 @@ impl S3Client {
 
         let response = self.send(request, None).await?;
         let response_reader = ResponseReader::new(response);
-        response_reader.delete_object_response().await
+        response_reader.delete_object_response(object_key).await
     }
 
     /// Delete multiple objects from a bucket using a single HTTP request.
diff --git a/proxmox-s3-client/src/response_reader.rs b/proxmox-s3-client/src/response_reader.rs
index 7066c33b..be7c0950 100644
--- a/proxmox-s3-client/src/response_reader.rs
+++ b/proxmox-s3-client/src/response_reader.rs
@@ -361,7 +361,10 @@ impl ResponseReader {
     /// Read and parse the delete object response.
     ///
     /// Returns with error if an unexpected status code is encountered.
-    pub(crate) async fn delete_object_response(self) -> Result<(), Error> {
+    pub(crate) async fn delete_object_response(
+        self,
+        key: S3ObjectKey,
+    ) -> Result<DeletedObject, Error> {
         let (parts, _body) = self.response.into_parts();
 
         match parts.status {
@@ -369,7 +372,21 @@ impl ResponseReader {
             status_code => bail!("unexpected status code {status_code}"),
         };
 
-        Ok(())
+        let delete_marker = Self::parse_optional_header(
+            HeaderName::from_static("x-amz-delete-marker"),
+            &parts.headers,
+        )?;
+        let delete_marker_version_id = Self::parse_optional_header(
+            HeaderName::from_static("x-amz-version-id"),
+            &parts.headers,
+        )?;
+
+        Ok(DeletedObject {
+            delete_marker,
+            delete_marker_version_id,
+            key: Some(key),
+            version_id: None,
+        })
     }
 
     /// Read and parse the delete objects response.
-- 
2.47.3



_______________________________________________
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: extend provider quirks by delete objects via delete object

[Christian Ebner]

Provide a workaround for providers not implementing the delete objects
S3 API method.

If ProviderQuirks::DeleteObjectsViaDeleteObject is set,
S3Client::delete_objects() performs individual
S3Client::delete_object() calls on each provided object key instead
of deleting the keys via the deleteObjects API call.

This can also be used to reduce POST calls in favor of multiple
DELETE calls, which might be charged differently by some S3 object
store providers.

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

diff --git a/proxmox-s3-client/src/api_types.rs b/proxmox-s3-client/src/api_types.rs
index 93573fbd..b4fc24e0 100644
--- a/proxmox-s3-client/src/api_types.rs
+++ b/proxmox-s3-client/src/api_types.rs
@@ -87,6 +87,8 @@ pub const S3_BUCKET_NAME_SCHEMA: Schema = StringSchema::new("Bucket name for S3
 pub enum ProviderQuirks {
     /// Prvider does not support the If-None-Match http header
     SkipIfNoneMatchHeader,
+    /// Prvider does not support DeleteObjects API endpoint, use delete object calls instead
+    DeleteObjectsViaDeleteObject,
 }
 serde_plain::derive_display_from_serialize!(ProviderQuirks);
 serde_plain::derive_fromstr_from_deserialize!(ProviderQuirks);
diff --git a/proxmox-s3-client/src/client.rs b/proxmox-s3-client/src/client.rs
index 3d0af5d6..a6a3c5f9 100644
--- a/proxmox-s3-client/src/client.rs
+++ b/proxmox-s3-client/src/client.rs
@@ -29,7 +29,7 @@ use crate::aws_sign_v4::AWS_SIGN_V4_DATETIME_FORMAT;
 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, DeletedObject, GetObjectResponse,
+    CopyObjectResponse, DeleteObjectError, DeleteObjectsResponse, DeletedObject, GetObjectResponse,
     HeadObjectResponse, ListBucketsResponse, ListObjectsV2Response, PutObjectResponse,
     ResponseReader,
 };
@@ -567,6 +567,35 @@ impl S3Client {
             return Ok(DeleteObjectsResponse::default());
         }
 
+        if self
+            .options
+            .provider_quirks
+            .contains(&ProviderQuirks::DeleteObjectsViaDeleteObject)
+        {
+            let mut response = DeleteObjectsResponse::default();
+            response.deleted = Some(Vec::with_capacity(object_keys.len()));
+
+            for object_key in object_keys {
+                match self.delete_object(object_key.clone()).await {
+                    Ok(deleted_object) => {
+                        let deleted = response.deleted.get_or_insert(Vec::new());
+                        deleted.push(deleted_object);
+                    }
+                    Err(err) => {
+                        let mut errors = response.error.get_or_insert(Vec::new());
+                        errors.push(DeleteObjectError {
+                            code: None,
+                            key: Some(object_key.clone()),
+                            message: Some(format!("{err}")),
+                            version_id: None,
+                        });
+                    }
+                }
+            }
+
+            return Ok(response);
+        }
+
         let mut body = String::from(r#"<Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/">"#);
         for object_key in object_keys {
             body.push_str("<Object><Key>");
-- 
2.47.3



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

[pbs-devel] [PATCH proxmox-backup 1/1] fix #7078: ui: exponse DeleteObjects via DeleteObject provider quirk

[Christian Ebner]

The S3 compatible storage API for Google Cloud Storage does not
implement deleteObjects api method, even if mentioned in the docs,
although erratic [0] (incorrect anchor and docs switched with delete
object ones at the time of last check).

Provide users a workaround by allowing to set the provider quirk to
perform individual deleteObject calls instead of deleteObjects on the
list of objects to delete.

The same quirk might also be used when delete object calls are
monetarily cheaper as compared to the multi object deletion, at the
cost of additional api calls and all the added delays that implies.

As suggested when the provider quirks first got applied in [1],
instead of exposing the additional quirk via an added entry to the
dropdown selector, move to a fieldset with nested checkboxes for
better accessibility.

[0] https://cloud.google.com/distributed-cloud/hosted/docs/latest/gdch/apis/service/storage/storage-s3-rest-api#DeleteObjects
[1] https://lore.proxmox.com/pbs-devel/175440823229.3188344.11268683178675917633.b4-ty@proxmox.com/

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7078
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 www/window/S3ClientEdit.js | 57 ++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/www/window/S3ClientEdit.js b/www/window/S3ClientEdit.js
index 8862683b8..c7e8ade1f 100644
--- a/www/window/S3ClientEdit.js
+++ b/www/window/S3ClientEdit.js
@@ -135,20 +135,6 @@ Ext.define('PBS.window.S3ClientEdit', {
                 emptyText: gettext('Unlimited'),
                 submitAutoScaledSizeUnit: true,
             },
-            {
-                xtype: 'proxmoxKVComboBox',
-                name: 'provider-quirks',
-                fieldLabel: gettext('Provider Quirks'),
-                value: '__default__',
-                defaultValue: '__default__',
-                comboItems: [
-                    ['__default__', gettext('None (default)')],
-                    ['skip-if-none-match-header', gettext('Skip If-None-Match header')],
-                ],
-                cbind: {
-                    deleteEmpty: '{!isCreate}',
-                },
-            },
         ],
         advancedColumn2: [
             {
@@ -166,6 +152,27 @@ Ext.define('PBS.window.S3ClientEdit', {
                 submitAutoScaledSizeUnit: true,
             },
         ],
+        advancedColumnB: [
+            {
+                xtype: 'fieldset',
+                name: 'provider-quirks',
+                fieldLabel: gettext('Provider Quirks'),
+                items: [
+                    {
+                        xtype: 'checkbox',
+                        name: 'skip-if-none-match-header',
+                        fieldLabel: gettext('Skip If-None-Match header'),
+                        labelWidth: 200,
+                    },
+                    {
+                        xtype: 'checkbox',
+                        name: 'delete-objects-via-delete-object',
+                        fieldLabel: gettext('DeleteObjects via deleteObject'),
+                        labelWidth: 200,
+                    },
+                ],
+            },
+        ],
     },
 
     getValues: function () {
@@ -193,6 +200,28 @@ Ext.define('PBS.window.S3ClientEdit', {
             delete values['secret-key'];
         }
 
+        let quirks = [];
+        ['skip-if-none-match-header', 'delete-objects-via-delete-object'].forEach((quirk) => {
+            if (values[quirk]) {
+                quirks.push(quirk);
+                delete values[quirk];
+            }
+        });
+
+        if (quirks.length > 0) {
+            values['provider-quirks'] = quirks;
+        } else if (!me.isCreate) {
+            values.delete.push('provider-quirks');
+        }
+
         return values;
     },
+
+    setValues: function (values) {
+        if (values['provider-quirks']) {
+            values['provider-quirks'].forEach((quirk) => (values[quirk] = true));
+        }
+
+        this.callParent(arguments);
+    },
 });
-- 
2.47.3



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