Repository navigation
Conversation
FrankChen021
left a comment
Member
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in this review. The multipart encryption decorators are correctly wired through the existing S3 wrapper; KMS and SSE-S3 apply at upload initiation, while SSE-C applies at initiation and to every part.
Reviewed 5 of 5 changed files.
Validation: git diff --check passed.
This is an automated review by Codex GPT-5.6-Luna(max)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
MSQ durable storage and S3 exports write files with S3 multipart uploads, through
RetryableS3OutputStream.KmsServerSideEncryptionandS3ServerSideEncryptiononly added encryption settings to singleuploads and copies, not to the request that starts a multipart upload. Large durable-storage and export files therefore got the bucket's default encryption instead of the configured
druid.storage.sse.*.With
kms, this meantkeyIdwas never sent. Only files small enough for a single request were encrypted with the configured settings.This PR adds the missing settings:
kms: addsaws:kmsandssekmsKeyIdto the request that starts a multipart upload.s3: addsAES256to the request that starts a multipart upload.custom: adds the customer key to both the start request and each part upload, because S3 needs the key on every part.Release note
S3 server-side encryption (
druid.storage.sse.type) now applies to multipart uploads in MSQ durable storage and S3 exports. Previously, large files fell back to the bucket's default encryption.Upgrade notes:
kmsneed no special upgrade order. Reading a KMS-encrypted object needs no extra request headers, so mixed versions and rollbacks work.kms:GenerateDataKeyandkms:Decryptto the roles that write these files, such as MSQ tasks. AWS requireskms:Decryptfor KMS-encryptedmultipart uploads. Grant
kms:Decryptto everything that reads them, including Brokers that read MSQ query results and any systems outside Druid that read exported files.custom, large durable-storage objects that older tasks write during the upgrade may still fail to read, as they already do today. The failures stop once all services are upgraded.Key changed/added classes in this PR
KmsServerSideEncryptionS3ServerSideEncryptionCustomServerSideEncryptionServerSideEncryptionTestThis PR has: