[FLINK-39113][s3] Fix s3.sse.kms.encryption-context config in native s3 connector#28070
[FLINK-39113][s3] Fix s3.sse.kms.encryption-context config in native s3 connector#28070gaborgsomogyi wants to merge 5 commits intoapache:masterfrom
Conversation
|
cc @Samrat002 |
d335870 to
b3f1760
Compare
b3f1760 to
8e16a10
Compare
Samrat002
left a comment
There was a problem hiding this comment.
NativeS3OutputStream.uploadToS3() is a separate write path for small files. It builds a PutObjectRequest but currently doesn't call ssekmsEncryptionContext(). The context wired through NativeS3ObjectOperations.applyEncryption() won't apply to writes that go through NativeS3OutputStream. Could you extend that code path as well?
69ec88c to
cdd8670
Compare
Samrat002
left a comment
There was a problem hiding this comment.
Left few comments.
Cheers,
Samrat
| } | ||
|
|
||
| @VisibleForTesting | ||
| @Nullable |
There was a problem hiding this comment.
| @Nullable |
Keep the field non-nullable. It must fail fast.
There was a problem hiding this comment.
This alone is not enough because there are nullable marker scattered around and we should make that consistent... We need to check if we remove all then what will happen
| * Creates a config for SSE-KMS encryption with the default KMS key and an encryption context. | ||
| * | ||
| * @param encryptionContext The encryption context key-value pairs | ||
| */ | ||
| public static S3EncryptionConfig sseKms(Map<String, String> encryptionContext) { | ||
| return new S3EncryptionConfig(EncryptionType.SSE_KMS, null, encryptionContext); | ||
| } |
There was a problem hiding this comment.
| * Creates a config for SSE-KMS encryption with the default KMS key and an encryption context. | |
| * | |
| * @param encryptionContext The encryption context key-value pairs | |
| */ | |
| public static S3EncryptionConfig sseKms(Map<String, String> encryptionContext) { | |
| return new S3EncryptionConfig(EncryptionType.SSE_KMS, null, encryptionContext); | |
| } |
| return kmsKeyId != null && !kmsKeyId.isEmpty() | ||
| ? sseKms(kmsKeyId, encryptionContext) | ||
| : sseKms(encryptionContext); |
There was a problem hiding this comment.
| return kmsKeyId != null && !kmsKeyId.isEmpty() | |
| ? sseKms(kmsKeyId, encryptionContext) | |
| : sseKms(encryptionContext); | |
| return sseKms(kmsKeyId, encryptionContext); |
There was a problem hiding this comment.
This can be abstracted into a single method. There is no requirement for an overloaded method for sseKms
public static S3EncryptionConfig sseKms(
String kmsKeyId, Map<String, String> encryptionContext) {
return new S3EncryptionConfig(EncryptionType.SSE_KMS, kmsKeyId, encryptionContext);
if (kmsKeyId != null && !kmsKeyId.isEmpty()) {
return new S3EncryptionConfig(EncryptionType.SSE_KMS, kmsKeyId, encryptionContext);
} else {
return new S3EncryptionConfig(EncryptionType.SSE_KMS, null, encryptionContext);
}
| @MethodSource | ||
| void sseKms_contextOnlyFactory_absentContext_hasEncryptionContextFalse( | ||
| Map<String, String> context) { | ||
| S3EncryptionConfig c = S3EncryptionConfig.sseKms(context); |
There was a problem hiding this comment.
| S3EncryptionConfig c = S3EncryptionConfig.sseKms(context); | |
| S3EncryptionConfig c = S3EncryptionConfig.sseKms(null, context); |
| @Test | ||
| void sseKms_contextOnlyFactory_contextMutatedAfterCreation_contextUnchanged() { | ||
| Map<String, String> ctx = new HashMap<>(Map.of("dept", "finance")); | ||
| S3EncryptionConfig c = S3EncryptionConfig.sseKms(ctx); |
There was a problem hiding this comment.
| S3EncryptionConfig c = S3EncryptionConfig.sseKms(ctx); | |
| S3EncryptionConfig c = S3EncryptionConfig.sseKms(null, ctx); |
| @MethodSource | ||
| void serializeEncryptionContext_jsonSpecialChars_escapedCorrectly( | ||
| String key, String value, String expectedFragment) { | ||
| S3EncryptionConfig c = S3EncryptionConfig.sseKms(Map.of(key, value)); |
There was a problem hiding this comment.
| S3EncryptionConfig c = S3EncryptionConfig.sseKms(Map.of(key, value)); | |
| S3EncryptionConfig c = S3EncryptionConfig.sseKms(null, Map.of(key, value)); |
| private static S3Exception s3ExceptionStatusOnly(int statusCode) { | ||
| S3Exception.Builder b = S3Exception.builder(); | ||
| b.statusCode(statusCode); | ||
| return (S3Exception) b.build(); | ||
| } |
There was a problem hiding this comment.
| private static S3Exception s3ExceptionStatusOnly(int statusCode) { | |
| S3Exception.Builder b = S3Exception.builder(); | |
| b.statusCode(statusCode); | |
| return (S3Exception) b.build(); | |
| } | |
| private static AwsServiceException s3ExceptionStatusOnly(int statusCode) { | |
| return S3Exception.builder().statusCode(statusCode).build(); | |
| } |
| private static S3Exception s3ExceptionWithMessageAndDetails( | ||
| int statusCode, String message, AwsErrorDetails details) { | ||
| S3Exception.Builder b = S3Exception.builder(); | ||
| b.statusCode(statusCode); | ||
| b.message(message); | ||
| b.awsErrorDetails(details); | ||
| return (S3Exception) b.build(); | ||
| } |
There was a problem hiding this comment.
| private static S3Exception s3ExceptionWithMessageAndDetails( | |
| int statusCode, String message, AwsErrorDetails details) { | |
| S3Exception.Builder b = S3Exception.builder(); | |
| b.statusCode(statusCode); | |
| b.message(message); | |
| b.awsErrorDetails(details); | |
| return (S3Exception) b.build(); | |
| } | |
| private static AwsServiceException s3ExceptionWithMessageAndDetails( | |
| int statusCode, String message, AwsErrorDetails details) { | |
| return S3Exception.builder() | |
| .statusCode(statusCode) | |
| .message(message) | |
| .awsErrorDetails(details) | |
| .build(); | |
| } |
| private static S3Exception s3Exception(int statusCode, String message) { | ||
| S3Exception.Builder b = S3Exception.builder(); | ||
| b.statusCode(statusCode); | ||
| b.message(message); | ||
| return (S3Exception) b.build(); | ||
| } |
There was a problem hiding this comment.
| private static S3Exception s3Exception(int statusCode, String message) { | |
| S3Exception.Builder b = S3Exception.builder(); | |
| b.statusCode(statusCode); | |
| b.message(message); | |
| return (S3Exception) b.build(); | |
| } | |
| private static AwsServiceException s3Exception(int statusCode, String message) { | |
| return S3Exception.builder().statusCode(statusCode).message(message).build(); | |
| } |
| private static S3Exception s3Exception(int statusCode, AwsErrorDetails details) { | ||
| S3Exception.Builder b = S3Exception.builder(); | ||
| b.statusCode(statusCode); | ||
| b.awsErrorDetails(details); | ||
| return (S3Exception) b.build(); | ||
| } |
There was a problem hiding this comment.
| private static S3Exception s3Exception(int statusCode, AwsErrorDetails details) { | |
| S3Exception.Builder b = S3Exception.builder(); | |
| b.statusCode(statusCode); | |
| b.awsErrorDetails(details); | |
| return (S3Exception) b.build(); | |
| } | |
| private static AwsServiceException s3Exception(int statusCode, AwsErrorDetails details) { | |
| return S3Exception.builder().statusCode(statusCode).awsErrorDetails(details).build(); | |
| } |
…ryption context - Switch fromConfig() normalization to toLowerCase(Locale.ROOT), removing the SSE_KMS placeholder case - Add sseKms(Map) factory overload so encryption context is preserved when using the default AWS-managed key - Revert Map.copyOf() to null-tolerant unmodifiableMap(new HashMap<>())
…eam write path Move serializeEncryptionContext to S3EncryptionConfig and apply it in NativeS3OutputStream.uploadToS3(), which previously dropped the KMS encryption context for small-file (non-multipart) writes.
What is the purpose of the change
s3.sse.kms.encryption-contextwas listed in the README as a supported configuration option for SSE-KMS, but the corresponding ConfigOption never existed in NativeS3FileSystemFactory. As a result, the encryption context was silently ignored regardless of what users configured, and thesseKms(keyId, context)code path inS3EncryptionConfigwas dead code unreachable from production.This PR closes the gap by adding the missing config option and wiring it end-to-end.
Brief change log
SSE_KMS_ENCRYPTION_CONTEXT(s3.sse.kms.encryption-context) ConfigOption toNativeS3FileSystemFactoryand wire it throughS3EncryptionConfig.fromConfig()— the feature was documented but never implementedS3EncryptionConfig,S3ExceptionUtils,S3FileStatus,S3BlockLocationVerifying this change
Existing and new unit tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude code