HDDS-6938. handle NPE when removing prefixAcl #3568

mohan3d · 2022-06-29T10:27:07Z

What changes were proposed in this pull request?

Handle the case when removing prefixAcl of a non-existing prefix (A prefix that has no acls set before, or invalid prefix). it throws NPE if no acls are there.

What is the link to the Apache JIRA

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

How was this patch tested?

unittest/manual tests.

mohan3d · 2022-06-29T10:32:27Z

Testing:

bash-4.2$ kinit -kt /etc/security/keytabs/testuser.keytab testuser/[email protected]
bash-4.2$ ozone shell volume create /vol1
bash-4.2$ ozone shell bucket create /vol1/bucket1
bash-4.2$ ozone shell key put /vol1/bucket1/prefix/key1 ./CONTRIBUTING.md
bash-4.2$ ozone shell key put /vol1/bucket1/prefix/p/key2 ./CONTRIBUTING.md

bash-4.2$ ozone shell prefix removeacl --acls=user:mohanad.elsafty:a /vol1/bucket1/prefix/
PREFIX_NOT_FOUND No prefix info for the prefix path: /vol1/bucket1/prefix/

bash-4.2$ ozone shell prefix removeacl --acls=user:mohanad.elsafty:a /vol1/bucket1/prefix/key1xyz
PREFIX_NOT_FOUND No prefix info for the prefix path: /vol1/bucket1/prefix/key1xyz

OM Log

