dimas-b commented on code in PR #1424:
URL: https://github.com/apache/polaris/pull/1424#discussion_r2277790140
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -239,4 +291,11 @@ private String getArnPrefixFor(String roleArn) {
}
return path;
}
+
+ private boolean isKMSSupported(CallContext callContext) {
+ return !callContext
+ .getRealmConfig()
+ .getConfig(KMS_SUPPORT_LEVEL_S3)
Review Comment:
With this approach to getting the config value, is `KmsSupportLevel.TABLE`
relevant?
##########
polaris-core/src/main/java/org/apache/polaris/core/config/FeatureConfiguration.java:
##########
@@ -349,4 +349,18 @@ public static void enforceFeatureEnabledOrThrow(
+ "it is still possible to enforce the uniqueness of table
locations within a catalog.")
.defaultValue(false)
.buildFeatureConfiguration();
+
+ public static final FeatureConfiguration<KmsSupportLevel>
KMS_SUPPORT_LEVEL_S3 =
+ PolarisConfiguration.<KmsSupportLevel>builder()
+ .key("ENABLE_KMS_SUPPORT_FOR_S3")
+ .catalogConfig("polaris.config.enable-kms-support-for-s3")
+ .description("If true, enables KMS support for S3 storage
integration")
+ .defaultValue(KmsSupportLevel.NONE)
Review Comment:
It would be nice to add an end-to-end test for non-default values...
Otherwise, how do we know they work if configured? :wink:
Ideally, we should test with MinIO too... It looks like MinIO has KMS.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -214,7 +223,32 @@ private IamPolicy policyString(
bucketGetLocationStatementBuilder
.values()
.forEach(statementBuilder ->
policyBuilder.addStatement(statementBuilder.build()));
- return
policyBuilder.addStatement(allowGetObjectStatementBuilder.build()).build();
+
+ policyBuilder.addStatement(allowGetObjectStatementBuilder.build());
+
+ if (isKMSSupported(callContext)) {
Review Comment:
I tend to think the KMS config rather belongs in
`AwsStorageConfigurationInfo`.
We can still keep the feature flag as a global kill switch (in case
unexpected behaviour occurs in runtime), but using KMS or not is probably
something to be configured at each specific storage integration point. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
