HDDS-9388. OM Ratis Write: Move ACL check and Bucket resolution to preExecute by duongkame · Pull Request #5694 · apache/ozone

duongkame · 2023-11-28T18:48:40Z

What changes were proposed in this pull request?

ACL checks and bucket soft-link resolution should be moved to preExecute. Today, these checks are done in validateAndUpdateCache and performed in each replica. Those should be done when the leader node receives the operation. Checks should be validated upfront, not when applying the state.

What is the link to the Apache JIRA

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

How was this patch tested?

CI.

adoroszlai · 2023-12-04T21:19:18Z

Thanks @duongkame for the patch.

Failure in TestOzoneAtRestEncryption.mpuWithLink and TestOzoneFileSystemWithFSO.testFSDeleteLogWarnNoExist seem related, can be reproduced locally.

adoroszlai

There are calls to checkKeyAcls from some validateAndUpdateCache methods, so far I've found:

OMKeyRenameRequestWithFSO
S3MultipartUploadCompleteRequest
OMRecoverLeaseRequest

The first two also resolve bucket links.

Are these intentional?

duongkame · 2023-12-05T01:55:38Z

There are calls to checkKeyAcls from some validateAndUpdateCache methods, so far I've found:

OMKeyRenameRequestWithFSO

S3MultipartUploadCompleteRequest

OMRecoverLeaseRequest

The first two also resolve bucket links.

Are these intentional?

They're not intentional. I was still cleaning up a few places. Thanks for the detailed look, @adoroszlai .

duongkame · 2023-12-05T06:50:46Z

There's an open point from this PR. Some S3 APIs like MultipartInfoInitiate or MultipartUploadComplete return unnecessary/redundant data like original volume, bucket and key names in their response. With the refactor to move bucket resolution to preExecute, we don't have the original bucket/volume name in applyTransaction to include in the response.
Anyway, those are redundant data and are not used in S3G (only used in tests), I decided to just mark the data @deprecated for compatibility, and removed all the relevant usage in tests.

sumitagrawl

@duongkame Thanks for working over this, this changes may have issue,
in pre-execute, it can not validate the relational things which may be part of previous request as,

link bucket validation as link bucket may not be created and pending to be excuted
ACL validation as acl change may be pending to be executed

Only static validation related to request can be done, Or some thing from system perspective which normally do not depend on previous possible request

kerneltime · 2023-12-11T22:01:05Z

@duongkame Thanks for working over this, this changes may have issue, in pre-execute, it can not validate the relational things which may be part of previous request as,

link bucket validation as link bucket may not be created and pending to be excuted

ACL validation as acl change may be pending to be executed

Only static validation related to request can be done, Or some thing from system perspective which normally do not depend on previous possible request

Concurrent requests can be interleaved in any order. I am not sure this is an issue. Do you have a specific issue in mind?

errose28 · 2023-12-12T00:07:35Z

@sumitagrawl I think you are also describing what I wrote on the Jira here. IMO this is more of a minor change in behavior than an issue. The server is still failing the request based on an ordering either way.

sumitagrawl · 2023-12-12T08:15:24Z

@sumitagrawl I think you are also describing what I wrote on the Jira here. IMO this is more of a minor change in behavior than an issue. The server is still failing the request based on an ordering either way.

@errose28
yes, 2 cases we are changing,

ACL as already mentioned
linked bucket resolution also

duongkame · 2023-12-15T06:35:42Z

@sumitagrawl I think you are also describing what I wrote on the Jira here. IMO this is more of a minor change in behavior than an issue. The server is still failing the request based on an ordering either way.

@errose28 yes, 2 cases we are changing,

ACL as already mentioned

linked bucket resolution also

@sumitagrawl
I don't see problems with Link resolution in preExecute. E.g. at the time a key-create request reaches the leader node, the request should be rejected if the link is not there. This is different from before when the link creation can happen after a while before the key-creation request reaches each replica. A reverse example is when a link is being removed. Either case, making the decision at the leader makes sense to me.

Yes, there are changes in behaviors the edge cases but they're not necessarily issues.

The same story for moving ACL checks, as @errose28 mentioned.

I think moving validation/pre-processing out of applyTransaction is the right direction. ApplyTransaction should be to apply the state of a request to the state machine.

C.c. some more folks as I need your opinions too. @kerneltime @szetszwo @smengcl.

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

...ager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequestWithFSO.java

smengcl

Core change lgtm. Thanks @duongkame for this

…MRequest.

…ne/om/request/key/OMKeyRequest.java Co-authored-by: Siyao Meng <50227127+smengcl@users.noreply.github.com>

smengcl · 2024-01-04T06:52:18Z

