HDDS-8653. Let directory inherit parent default ACLs #4738

whbing · 2023-05-19T05:28:34Z

What changes were proposed in this pull request?

Let directory inherit bucket default ACLs

What is the link to the Apache JIRA

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

Please replace this section with the link to the Apache JIRA)

How was this patch tested?

Tested CLI commands in ozone compose environment.

# use hadoop admin user to execute
schema=hadoop1
vol=vol1

# CASE 1: TEST FSO
buk=bucket-fso
ozone sh bucket create ${vol}/${buk} --layout FILE_SYSTEM_OPTIMIZED
ozone sh bucket addacl -a=user:user1:rwl[DEFAULT] ${vol}/${buk}
# missing dir case
ozone fs -mkdir -p ofs://${schema}/${vol}/${buk}/d1/d2
ozone sh key getacl ${vol}/${buk}/d1
[ {
  "type" : "USER",
  "name" : "user1",
  "aclScope" : "DEFAULT",
  "aclList" : [ "READ", "WRITE", "LIST" ]
}, {
  "type" : "USER",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "GROUP",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
} ]
ozone sh key getacl ${vol}/${buk}/d1/d2
[ {
  "type" : "USER",
  "name" : "user1",
  "aclScope" : "DEFAULT",
  "aclList" : [ "READ", "WRITE", "LIST" ]
}, {
  "type" : "USER",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "GROUP",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
} ]
# file acl
ozone fs -put file ofs://${schema}/${vol}/${buk}/d1/d2/f1
ozone sh key getacl ${vol}/${buk}/d1/d2/f1
[ {
  "type" : "USER",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "GROUP",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "USER",
  "name" : "user1",
  "aclScope" : "ACCESS",
  "aclList" : [ "READ", "WRITE", "LIST" ]
} ]

# test dir exist
ozone fs -mkdir ofs://${schema}/${vol}/${buk}/d1/d2/d3
ozone sh key addacl -a=user:user2:rw[DEFAULT] ${vol}/${buk}/d1/d2/d3
ozone sh key getacl ${vol}/${buk}/d1/d2/d3
[ {
  "type" : "USER",
  "name" : "user1",
  "aclScope" : "DEFAULT",
  "aclList" : [ "READ", "WRITE", "LIST" ]
}, {
  "type" : "USER",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "GROUP",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "USER",
  "name" : "user2",
  "aclScope" : "DEFAULT",
  "aclList" : [ "READ", "WRITE" ]
} ]
ozone fs -put file ofs://${schema}/${vol}/${buk}/d1/d2/d3/f2
ozone sh key getacl ${vol}/${buk}/d1/d2/d3/f2
[ {
  "type" : "USER",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "GROUP",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "USER",
  "name" : "user1",
  "aclScope" : "ACCESS",
  "aclList" : [ "READ", "WRITE", "LIST" ]
}, {
  "type" : "USER",
  "name" : "user2",
  "aclScope" : "ACCESS",
  "aclList" : [ "READ", "WRITE" ]
} ]
# test ozone client
ozone sh key put ${vol}/${buk}/d1/d2/d3/f3 file
ozone sh key getacl ${vol}/${buk}/d1/d2/d3/f3
[ {
  "type" : "USER",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "GROUP",
  "name" : "hadoop",
  "aclScope" : "ACCESS",
  "aclList" : [ "ALL" ]
}, {
  "type" : "USER",
  "name" : "user1",
  "aclScope" : "ACCESS",
  "aclList" : [ "READ", "WRITE", "LIST" ]
}, {
  "type" : "USER",
  "name" : "user2",
  "aclScope" : "ACCESS",
  "aclList" : [ "READ", "WRITE" ]
} ]

# ----------

# CASE 2: TEST LEGACY
buk=bucket-lg
ozone sh bucket create ${vol}/${buk} --layout LEGACY
# same cli as above (dir endwith / ), and get the same result

adoroszlai

Thanks @whbing for reporting the problem and working on it.

TestRecursiveAclWithFSO is failing, please check.

Please also add tests to verify the new behavior.

...r/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequestWithFSO.java

...-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java

adoroszlai

Thanks @whbing for updating the patch.

hadoop-ozone/dist/src/main/smoketest/ozonefs/ozonefs.robot

...r/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequestWithFSO.java

...ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestRecursiveAclWithFSO.java

whbing · 2023-05-21T08:31:11Z

I started a disscuss in #4752. Will update commit after disscussing.

whbing · 2023-06-01T07:32:10Z

Currently implemented logic:
all node(dir/file) has its owner acls as well as inherited acls.

The logic of acl inheritance is as follows

FSO: subdir or leaf file inherit direct parent's DEFAULT acl, subdir keeps DEFAULT scope and file keeps ACCESS scope
LEGACY: subdir (endwith /) inherit direct parent's DEFAULT acl and keeps DEFAULT scope, leaf file inherit bucket DEFAULT acls (because can't get the parent info temporarily, maybe can optimize it in subsequent PR)
OBS: no dir, inherit from bucket DEFAULT acls.