otocol.OzoneManagerProtocol
om1_1        | 2022-06-29 10:24:02,415 [OM StateMachine ApplyTransaction Thread - 0] ERROR prefix.OMPrefixRemoveAclRequest: Remove acl [user:mohanad.elsafty:a[ACCESS]] to path /vol1/bucket1/prefix/ failed!
om1_1        | PREFIX_NOT_FOUND org.apache.hadoop.ozone.om.exceptions.OMException: No prefix info for the prefix path: /vol1/bucket1/prefix/
om1_1        | 	at org.apache.hadoop.ozone.om.request.key.acl.prefix.OMPrefixAclRequest.validateAndUpdateCache(OMPrefixAclRequest.java:98)
om1_1        | 	at org.apache.hadoop.ozone.protocolPB.OzoneManagerRequestHandler.handleWriteRequest(OzoneManagerRequestHandler.java:293)
om1_1        | 	at org.apache.hadoop.ozone.om.ratis.OzoneManagerStateMachine.runCommand(OzoneManagerStateMachine.java:529)
om1_1        | 	at org.apache.hadoop.ozone.om.ratis.OzoneManagerStateMachine.lambda$1(OzoneManagerStateMachine.java:324)
om1_1        | 	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1700)
om1_1        | 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
om1_1        | 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
om1_1        | 	at java.base/java.lang.Thread.run(Thread.java:829)
om1_1        | 2022-06-29 10:24:16,204 [Socket Reader #1 for port 9862] INFO ipc.Server: Auth successful for testuser/[email protected] (auth:KERBEROS) from 172.25.0.116:52802
om1_1        | 2022-06-29 10:24:16,219 [Socket Reader #1 for port 9862] INFO authorize.ServiceAuthorizationManager: Authorization successful for testuser/[email protected] (auth:KERBEROS) for protocol=interface org.apache.hadoop.ozone.om.protocol.OzoneManagerProtocol
om1_1        | 2022-06-29 10:24:16,628 [OM StateMachine ApplyTransaction Thread - 0] ERROR prefix.OMPrefixRemoveAclRequest: Remove acl [user:mohanad.elsafty:a[ACCESS]] to path /vol1/bucket1/prefix/key1xyz failed!
om1_1        | PREFIX_NOT_FOUND org.apache.hadoop.ozone.om.exceptions.OMException: No prefix info for the prefix path: /vol1/bucket1/prefix/key1xyz
om1_1        | 	at org.apache.hadoop.ozone.om.request.key.acl.prefix.OMPrefixAclRequest.validateAndUpdateCache(OMPrefixAclRequest.java:98)
om1_1        | 	at org.apache.hadoop.ozone.protocolPB.OzoneManagerRequestHandler.handleWriteRequest(OzoneManagerRequestHandler.java:293)
om1_1        | 	at org.apache.hadoop.ozone.om.ratis.OzoneManagerStateMachine.runCommand(OzoneManagerStateMachine.java:529)
om1_1        | 	at org.apache.hadoop.ozone.om.ratis.OzoneManagerStateMachine.lambda$1(OzoneManagerStateMachine.java:324)
om1_1        | 	at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1700)
om1_1        | 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
om1_1        | 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
om1_1        | 	at java.base/java.lang.Thread.run(Thread.java:829)

Earlier before this update, the OM will be terminated due to NullPointerException at this line

ayushtkn

Please add a unit test for the change

aswinshakil

Thanks for working on this. I have some comments inline. Can you add a Unit Test as suggested by @ayushtkn

...ager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java

adoroszlai · 2022-06-30T11:45:57Z

@mohan3d Thanks for working on the fix. Please try to avoid force-push when updating the PR. Here are some great articles that explain why:

https://developers.mattermost.com/blog/submitting-great-prs/#4-avoid-force-pushing
https://www.freecodecamp.org/news/optimize-pull-requests-for-reviewer-happiness#request-a-review

mohan3d · 2022-06-30T12:03:08Z

Thanks @adoroszlai ! will be careful next time.

adoroszlai

Thanks @mohan3d for updating the patch, LGTM.

@aswinshakil @ayushtkn would you like to take another look?

ayushtkn · 2022-07-02T20:06:09Z

...nager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMPrefixRemoveAclRequest.java

+ */
+public class TestOMPrefixRemoveAclRequest extends TestOMKeyRequest {
+
+  @Test


Do we need a new test class for this?
Can't we adjust it in some existing class? like TestOMPrefixAclRequest?

Not really, I thought it would be cleaner to have corresponding testing class for RemoveAcl. Yeah it can be done their as well.

The change is in OMPrefixAclRequest.java and if we have a corresponding test class TestOMPrefixAclRequest
better to add tests there only unless there are issues doing so.
Not very convinced with having one-one test class for each use case. Better keep all the related tests in one place only

@ayushtkn updated accordingly.

ayushtkn

LGTM

ferhui · 2022-07-05T06:20:49Z

@mohan3d Thanks for your contribution. @adoroszlai @aswinshakil @ayushtkn Thanks for your review! Merged

* master: (46 commits) HDDS-6901. Configure HDDS volume reserved as percentage of the volume space. (apache#3532) HDDS-6978. EC: Cleanup RECOVERING container on DN restarts (apache#3585) HDDS-6982. EC: Attempt to cleanup the RECOVERING container when reconstruction failed at coordinator. (apache#3583) HDDS-6968. Addendum: [Multi-Tenant] Fix USER_MISMATCH error even on correct user. (apache#3578) HDDS-6794. EC: Analyze and add putBlock even on non writing node in the case of partial single stripe. (apache#3514) HDDS-6900. Propagate TimeoutException for all SCM HA Ratis calls. (apache#3564) HDDS-6938. handle NPE when removing prefixAcl (apache#3568) HDDS-6960. EC: Implement the Over-replication Handler (apache#3572) HDDS-6979. Remove unused plexus dependency declaration (apache#3579) HDDS-6957. EC: ReplicationManager - priortise under replicated containers (apache#3574) HDDS-6723. Close Rocks objects properly in OzoneManager (apache#3400) HDDS-6942. Ozone Buckets/Objects created via S3 should not allow group access (apache#3553) HDDS-6965. Increase timeout for basic check (apache#3563) HDDS-6969. Add link to compose directory in smoketest README (apache#3567) HDDS-6970. EC: Ensure DatanodeAdminMonitor can handle EC containers during decommission (apache#3573) HDDS-6977. EC: Remove references to ContainerReplicaPendingOps in TestECContainerReplicaCount (apache#3575) HDDS-6217. Cleanup XceiverClientGrpc TODOs, and document how the client works and should be used. (apache#3012) HDDS-6773. Cleanup TestRDBTableStore (apache#3434) - fix checkstyle HDDS-6773. Cleanup TestRDBTableStore (apache#3434) HDDS-6676. KeyValueContainerData#getProtoBufMessage() should set block count (apache#3371) ... Conflicts: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/upgrade/SCMUpgradeFinalizer.java

ayushtkn reviewed Jun 29, 2022

View reviewed changes

HDDS-6938. handle NPE when removing prefixAcl of a non-set prefix

7f20757

aswinshakil reviewed Jun 30, 2022

View reviewed changes

...ager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java Outdated Show resolved Hide resolved

...ager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java Outdated Show resolved Hide resolved

mohan3d added 2 commits June 30, 2022 14:30

HDDS-6938. add removePrefixAcl unittest

9fe1a62

HDDS-6938. fix style

bd42b66

mohan3d requested a review from ayushtkn June 30, 2022 08:17

HDDS-6938. get rid of useless variable

e2e83a5

adoroszlai reviewed Jul 1, 2022

View reviewed changes

ferhui approved these changes Jul 2, 2022

View reviewed changes

ayushtkn reviewed Jul 2, 2022

View reviewed changes

HDDS-6938. move removeAcl test to TestOMPrefixAclRequest

d87be89

mohan3d requested a review from ayushtkn July 3, 2022 12:01

ayushtkn approved these changes Jul 3, 2022

View reviewed changes

ferhui merged commit 243efe2 into apache:master Jul 5, 2022

HDDS-6938. handle NPE when removing prefixAcl #3568

HDDS-6938. handle NPE when removing prefixAcl #3568

Uh oh!

Conversation

mohan3d commented Jun 29, 2022 • 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!

mohan3d commented Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

aswinshakil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adoroszlai commented Jun 30, 2022

Uh oh!

mohan3d commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

ayushtkn Jul 2, 2022

Choose a reason for hiding this comment

Uh oh!

mohan3d Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

ayushtkn Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

mohan3d Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

ayushtkn left a comment

Choose a reason for hiding this comment

Uh oh!

ferhui commented Jul 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mohan3d commented Jun 29, 2022 •

edited

Loading

mohan3d commented Jun 29, 2022 •

edited

Loading

mohan3d commented Jun 30, 2022 •

edited

Loading