Thanks @duongkame for the patch, and @adoroszlai @sumitagrawl @kerneltime @errose28 for the reviews and comments.

duongkame · 2024-01-04T07:03:05Z

Thanks @smengcl for the reviews and merging it.

smengcl · 2024-02-13T23:52:32Z

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

    generateRequiredEncryptionInfo(keyArgs, newKeyArgs, ozoneManager);
    CreateFileRequest.Builder newCreateFileRequest =
-        createFileRequest.toBuilder().setKeyArgs(newKeyArgs)
+        createFileRequest.toBuilder().setKeyArgs(resolvedArgs)


Turns out this causes a regression where encrypted buckets won't correctly have file encryption info in keyargs set. Because encryption info is generated in generateRequiredEncryptionInfo above and put into newKeyArgs.

Credit to @jojochuang for locating this bug.

…lution to preExecute (apache#5694) Change-Id: Ib4b9f7ec9c0320342485f20405197a1b05e61333 Conflicts: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneClientMultipartUploadWithFSO.java hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientAbstract.java hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClientWithRatis.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMAllocateBlockRequest.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequestWithFSO.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequestWithFSO.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequestWithFSO.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequestWithFSO.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequest.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequestWithFSO.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadAbortRequest.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCommitPartRequest.java hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequestWithFSO.java hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequest.java hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequestWithFSO.java hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequestWithFSO.java hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/TestCleanupTableInfo.java Change-Id: I807c3d08ff63a6ec21023f4dd65ad4801e27e6e7

duongkame marked this pull request as ready for review November 28, 2023 23:14

duongkame marked this pull request as draft November 29, 2023 01:58

duongkame marked this pull request as ready for review December 1, 2023 20:25

kerneltime requested review from adoroszlai, kerneltime and szetszwo December 4, 2023 17:06

adoroszlai reviewed Dec 4, 2023

View reviewed changes

duongkame force-pushed the HDDS-9388 branch from 60efa41 to 9b642fc Compare December 8, 2023 18:24

sumitagrawl reviewed Dec 11, 2023

View reviewed changes

duongkame force-pushed the HDDS-9388 branch from 9b642fc to 95a7a03 Compare December 15, 2023 18:05

duongkame requested a review from smengcl December 15, 2023 19:06

smengcl reviewed Jan 3, 2024

View reviewed changes

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

smengcl reviewed Jan 3, 2024

View reviewed changes

...ager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequestWithFSO.java Show resolved Hide resolved

smengcl approved these changes Jan 3, 2024

View reviewed changes

duongkame added 9 commits January 3, 2024 10:10

Move bucket resolution and ACLs check to preExecute

7f5c209

Checkstyle.

9b4f698

Fix AuthenticationException because the UserInfo is not attached to O…

7a277a8

…MRequest.

Apply on key requests.

8332d5b

Apply on s3 requests + style check.

c28fa86

stylecheck.

62134d0

stylecheck.

240de09

Fix failing tests.

11841ed

Remove dedundant check.

440f1de

duongkame and others added 8 commits January 3, 2024 10:25

Fix failing tests.

1b3e86f

Fix failing tests.

032bb7a

Fix failing tests and missing ACLs check moving.

81b4138

checkstyle

6204b73

Fix failing UTs.

88f3720

Update hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozo…

3f8e25f

…ne/om/request/key/OMKeyRequest.java Co-authored-by: Siyao Meng <50227127+smengcl@users.noreply.github.com>

Rename.

88d5f2a

Fix TestOMKeyDeleteRequest after rebasing.

52a94bc

duongkame force-pushed the HDDS-9388 branch from 65d8cd1 to 52a94bc Compare January 3, 2024 19:29

Fix TestOMKeyDeleteRequest after rebasing.

870d601

smengcl merged commit cca6782 into apache:master Jan 4, 2024

smengcl reviewed Feb 13, 2024

View reviewed changes

smengcl mentioned this pull request Feb 23, 2024

HDDS-10363. HDDS-9388 broke encryption. #6219

Merged

duongkame deleted the HDDS-9388 branch April 12, 2025 00:10

Conversation

duongkame commented Nov 28, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai commented Dec 4, 2023

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

duongkame commented Dec 5, 2023

Uh oh!

duongkame commented Dec 5, 2023

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

kerneltime commented Dec 11, 2023

Uh oh!

errose28 commented Dec 12, 2023

Uh oh!

sumitagrawl commented Dec 12, 2023

Uh oh!

duongkame commented Dec 15, 2023

Uh oh!

Uh oh!

Uh oh!

smengcl left a comment

Choose a reason for hiding this comment

Uh oh!

smengcl commented Jan 4, 2024

Uh oh!

duongkame commented Jan 4, 2024

Uh oh!

smengcl Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

smengcl Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments