dimas-b commented on code in PR #3493:
URL: https://github.com/apache/polaris/pull/3493#discussion_r2713013559
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -299,9 +299,16 @@ private static void addKmsKeyPolicy(
String region,
String accountId) {
- IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
boolean hasCurrentKey = kmsKeyArn != null;
boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
+ boolean isAwsS3 = region != null && accountId != null;
Review Comment:
For follow-up:
This logic for deciding "AWS or not" does not look robust to me.
I'd prefer to make this a property in the Storage Config (similar to
`stsUnavailable`). We could choose the default value for it as `true` if no
custom `endpoint` or `stsEndpoint` values are set (`false` if custom endpoints
are set).
This should be fairly backward compatible... WDYT?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -299,9 +299,16 @@ private static void addKmsKeyPolicy(
String region,
String accountId) {
- IamStatement.Builder allowKms = buildBaseKmsStatement(canWrite);
boolean hasCurrentKey = kmsKeyArn != null;
boolean hasAllowedKeys = hasAllowedKmsKeys(allowedKmsKeys);
+ boolean isAwsS3 = region != null && accountId != null;
Review Comment:
The change in this PR appears to be compatible to old KMS logic, so I think
it's ok to merge "as is" and improve config later.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