The benefits of the above changes are:

Unified inheritance rules for subdirs and files. For example, READ_GROUP[DEFAULT] acl is given to a bucket, which can be inherited to children for easy authentication.
LEGACY and OBS also try to be consistent with FSO to reduce the complexity of understanding.
It is good for recursion acl-checking (maybe there will be a new PR combined with prefix in the future for more optimized authentication).

whbing · 2023-06-05T09:36:31Z

ci passed in : https://github.com/whbing/ozone/actions/runs/5172606799
The results tested CLI commands in my compose environment are at the top of this page.
Thanks for taking the time to review ! 😁

adoroszlai · 2023-06-05T17:15:52Z

@ChenSammi @fapifta please review

sumitagrawl

@whbing Thanks for working for this, having few comments.

IMO,

getting inheritDefaultAcl can be done at place where creating Directory, instead of passing filtered defaultAcl, similar to OmKeyRequest.getAclsForKey used in FileInfo create
This flow is for creating missing parent directory.

sumitagrawl · 2023-06-06T11:52:04Z

...ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileRequest.java

filterDefaultScope is not done for omBucketInfo.getAcls() present at line no 197, so if no parent dir, only bucket, then will take all acls from bucket

IMO, instead of changing PathInfo, it can filter Default at time of Directory / file creation. FileInfo creation, its handled. But directory creation for all flow, this needs handle.

sumitagrawl · 2023-06-06T12:02:20Z

...p-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java

method itself will inherit only default acl, it seems no need set again toDefaultScope() and this method may not be useful

method itself will inherit only default acl, it seems no need set again toDefaultScope() and this method may not be useful

@sumitagrawl The current convention is ACLs which the directory inherits necessary to keep DEFAULT scope, except leaf file. I added toDefaultScope without changing the basis inheritDefaultAcls method. How do you think about it? Thanks.

...n/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequest.java

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OzoneAclUtil.java

whbing · 2023-06-06T13:58:57Z

@sumitagrawl Thanks for review ! I will try to make some optimizations based on your comments, and then reply to you.

whbing · 2023-06-16T16:52:31Z

ci passed in： https://github.com/whbing/ozone/actions/runs/5291548464
@sumitagrawl Code updated as suggested. PTAL If you have the time, thanks !

whbing · 2023-06-16T17:04:21Z

Class organized as follows. (OMFileRequest can be considered as a tool class). All create related classes are adapted.

sumitagrawl

@whbing Thanks for fixing the previous comments, few new comments are there, please handle.

...e/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java

...n/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequest.java

...org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequestWithFSO.java

ChenSammi · 2023-07-12T14:18:30Z

Currently implemented logic: all node(dir/file) has its owner acls as well as inherited acls.

The logic of acl inheritance is as follows

1. `FSO`: subdir or leaf file inherit direct parent's DEFAULT acl, subdir keeps DEFAULT scope and file keeps ACCESS scope

2. `LEGACY`: subdir (endwith /) inherit direct parent's DEFAULT acl and keeps DEFAULT scope, leaf file inherit bucket DEFAULT acls (because can't get the parent info temporarily, maybe can optimize it in subsequent PR)

3. `OBS`: no dir, inherit from bucket DEFAULT acls.

@whbing , it's good to have a summary here. I have one question about "LEGACY" bucket. Say there are two subdirs under bucket1, one is dir1/, another is dir1/dir2/. Bucket has one DEFAULT ACL1, dir1/ has one DEFAULT ACL2(prefix ACL), will dir1/dir2/ inherits both ACL1 and ACL2?

whbing · 2023-07-12T14:46:43Z

Currently implemented logic: all node(dir/file) has its owner acls as well as inherited acls.
The logic of acl inheritance is as follows
1. `FSO`: subdir or leaf file inherit direct parent's DEFAULT acl, subdir keeps DEFAULT scope and file keeps ACCESS scope

2. `LEGACY`: subdir (endwith /) inherit direct parent's DEFAULT acl and keeps DEFAULT scope, leaf file inherit bucket DEFAULT acls (because can't get the parent info temporarily, maybe can optimize it in subsequent PR)

3. `OBS`: no dir, inherit from bucket DEFAULT acls.
@whbing , it's good to have a summary here. I have one question about "LEGACY" bucket. Say there are two subdirs under bucket1, one is dir1/, another is dir1/dir2/. Bucket has one DEFAULT ACL1, dir1/ has one DEFAULT ACL2(prefix ACL), will dir1/dir2/ inherits both ACL1 and ACL2?

I think it is:

FSO or Legacy with EnableFileSystem: inherit the latest parent dir's DEFAULT acls. If latest parent dir has no DEFAULT acls, just inherit bucket acls and the ancestor is not traced.
OBS or Legacy with DisableFileSystem: inherit the the bucket DEFAULT acls.

In normal cases, inheritance starts with the DEFAULT of the bucket. If the middle dir is changed, the sub-node should only follow the behavior of the latest parent dir. I guess so.

