Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(services/gcs): Impl content-encoding support for GCS stat, write and presign #5610

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

wlinna
Copy link

@wlinna wlinna commented Feb 7, 2025

Which issue does this PR close?

Closes #5607

Rationale for this change

Explained in the issue

What changes are included in this PR?

  • Implements content_encoding support for Gcs for stat, write and presign.
  • Adds stat_has_content_encoding: true and write_with_content_encoding: true to capabilities of GcsBackend
  • Added contentEncoding check to test_deserialize_get_object_json_response

NOTE: I haven't tested presign yet with these changes yet, which is why I added WIP.

Are there any user-facing changes?

If the users were relying on Gcs service to ignore content_encoding, then they might have to update their code.

@wlinna wlinna requested a review from Xuanwo as a code owner February 7, 2025 10:07
@wlinna
Copy link
Author

wlinna commented Feb 7, 2025

Do I need to add some other tests? So far I have tested this with my own program and stat and write work as expected

@Xuanwo
Copy link
Member

Xuanwo commented Feb 7, 2025

Do I need to add some other tests? So far I have tested this with my own program and stat and write work as expected

We have integration tests for gcs, so it should fine as is.

@wlinna
Copy link
Author

wlinna commented Feb 7, 2025

I just realized that although I've added support for presign within Gcs service, FuturePresignWrite itself does not have content_encoding function.

I wonder if me adding content_encoding to FuturePresignWrite would be beyond the scope of this simple PR? Adding the following function there doesn't seem too complicated though:

    /// Set the content encoding of the operation
    pub fn content_encoding(self, v: &str) -> Self {
        self.map(|(args, dur)| (args.with_content_encoding(v), dur))
    }

@wlinna wlinna marked this pull request as draft February 7, 2025 13:01
@wlinna wlinna changed the title WIP: feat(services/gcs): Impl content-encoding support for GCS stat, write and presign feat(services/gcs): Impl content-encoding support for GCS stat, write and presign Feb 7, 2025
@Xuanwo
Copy link
Member

Xuanwo commented Feb 7, 2025

I wonder if me adding content_encoding to FuturePresignWrite would be beyond the scope of this simple PR? Adding the following function there doesn't seem too complicated though:

It's looks fine to me. Feel free to add in this PR directly.

@wlinna
Copy link
Author

wlinna commented Feb 7, 2025

I will add it, but I'm not able to test it, because the Gcs service ACL logic is problematic, and I get an error like this when testing:

<?xml version='1.0' encoding='UTF-8'?>
<Error>
	<Code>InvalidArgument</Code>
	<Message>Invalid argument.</Message>
	<Details>Invalid canned acl: publicRead</Details>
</Error>

It turns out that the XML API predefined ACLs uses kebab casing instead of camel casing, and thus publicRead should be converted to public-read when signing urls.

https://cloud.google.com/storage/docs/access-control/lists#predefined-acl

@wlinna wlinna marked this pull request as ready for review February 7, 2025 14:00
@wlinna
Copy link
Author

wlinna commented Feb 7, 2025

Okay, I tested presign without predefinedAcl, and now it works. That problem should be addressed in a separate PR.

In my opinion this PR is ready to be merged

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM

core/src/services/gcs/backend.rs Outdated Show resolved Hide resolved
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @wlinna for working on this. It's really happy to work with you.

@wlinna
Copy link
Author

wlinna commented Feb 7, 2025

Thank you @wlinna for working on this. It's really happy to work with you.

Thanks for making it easy to contribute :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new feature: Allow setting Content-Encoding in GCS (at least when writing)
2 participants