HDDS-13999. Complete ignore the ACL sent by client during object crea…#21
HDDS-13999. Complete ignore the ACL sent by client during object crea…#21
Conversation
Summary of ChangesHello @ChenSammi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new configuration option in Ozone Manager that allows administrators to control whether client-provided Access Control Lists (ACLs) are honored during the creation of storage objects like volumes, buckets, and keys. By default, client ACLs are considered, but with this new setting, they can be optionally ignored, ensuring that only server-side default ACLs are applied. This enhances security and policy enforcement by giving administrators more granular control over object access permissions. The changes involve updating core object creation request handlers and adding extensive test cases to verify the correct behavior of this new security control. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration, object.creation.ignore.client.acls, to allow Ozone Manager to ignore ACLs provided by clients during the creation of volumes, buckets, and keys. This is a valuable feature for environments where ACLs need to be managed centrally. The implementation is straightforward, adding the configuration to OmConfig and applying it in the respective request handlers. The changes are well-supported by a comprehensive set of parameterized tests that validate the behavior for both states of the new configuration. My review includes a couple of suggestions to improve the code's correctness when checking for the presence of ACLs in protobuf messages.
| if (keyArgs.getAclsList() != null && !config.ignoreClientACLs()) { | ||
| acls.addAll(OzoneAclUtil.fromProtobuf(keyArgs.getAclsList())); | ||
| } |
There was a problem hiding this comment.
The check keyArgs.getAclsList() != null is redundant. For a repeated field in a protobuf message, the corresponding getter method (getAclsList() in this case) never returns null; it returns an empty list if the field is not set. It is more idiomatic and correct to check if the list is not empty using !keyArgs.getAclsList().isEmpty().
| if (keyArgs.getAclsList() != null && !config.ignoreClientACLs()) { | |
| acls.addAll(OzoneAclUtil.fromProtobuf(keyArgs.getAclsList())); | |
| } | |
| if (!keyArgs.getAclsList().isEmpty() && !config.ignoreClientACLs()) { | |
| acls.addAll(OzoneAclUtil.fromProtobuf(keyArgs.getAclsList())); | |
| } | |
| if (keyArgs.getAclsList() != null && !config.ignoreClientACLs()) { | ||
| acls.addAll(OzoneAclUtil.fromProtobuf(keyArgs.getAclsList())); | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, the check keyArgs.getAclsList() != null is redundant because getAclsList() for a repeated protobuf field never returns null. Please check if the list is not empty instead for correctness and consistency.
| if (keyArgs.getAclsList() != null && !config.ignoreClientACLs()) { | |
| acls.addAll(OzoneAclUtil.fromProtobuf(keyArgs.getAclsList())); | |
| } | |
| if (!keyArgs.getAclsList().isEmpty() && !config.ignoreClientACLs()) { | |
| acls.addAll(OzoneAclUtil.fromProtobuf(keyArgs.getAclsList())); | |
| } | |
There was a problem hiding this comment.
Pull request overview
This pull request implements functionality to ignore client-provided ACLs during object creation in Ozone Manager. The feature adds a new configuration option ozone.om.object.creation.ignore.client.acls that when enabled, ensures only default ACLs are applied to newly created volumes, buckets, keys, and directories, ignoring any ACLs sent by the client.
Key changes:
- Added a new boolean configuration property
object.creation.ignore.client.acls(defaults tofalsefor backward compatibility) - Modified ACL handling logic in volume, bucket, key, and directory creation requests to conditionally exclude client ACLs based on the configuration
- Added comprehensive parameterized tests for all object types to verify the feature works correctly in both modes
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OmConfig.java | Added new configuration property ignoreClientACLs with getter/setter methods |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeCreateRequest.java | Modified volume creation to check configuration before adding client ACLs |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketCreateRequest.java | Modified bucket creation to check configuration before adding client ACLs |
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java | Modified key and directory ACL handling methods to check configuration before adding client ACLs |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/volume/TestOMVolumeRequest.java | Updated helper method signature to accept ACL parameter for more flexible testing |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/volume/TestOMVolumeCreateRequest.java | Added parameterized test for volume creation ACL ignoring and updated existing test calls |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/bucket/TestOMBucketCreateRequest.java | Added parameterized test for bucket creation ACL ignoring |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyCreateRequest.java | Added parameterized test for key creation ACL ignoring |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMFileCreateRequest.java | Added parameterized test for file creation ACL ignoring and enhanced helper method to accept ACLs |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequest.java | Added parameterized test for directory creation ACL ignoring and enhanced helper method to accept ACLs |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/file/TestOMDirectoryCreateRequestWithFSO.java | Added parameterized test for directory creation ACL ignoring (FSO layout) and enhanced helper method to accept ACLs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| defaultValue = "false", | ||
| type = ConfigType.BOOLEAN, | ||
| tags = {ConfigTag.OM, ConfigTag.SECURITY}, | ||
| description = "Ignore native ACLs sent by client to OzoneManager during volume/bucket/key creation." |
There was a problem hiding this comment.
[nitpick] The description should use "client-sent" or "client-provided" instead of "native" for clarity. The term "native ACLs" is ambiguous and could be confusing. Consider: "Ignore ACLs sent by client to OzoneManager during volume/bucket/key creation."
| description = "Ignore native ACLs sent by client to OzoneManager during volume/bucket/key creation." | |
| description = "Ignore ACLs sent by client to OzoneManager during volume/bucket/key creation." |
ChenSammi
force-pushed
the
HDDS-13999
branch
from
47f9275 to
a49986b
Compare
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
|
Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it. |
…tion
What changes were proposed in this pull request?
Provide a one-liner summary of the changes in the PR Title field above.
It should be in the form of
HDDS-1234. Short summary of the change.Please describe your PR in detail:
perspective not just for the reviewer.
the Jira's description if the jira is well defined.
issue investigation, github discussion, etc.
Examples of well-written pull requests:
What is the link to the Apache JIRA
Please create an issue in ASF JIRA before opening a pull request, and you need to set the title of the pull
request which starts with the corresponding JIRA issue number. (e.g. HDDS-XXXX. Fix a typo in YYY.)
(Please replace this section with the link to the Apache JIRA)
How was this patch tested?
(Please explain how this patch was tested. Ex: unit tests, manual tests, workflow run on the fork git repo.)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this.)