Skip to content

Update sseType when creating S3Context with KmsKeyId #23299

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

Merged
electrum merged 1 commit into from
Sep 5, 2024
Merged

Update sseType when creating S3Context with KmsKeyId #23299

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/trino-filesystem-s3/src/main/java/io/trino/filesystem/s3/S3Context.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public RequestPayer requestPayer()

public S3Context withKmsKeyId(String kmsKeyId)
{
return new S3Context(partSize, requesterPays, sseType, kmsKeyId, credentialsProviderOverride, cannedAcl, exclusiveWriteSupported);
Copy link
Copy Markdown
Contributor

@wendigo wendigo Sep 5, 2024

Choose a reason for hiding this comment

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

verify(sseType == NONE, "Cannot set KMS when sseType is %s", sseType)

Copy link
Copy Markdown
Contributor

@wendigo wendigo Sep 5, 2024

Choose a reason for hiding this comment

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

Let's verify it properly

Copy link
Copy Markdown
Member Author

@Praveen2112 Praveen2112 Sep 5, 2024

Choose a reason for hiding this comment

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

But it would have a restriction on S3SecurityMapping side that all the buckets should be under this KMS - For instance we could have one bucket which has S3 security mapping and we could have one bucket which could have KMS type SSE which could be mapped to specific users - If we add a verification they we might not be able access bucket with different SSE settings. Or am I missing something here ?

Copy link
Copy Markdown
Contributor

@wendigo wendigo Sep 5, 2024

Choose a reason for hiding this comment

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

Isn't the filesystem created per bucket?

Copy link
Copy Markdown
Member Author

@Praveen2112 Praveen2112 Sep 5, 2024

Choose a reason for hiding this comment

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

In case of S3SecurityMapping or in general ? But the configs are shared right ?

Copy link
Copy Markdown
Member

@electrum electrum Sep 5, 2024

Choose a reason for hiding this comment

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

In general, the TrinoFileSystemFactory is created once at injection time and then TrinoFileSystem is created at each usage site with ConnectorIdentity.

S3 security mapping uses SwitchingFileSystemFactory with S3FileSystemLoader to create a separate S3SecurityMappingFileSystemFactory per location.

Copy link
Copy Markdown
Member

@electrum electrum Sep 5, 2024

Choose a reason for hiding this comment

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

This change is correct. The purpose of S3 security mapping is to override the security settings for that mapping. So you could configure s3.sse.type=S3 as the global default and override for a specific bucket, path, etc.

return new S3Context(partSize, requesterPays, S3SseType.KMS, kmsKeyId, credentialsProviderOverride, cannedAcl, exclusiveWriteSupported);
}

public S3Context withCredentials(ConnectorIdentity identity)
Expand Down