dimas-b commented on code in PR #2815:
URL: https://github.com/apache/polaris/pull/2815#discussion_r2433274475
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -46,7 +46,11 @@ public static ImmutableAwsStorageConfigurationInfo.Builder
builder() {
// Technically, it should be
^arn:(aws|aws-cn|aws-us-gov):iam::(\d{12}):role/.+$,
@JsonIgnore
- public static final String ROLE_ARN_PATTERN =
"^arn:(aws|aws-us-gov):iam::(\\d{12}):role/.+$";
+ // Account id may be a 12-digit AWS account number or a vendor-specific
namespace that must
+ // not be purely numeric (must start with a letter, underscore or hyphen
followed by allowed
+ // chars).
+ public static final String ROLE_ARN_PATTERN =
+
"^(arn|urn):(aws|aws-us-gov|ecs):iam::((\\d{12})|([a-zA-Z_-][a-zA-Z0-9_-]*)):role/.+$";
Review Comment:
Does the `([a-zA-Z_-][a-zA-Z0-9_-]*)` part apply only to ECS?
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -179,14 +179,24 @@ private IamPolicy policyString(
Map<String, IamStatement.Builder> bucketGetLocationStatementBuilder = new
HashMap<>();
String arnPrefix = arnPrefixForPartition(awsPartition);
+ boolean isEcsPartition = "ecs".equals(awsPartition);
Stream.concat(readLocations.stream(), writeLocations.stream())
.distinct()
.forEach(
location -> {
URI uri = URI.create(location);
- allowGetObjectStatementBuilder.addResource(
- IamResource.create(
- arnPrefix +
StorageUtil.concatFilePrefixes(parseS3Path(uri), "*", "/")));
+ // Some on-prem S3/STSc implementations (for example ECS) do not
accept object ARNs
+ // that include the path portion (bucket/key/*). For those,
scope object permissions
+ // to
+ // the whole bucket (bucket/*) and rely on s3:prefix conditions
for finer granularity.
+ if (isEcsPartition) {
Review Comment:
I'd prefer to avoid if/else statements by using a more OO design. This feels
like ECS is not quite the same as AWS. Supporting ECS probably requires a
sub-type property in the config and a different storage integration sub-class
(with proper refactoring of shared code into a common class).
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsStorageConfigurationInfo.java:
##########
@@ -122,7 +126,8 @@ public String getAwsAccountId() {
if (arn != null) {
Matcher matcher = ROLE_ARN_PATTERN_COMPILED.matcher(arn);
checkState(matcher.matches());
- return matcher.group(2);
+ // group(3) is the account identifier (either 12-digit AWS account or
vendor namespace)
Review Comment:
The `getAwsAccountId` method name becomes misleading now that the value may
not be an AWS accound ID. Please rename.
--
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]
