[#6055] feat(core): extend OSS credential provider to support OSS fileset operations #6029
Conversation
| .addAction("oss:GetBucketLocation") | ||
| .addAction("oss:GetBucketInfo") |
There was a problem hiding this comment.
Please confirm that it is okay to have two actions added here, and update the comments accordingly.
There was a problem hiding this comment.
it's ok to do so, I prefer not adding extra comments because this pattern is used several time in the class, it's self explained. I add extra comment for why adding the action.
| // ListBucket | ||
| bucketListStatementBuilder.computeIfAbsent( |
There was a problem hiding this comment.
These are supposed to be revised at well.
FANNG1
changed the title
|
it's ready to review now, @tengqm @sunxiaojian @jerryshao @yuqi1129 PTAL |
| @@ -150,13 +151,13 @@ private String createPolicy(Set<String> readLocations, Set<String> writeLocation | |||
| URI uri = URI.create(location); | |||
| allowGetObjectStatementBuilder.addResource(getOssUriWithArn(arnPrefix, uri)); | |||
| String bucketArn = arnPrefix + getBucketName(uri); | |||
| // ListBucket | |||
| // OSS use 'oss:ListObjects' to list objects in a bucket while s3 use 's3:ListBucket' | |||
There was a problem hiding this comment.
Thank you. This comment is very useful for other developers to know why the action used is different.
| @@ -166,6 +167,8 @@ private String createPolicy(Set<String> readLocations, Set<String> writeLocation | |||
| Statement.builder() | |||
| .effect(Effect.ALLOW) | |||
| .addAction("oss:GetBucketLocation") | |||
| // OSS Hadoop connector need to get bucket information | |||
There was a problem hiding this comment.
I didn't mean this line. I meant the line 163.
Other developers (including me) would like to know that the GetBucketLocation action is not sufficient. In other words, this is two-actions in a row is by design, not a careless mistake.
There was a problem hiding this comment.
OH, got it, update the comment and variable name
|
LGTM |
…SS fileset operations (apache#6029) ### What changes were proposed in this pull request? 1. correct `ListBucket` to `ListObjects` 2. add `oss:GetBucketInfo` action ### Why are the changes needed? Fix: apache#6055 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? 1. run pass fileset oss test
What changes were proposed in this pull request?
ListBuckettoListObjectsoss:GetBucketInfoactionWhy are the changes needed?
Fix: #6055
Does this PR introduce any user-facing change?
no
How was this patch tested?