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

support for s3 express one zone #53

Merged
merged 3 commits into from
Apr 24, 2024
Merged

Conversation

duzinkie
Copy link
Contributor

https://aws.amazon.com/s3/storage-classes/express-one-zone/ is a new type of s3 bucket which offers better performance, lower cost for some use cases. It's mostly compatible with regular s3 api with a few caveats:

  • listBuckets operation would not return buckets of s3 express one zone type (aka directory buckets)
  • the objects in the bucket use their own storage class, so setting it explicitly to reduced redundancy/standard would cause a store request to fail.

This PR fixes both by:

  • swapping listBuckets operation to headBucket. This also:
    • lowers the permissions required by the IAM role that the plugin attempts to use, as no listBucket permission is needed anymore
    • allows for cross-account bucket access without having to assume a role in the account owning the bucket (aws s3 allows cross account access to bucket objects, but that would not results in request to list all buckets to list ones in different accounts)
  • don't set the storage class in the putobject request if it's not set to reduced redundancy. this makes the storage class default to appropriate one depending on the type of the bucket (standard for s3, default express one zone class for directory buckets (which name I can't remember :)))

lmk if I missed anything in the PR, or anything you'd like changes.
I did change gradle/verification-metadata.xml by running gradle xyz --write-verification-metadata sha256 but idk if that was the right thing to do, I might have not studied the gradle manual well enough (I thought the update is legit and caused by bumping the version of aws sdk as per libs.versions.toml)


Lastly, wanted to ask about error handling in the s3 cache and what's the rationale behind current implementation, which logs cache load/store errors (basically fails silently) instead of relying on throwing BuildCacheExceptions (and or other classes) and depending on gradle to fail the build/disable the cache as needed.

I'm referring to the following parts of gradle default http cache implementation:

lmk if this should be a separate issue, and or whether contributions are welcome, but I was thinking if it'd be ok to switch to using those. (I'd prefer explicit errors when storing objects fails for let's say auth reasons, but the same error handling pattern is present in gcp plugin so I'm not sure if there's a good reason behind that and whether you'd be fine with s3 handling it different than gcp)

@duzinkie
Copy link
Contributor Author

I see this has been sitting for a while with one approval - is there something else you'd like me to do to have this merged?

@duzinkie
Copy link
Contributor Author

could someone merge now that all approval are in place?
ty

@liutikas liutikas merged commit 24d137f into androidx:main Apr 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants