Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HDDS-4787. Trash emptier fails to create checkpoints in a secure setup #1888

Merged
merged 5 commits into from
Feb 4, 2021
Merged

HDDS-4787. Trash emptier fails to create checkpoints in a secure setup #1888

merged 5 commits into from
Feb 4, 2021

Conversation

sadanand48
Copy link
Contributor

@sadanand48 sadanand48 commented Feb 3, 2021 •
edited
Loading

What changes were proposed in this pull request?

Since in TrashOzoneFilesystem the calls are internal and not through RPC, NPE is generated during checkacls(), Since Server.getRemoteIp and other such parameters would be null, the security authorizers too generate an NPE. The fix here is if Server.getRemoteIp() is null replace it with ip of the node that starts OM

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-4787#

How was this patch tested?

Existing unit tests. also tried on cluster https://github.com/sadanand48/hadoop-ozone/runs/1825648963?check_suite_focus=true

adoroszlai
adoroszlai requested changes Feb 3, 2021
View reviewed changes
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sadanand48 for working on this.

I think secure acceptance test failure is related:

om_1        | 2021-02-03 05:18:00,113 [OM StateMachine ApplyTransaction Thread - 0] ERROR ratis.OzoneManagerStateMachine: Terminating with exit status 1: Request cmdType: RenameKey
om_1        | clientId: "client-6670A68B0F8D"
om_1        | userInfo {
om_1        | }
om_1        | renameKeyRequest {
om_1        |   keyArgs {
om_1        |     volumeName: "fstest1-src"
om_1        |     bucketName: "link1-o3fs-src"
om_1        |     keyName: ".Trash/root/Current/"
om_1        |     modificationTime: 1612329480031
om_1        |   }
om_1        |   toKeyName: ".Trash/root/210203051800/"
om_1        | }
om_1        | failed with exception
om_1        | java.lang.NullPointerException
om_1        | 	at org.apache.hadoop.ozone.security.acl.OzoneNativeAuthorizer.isAdmin(OzoneNativeAuthorizer.java:222)
om_1        | 	at org.apache.hadoop.ozone.security.acl.OzoneNativeAuthorizer.checkAccess(OzoneNativeAuthorizer.java:92)
om_1        | 	at org.apache.hadoop.ozone.om.OzoneManager.checkAcls(OzoneManager.java:1797)
om_1        | 	at org.apache.hadoop.ozone.om.request.OMClientRequest.checkAcls(OMClientRequest.java:176)
om_1        | 	at org.apache.hadoop.ozone.om.request.OMClientRequest.checkAcls(OMClientRequest.java:154)
om_1        | 	at org.apache.hadoop.ozone.om.request.key.OMKeyRequest.checkKeyAcls(OMKeyRequest.java:437)
om_1        | 	at org.apache.hadoop.ozone.om.request.key.OMKeyRenameRequest.validateAndUpdateCache(OMKeyRenameRequest.java:143)

https://github.com/apache/ozone/runs/1819931384#step:7:549
https://github.com/apache/ozone/suites/1956204276/artifacts/38766607

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/TrashOzoneFileSystem.java Outdated

if (remoteAddress != null) {
userInfo.setHostName(remoteAddress.getHostName());
userInfo.setRemoteAddress(remoteAddress.getHostAddress()).build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
userInfo.setRemoteAddress(remoteAddress.getHostAddress()).build();
userInfo.setRemoteAddress(remoteAddress.getHostAddress());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java Outdated
Comment on lines 1702 to 1703
ProtobufRpcEngine.Server.getRemoteUser() != null ?
ProtobufRpcEngine.Server.getRemoteUser() :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can you please store these in local variables instead of multiple calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

xiaoyuyao
xiaoyuyao reviewed Feb 3, 2021
View reviewed changes
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
resolved = resolveBucketLink(requested, new HashSet<>(),
null, null, null);
try {
if (isAclEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with @adoroszlai can we use a local variable to hold the result of Server.getRemoteIp().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

xiaoyuyao
xiaoyuyao reviewed Feb 3, 2021
View reviewed changes
Copy link
Contributor

@xiaoyuyao xiaoyuyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sadanand48 for working on this. Patch LGTM, just some minor comments. Can you add a test to cover this?

@sadanand48
Copy link
Contributor Author

Thanks @xiaoyuyao for reviewing this . TestOzoneFilesystem testTrash() is the unit test for this and this issue would arise when OZONE_ACL_ENABLED is set to true. I have made that change. There is also a robot test case for trash deletion inside ozone-fs.robot . The secure acceptance CI check will test that in a secure environment with native acls enabled.

@sadanand48
Copy link
Contributor Author

Thanks @adoroszlai for the review and pointing out the error in the secure acceptance check. It's passing now.

@adoroszlai
Copy link
Contributor

Thanks @sadanand48 for updating the patch.

@adoroszlai adoroszlai changed the title HDDS-4787.Trash emptier fails to create checkpoints in a secure setup HDDS-4787. Trash emptier fails to create checkpoints in a secure setup Feb 4, 2021
mukul1987
mukul1987 approved these changes Feb 4, 2021
View reviewed changes
Copy link
Contributor

@mukul1987 mukul1987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@mukul1987 mukul1987 merged commit fa5a080 into apache:master Feb 4, 2021
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Mar 11, 2021
@sadanand48
HDDS-4787. Trash emptier fails to create checkpoints in a secure setup (
47a5288 
apache#1888)

(cherry picked from commit fa5a080)
Change-Id: I5d9ffe21a9a42f8d957110d2fcc282ec5049a3f5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoyuyao xiaoyuyao xiaoyuyao left review comments

@mukul1987 mukul1987 mukul1987 approved these changes

@adoroszlai adoroszlai adoroszlai approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sadanand48 @adoroszlai @mukul1987 @xiaoyuyao