add-support-for-s3-express-zone-directory-buckets#168
add-support-for-s3-express-zone-directory-buckets#168Gargi-thakur01 wants to merge 1 commit intomasterfrom
Conversation
|
Question-
|
| // determine whether the bucket is an S3 Express directory bucket. It returns | ||
| // false on any error so that MinIO and non-AWS endpoints fall back gracefully | ||
| // to the existing general-purpose bucket code path. | ||
| func detectDirectoryBucket(ctx context.Context, client *s3.Client, bucket string) bool { |
There was a problem hiding this comment.
this detection mechanism (HeadBucket) is overkill and introduces an extra API call on every New() invocation for all users (not just S3 Express users), and requires an extra IAM permission s3:HeadBucket that wasn't needed before. although it silently falls back for regular buckets for backward compatibility, it's still overkill
the entire detectDirectoryBucket() function can be deleted and the isDirectoryBucket field on Backend struct is not needed either
pr's goal is right but the implementation needs to be significantly simplified
just use strings.HasSuffix(bucket, "--x-s3") like how the AWS SDK itself does (https://github.com/aws/aws-sdk-go-v2/blob/main/service/s3/express_user_agent.go#L27)
to detect the bucket type and inline the ACL and DSSE-KMS checks directly in Put() using the suffix check
this works universally because S3 Express One Zone is a proprietary AWS feature and it's not part of the S3 API specification that other providers implement (MinIO, R2, etc.), so no user of those providers would ever have a bucket ending in --x-s3. the suffix is AWS-enforced at bucket creation time, making it a zero-cost and reliable signal
There was a problem hiding this comment.
I planned the same before but it was suggested by Hemanth, Satya and Maanav before to use bucket metadata instead to detect this- Tech-Spec-Link
There was a problem hiding this comment.
thanks! I had considered the suffix approach earlier but understand the preference for bucket metadata detection. that said at the very least the HeadBucket call should be gated behind the suffix check to avoid the extra API call for all regular bucket users. right now it calls HeadBucket for every single bucket regardless of type which adds unnecessary latency
this is also how AWS's own k8s s3 controller handles it with pure suffix check, no API call: https://github.com/aws-controllers-k8s/s3-controller/blob/main/pkg/resource/bucket/hook.go#L38
HeadBucket on every New() is worth fixing regardless of which detection approach we go with
Ans - YES, It works transparently with S3 Express buckets specified in account/project-level cache settings. I manually tested for both cache intelligence and explicit save/restore steps.
Ans - Yes, Some additional IAM permissions are required to be configured:
Connector setup: Same AWS connector works - just ensure IAM user/role has these two permissions. No special ARN configuration needed beyond standard access key or role assumption. |
We should update documentation for this requirement Also why are we not using commit format- type: [JIRA-TICKET]: Description |
Please read CONTRIBUTING.md for details on our code of conduct, and the process for submitting pull requests to us.
Proposed Changes
This PR adds support for AWS S3 Express One Zone directory buckets to the drone-cache plugin.
Description
AWS S3 Express One Zone is a high-performance, single-AZ storage class that uses directory buckets with specific API limitations. This implementation:
Automatic Directory Bucket Detection
S3 Express API Compatibility
Preserves existing behavior for general-purpose buckets
Technical Changes
Testing
Checklist