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.

Sign up for GitHub

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

Jump to bottom

Add support for ACL config in native S3 file system #21176

Merged
merged 2 commits into from Mar 26, 2024
Merged

Add support for ACL config in native S3 file system #21176

merged 2 commits into from Mar 26, 2024

Conversation

anusudarsan
Copy link
Member

@anusudarsan anusudarsan commented Mar 20, 2024 •
edited

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X) Release notes are required, with the following suggested text:

# Section
* Add support for acl config in native s3 filesystem

@cla-bot cla-bot bot added the cla-signed label Mar 20, 2024
@anusudarsan anusudarsan force-pushed the anu/more-native-s3-options branch 2 times, most recently from e8de3f5 to e7b9117 Compare March 21, 2024 13:58
electrum
electrum reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

Let's remove the custom signer commit. We don't want to load arbitrary classes, plus there is the problem of how a user would actually use the feature and provide configuration. Is there a specific use case for this?

lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemConfig.java Outdated Show resolved Hide resolved
lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemConfig.java Outdated Show resolved Hide resolved
@anusudarsan anusudarsan changed the title Add custom signer and acl type configs to native s3 filesystem Add support for acl config in native s3 filesystem Mar 22, 2024
@anusudarsan
Copy link
Member Author

anusudarsan commented Mar 22, 2024 •
edited

Let's remove the custom signer commit. We don't want to load arbitrary classes, plus there is the problem of how a user would actually use the feature and provide configuration. Is there a specific use case for this?

I removed it from this PR. Im yet to hear on the actual use-case for custom signer.

@anusudarsan anusudarsan force-pushed the anu/more-native-s3-options branch 2 times, most recently from 648840a to 425ac97 Compare March 22, 2024 17:39
@anusudarsan
Copy link
Member Author

@findepi or @electrum can one of you trigger tests with secrets?

@electrum
Copy link
Member

/test-with-secrets sha=425ac97974b1f88bfbc2a3e8d9e3aafd5d9bef7e

@github-actions GitHub Actions
Copy link

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/8395787086

@github-actions github-actions bot added the docs label Mar 25, 2024
electrum
electrum reviewed Mar 25, 2024
View reviewed changes
lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3OutputStream.java Outdated Show resolved Hide resolved
lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemConfig.java Outdated Show resolved Hide resolved
docs/src/main/sphinx/object-storage/file-system-s3.md Outdated Show resolved Hide resolved
lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3FileSystemConfig.java Outdated
}

@Config("s3.canned-acl")
@ConfigDescription("Canned ACL (predefined grants) to manage access to buckets and objects")
Copy link
Member

Choose a reason for hiding this comment

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

Remove "buckets" since in this context it's only relevant for objects (canned ACLs can also be set at the bucket level but we're not doing that here)

Canned ACL (predefined grants) to manage access to objects

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, looks like this one was not updated

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry . missed it. updated now

@electrum
Copy link
Member

/test-with-secrets sha=0a6825ecec58fea7b6fce8fb8a1218579b756bcd

@github-actions GitHub Actions
Copy link

The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/8427028437

@findepi findepi mentioned this pull request Mar 25, 2024
Closed
@findepi findepi changed the title Add support for acl config in native s3 filesystem Add support for ACL config in native S3 filesystem Mar 25, 2024
@electrum electrum merged commit 1736da9 into trinodb:master Mar 26, 2024
53 of 61 checks passed
@github-actions github-actions bot added this to the 444 milestone Mar 26, 2024
@anusudarsan anusudarsan deleted the anu/more-native-s3-options branch March 26, 2024 18:28
@mosabua mosabua changed the title Add support for ACL config in native S3 filesystem Add support for ACL config in native S3 file system Mar 26, 2024
@mosabua mosabua mentioned this pull request Mar 26, 2024
Merged
@colebow colebow mentioned this pull request Mar 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electrum electrum electrum approved these changes

@findinpath findinpath findinpath left review comments

@findepi findepi Awaiting requested review from findepi

Assignees
No one assigned
Labels
cla-signed docs
Milestone
444
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@anusudarsan @electrum @findinpath