Support Dell ECS ObjectScale IAM for AssumeRole #2815
Conversation
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @rohangoli ! It looks like the current form of this PR might work, but I'm hesitant to approve it as the code does not look elegant. Would you be open to refactoring it?
| // 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/.+$"; |
There was a problem hiding this comment.
Does the ([a-zA-Z_-][a-zA-Z0-9_-]*) part apply only to ECS?
There was a problem hiding this comment.
ECS is accepting "May contain lowercase a to z, 0 to 9, dash (-), and underscore (_). Max 128 characters." this part as 12-digit AWS Account Number equivalent
There was a problem hiding this comment.
What I was trying to say is that expanding this RegEx to cover all vendors does not seem maintainable. As I commented in another thread, I wonder if we could refactor the code to have ECS-specific login in separate classes 🤔
| 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) |
There was a problem hiding this comment.
The getAwsAccountId method name becomes misleading now that the value may not be an AWS accound ID. Please rename.
| // 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) { |
There was a problem hiding this 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).
There was a problem hiding this comment.
This may be worth discussing on the dev ML.
|
How can this be tested? |
This can be tested via installing or deploying Dell ECS / OBS https://github.com/EMCECS/ECS-CommunityEdition/tree/4.1.0.0 |
|
@rohangoli : Thanks for the pointer to the ECS CommunityEdition! I had a quick look at their README, but it does not seem that there is a docker image for the Community Edition. Install guides appear to refer to building from source (which would be cumbersome for CI). WDYT? |
snazy
left a comment
snazy
left a comment
There was a problem hiding this comment.
Wondering if it's better to split this class into an abstract class AbstractS3CredentialsStorageIntegration with DefaultS3CredentialsStorageIntegration, AwsCredentialsStorageIntegration, EcsCredentialsStorageIntegration implementations. Or at least move the ECS specific parts into a new class EcsCredentialsStorageIntegration extends AwsCredentialsStorageIntegration?
|
I wonder whether it might be worth adding a |
Hm, looking at the requirements of this, it seems it's not usable in GitHub CI. The CPU, RAM and disk requirements largely exceed the Standard GitHub-hosted runner specs. |
3 participants
What changes were proposed in this pull request?
Enable support for AssumeRole ARN pattern validation (including URN/ecs role ARNs) and produce valid IAM policy resource JSON for Dell ECS or ObjectScale On-prem S3-compatible IAM.
Empty strings and the aws-cn partition are rejected with explicit error messages. Invalid formats produce a clear "Invalid role ARN format: ..." exception message, which matches test expectations.
Breakdown of Issue - Support Dell ECS On-Premise S3 Object Store with STS support #2743
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?
CHANGELOG.md