How about that logic?
The relevant codes in: OMKeyRequest#getAclsForKey and OMKeyRequest#getAclsForDir

whbing · 2023-07-12T16:34:43Z

ci passed in my branch with last commit: https://github.com/whbing/ozone/actions/runs/5532904297

sumitagrawl

@whbing Thanks for working over this, LGTM+1

whbing · 2023-07-13T16:34:23Z

I think it is:

FSO or Legacy with EnableFileSystem: inherit the latest parent dir's DEFAULT acls. If latest parent dir has no DEFAULT acls, just inherit bucket acls and the ancestor is not traced.

OBS or Legacy with DisableFileSystem: inherit the the bucket DEFAULT acls.

In normal cases, inheritance starts with the DEFAULT of the bucket. If the middle dir is changed, the sub-node should only follow the behavior of the latest parent dir. I guess so.

How about that logic? The relevant codes in: OMKeyRequest#getAclsForKey and OMKeyRequest#getAclsForDir

By the way, I can add this logic to content/security/SecurityAcls.md if no problem.

whbing · 2023-07-15T10:32:43Z

@sumitagrawl @ChenSammi The documentation was supplemented based on the previous commit. Other code logic has not changed. If you have time, PTAK, thanks !

whbing · 2023-07-22T04:10:39Z

Ci passed in branch. please help triger ci, thanks. @ChenSammi @sumitagrawl

whbing · 2023-08-02T04:08:37Z

Failed Test TestOzoneFileSystem.testListStatusOnKeyNameContainDelimiter seems unrelated and cannot be reproduced. I have run the unit tests multiple times and they are passing. Maybe we can trigger the UT again.

ChenSammi · 2023-08-16T08:19:03Z

Thank you @whbing for the contribution.

adoroszlai reviewed May 19, 2023

View reviewed changes

adoroszlai reviewed May 20, 2023

View reviewed changes

adoroszlai requested review from ChenSammi and fapifta May 20, 2023 20:36

whbing marked this pull request as draft May 29, 2023 14:00

whbing marked this pull request as ready for review June 1, 2023 07:32

sumitagrawl reviewed Jun 6, 2023

View reviewed changes

whbing marked this pull request as draft June 14, 2023 13:39

whbing force-pushed the HDDS-8653 branch from 14e832f to 65265c9 Compare June 15, 2023 13:39

whbing marked this pull request as ready for review June 15, 2023 13:40

whbing force-pushed the HDDS-8653 branch 3 times, most recently from 454248c to c276ffb Compare June 16, 2023 15:17

whbing changed the title ~~HDDS-8653. Let directory inherit bucket default ACLs~~ HDDS-8653. Let directory inherit parent default ACLs Jun 16, 2023

adoroszlai requested a review from sumitagrawl June 19, 2023 15:02

ChenSammi mentioned this pull request Jun 28, 2023

HDDS-7933. Document Prefix ACL #4938

Merged

sumitagrawl reviewed Jul 5, 2023

View reviewed changes

wanghongbing added 6 commits July 11, 2023 23:35

HDDS-8653. Let directory inherit bucket default ACLs

98e78ba

fix and add test.

02e2617

dir acls convert to DEFAULT scope

cce686f

add test

7ea7f2d

fill acl when creating dir

3f3c3d2

Add TestOMKeyCreateRequest and improve other test msg.

7d4342c

whbing force-pushed the HDDS-8653 branch from 7f4576b to 3cdc4a1 Compare July 12, 2023 05:11

Fix legacy and S3InitiateMultipartUpload logic, and add test.

4b4f680

whbing force-pushed the HDDS-8653 branch from 3cdc4a1 to 4b4f680 Compare July 12, 2023 14:32

sumitagrawl approved these changes Jul 13, 2023

View reviewed changes

Add document for inheritance rules

04972a5

Merge branch 'master' into HDDS-8653

fc7a0df

ChenSammi merged commit 4474085 into apache:master Aug 16, 2023

HDDS-8653. Let directory inherit parent default ACLs #4738

HDDS-8653. Let directory inherit parent default ACLs #4738

Uh oh!

Conversation

whbing commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

whbing commented May 21, 2023

Uh oh!

whbing commented Jun 1, 2023

Uh oh!

whbing commented Jun 5, 2023

Uh oh!

adoroszlai commented Jun 5, 2023

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

sumitagrawl Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

sumitagrawl Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

sumitagrawl Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

whbing Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

whbing commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whbing commented Jun 16, 2023

Uh oh!

whbing commented Jun 16, 2023

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChenSammi commented Jul 12, 2023

Uh oh!

whbing commented Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whbing commented Jul 12, 2023

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

whbing commented Jul 13, 2023

Uh oh!

whbing commented Jul 15, 2023

Uh oh!

whbing commented Jul 22, 2023

Uh oh!

whbing commented Aug 2, 2023

Uh oh!

whbing commented May 19, 2023 •

edited

Loading

whbing commented Jun 6, 2023 •

edited

Loading

whbing commented Jul 12, 2023 •

edited

Loading