Update sseType when creating S3Context with KmsKeyId#23299
Update sseType when creating S3Context with KmsKeyId#23299electrum merged 1 commit intotrinodb:masterfrom
Conversation
SSE configuration is not applied correctly if we have a S3SecurityMapping which configures kmsKeyId for a given user and the default S3 Config has `s3.sse.type` set to NONE or S3.
|
|
||
| public S3Context withKmsKeyId(String kmsKeyId) | ||
| { | ||
| return new S3Context(partSize, requesterPays, sseType, kmsKeyId, credentialsProviderOverride, cannedAcl, exclusiveWriteSupported); |
There was a problem hiding this comment.
verify(sseType == NONE, "Cannot set KMS when sseType is %s", sseType)
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Isn't the filesystem created per bucket?
There was a problem hiding this comment.
In case of S3SecurityMapping or in general ? But the configs are shared right ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| public S3Context withKmsKeyId(String kmsKeyId) | ||
| { | ||
| return new S3Context(partSize, requesterPays, sseType, kmsKeyId, credentialsProviderOverride, cannedAcl, exclusiveWriteSupported); |
There was a problem hiding this comment.
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.
4 participants
SSE configuration is not applied correctly if we have a S3SecurityMapping which configures kmsKeyId for a given user and the default S3 Config has
s3.sse.typeset to NONE or S3.Description
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: