HDDS-4585. Support bucket acl operation in S3g

[ChenSammi]

https://issues.apache.org/jira/browse/HDDS-4585

Design document:
https://issues.apache.org/jira/browse/HDDS-4550

[ChenSammi]

AWS CLI console output with the patch,

Get ACL
[root@]# aws s3api --profile root --endpoint-url http://host:9878 get-bucket-acl --bucket ozone
{
"Owner": {
"DisplayName": "root",
"ID": "root"
},
"Grants": [
{
"Grantee": {
"Type": "CanonicalUser",
"DisplayName": "apple",
"ID": "apple"
},
"Permission": "FULL_CONTROL"
}
]
}

Put ACL
Form1
[root@]#aws s3api --profile root --endpoint-url http://host:9878 put-bucket-acl --bucket=ozone --grant-read id=one,id=two,id=three --grant-full-control id=tom,id=jerry

Form2
[root@]#aws s3api --profile root --endpoint-url http://host:9878 put-bucket-acl --bucket=ozone --access-control-policy '{"Grants": [ {"Grantee": {"DisplayName": "root", "ID": "root", "Type": "CanonicalUser"},"Permission": "FULL_CONTROL"} ], "Owner": {"DisplayName": "root","ID": "root"}}'

[xiaoyuyao]

LGTM overall, a few comments inline.

[xiaoyuyao]

Some additional comments.

[ChenSammi]

misc acceptance exit with error code 1, while there is no obvious error. Can anyone tell me how to do a further investigation.

[adoroszlai]

misc acceptance exit with error code 1, while there is no obvious error.

I'm looking into it: HDDS-4760.

[ChenSammi]

misc acceptance exit with error code 1, while there is no obvious error.

I'm looking into it: HDDS-4760.

@adoroszlai ，thanks for the info.

[prashantpogde]

Do we need to setup ACL on a volume here ? couldn't this lead to manipulating permissions on a bucket where the user doesn't have a permission ?

[ChenSammi]

Yes, we need to setup ACL on a volume for S3 bucket. Here we grant the user least privilege on volume in order to make sure user can access the bucket. If a user has full control of the bucket while he doesn't have any permission on volume, user will fail to execute some operations, such as bucket list.

The thing is object hierarchy of Ozone is /volume/bucket/key. ACL on Ozone has the hierarchical characteristic. ACL of parent will impact the accessibility of it's child.

[xiaoyuyao]

Should we read the existing volume acl, add the new ones necessary and set it back instead of overwrite the existing ACL on the s3 volume?

@xiaoyuyao , I updated the implementation. Now it will remove all the ACLs of import users from Volume first, then apply the new ACLs on Volume.

[ChenSammi]

@xiaoyuyao @prashantpogde , I just rebased the patch to the master branch, do you have time to take another look?

The remaining checkstyle warnings are mainly XML content string used for test which I don't plan to fix.
The failed secure acceptance test seems irrelevant.

[adoroszlai]

The remaining checkstyle warnings are mainly XML content string used for test which I don't plan to fix.

These should be trivial to fix:

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Acl.java
 38: Utility classes should not have a public or default constructor.
 42: Name 'grantRead' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'.
 43: Name 'grantWrite' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'.
 44: Name 'grantReadACP' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'.
 45: Name 'grantWriteACP' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'.
 46: Name 'grantFullControl' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'.
 49: Name 'cannedAclHeader' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'.
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java
 53: Name 'bucketName' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'.
 60: Name 'aclMarker' must match pattern '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'.

and these can be fixed by moving XML content to external file(s) under hadoop-ozone/s3gateway/src/test/resources:

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java
 224: Line is longer than 80 characters (found 90).
 226: Line is longer than 80 characters (found 100).
 231: Line is longer than 80 characters (found 120).
 232: Line is longer than 80 characters (found 104).
 238: Line is longer than 80 characters (found 112).
 239: Line is longer than 80 characters (found 99).
 244: Line is longer than 80 characters (found 112).
 245: Line is longer than 80 characters (found 98).
 250: Line is longer than 80 characters (found 128).
 251: Line is longer than 80 characters (found 84).
 256: Line is longer than 80 characters (found 120).
 257: Line is longer than 80 characters (found 115).
 272: Line is longer than 80 characters (found 90).
 274: Line is longer than 80 characters (found 100).
 279: Line is longer than 80 characters (found 120).
 280: Line is longer than 80 characters (found 104).
 286: Line is longer than 80 characters (found 120).
 287: Line is longer than 80 characters (found 115).

[ChenSammi]

Hey，@xiaoyuyao do you have time to take another look？

[xiaoyuyao]

Thanks @ChenSammi for the update. I have a few additional question added.

[xiaoyuyao]

Should we read the existing volume acl, add the new ones necessary and set it back instead of overwrite the existing ACL on the s3 volume?

@xiaoyuyao , I updated the implementation. Now it will remove all the ACLs of import users from Volume first, then apply the new ACLs on Volume.

[xiaoyuyao]

Thanks @ChenSammi for the update. The latest change LGTM. +1. I will merge the PR if no further comments by EOD today.

[ChenSammi]

Thanks @xiaoyuyao for the code review.