Skip to content

Conversation

@ivandika3
Copy link
Contributor

@ivandika3 ivandika3 commented Feb 24, 2024

What changes were proposed in this pull request?

Following up on HDDS-4715, we also need to resolve the bucket link for prefix ACL.

Here are the changes included in this patch:

  • Include prefix resource type to be BucketManagerImpl so that prefix access under a bucket link will be resolved to the source bucket
  • Prefix ACL read: PrefixManagerImpl#getAcl will resolve the bucket to get the ACL
  • Prefix ACL write: OMPrefixAclRequest#validateAndUpdateCache will resolve the bucket before writing the ACL
    • This requires passing the resolved prefix OzoneObj to the request hooks (onComplete, apply) so that the request implementation can use the resolved object and not the link object
  • Fixed PrefixManagerImpl#setAcl so that default ACL is inherited only when the prefix does not exist
    • This was found in the integration test
  • Some comments and cosmetics improvements

What is the link to the Apache JIRA

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

How was this patch tested?

Acceptance and integration tests.

Clean CI run: https://github.com/ivandika3/ozone/actions/runs/8029229022

@ivandika3 ivandika3 marked this pull request as ready for review February 24, 2024 14:29
@ivandika3
Copy link
Contributor Author

@whbing @ChenSammi Could you help take a look when you have time?

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 @ivandika3 for the patch, LGTM.

String volume = null;
String bucket = null;
String key = null;
String prefix = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

String prefix or String prefixPath maybe only need one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. I removed all the unnecessary variables (volume, bucket, and prefix)


OzoneObj obj = getOzoneObj();
OzoneObj obj = resolvedPrefixObj;
if (obj == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is null condition necessary here ? I see a call to resolvedPrefixObj.getpath () on line 81, 82.

Copy link
Contributor Author

@ivandika3 ivandika3 Mar 4, 2024

Choose a reason for hiding this comment

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

If IOException is thrown during the getResolvedPrefixObj (i.e. during bucket resolving process), the resolvedPrefixObj should be null and we should fall back to the initial ozone object from getOzoneObj.

@whbing
Copy link
Contributor

whbing commented Mar 5, 2024

@ivandika3 Thanks ! LGTM.

@adoroszlai
Copy link
Contributor

@ChenSammi would you like to take a look?

@adoroszlai adoroszlai merged commit b69674c into apache:master Mar 5, 2024
@adoroszlai
Copy link
Contributor

Thanks @ivandika3 for the patch, @whbing for the review.

@ivandika3
Copy link
Contributor Author

Thank you @adoroszlai @whbing for the reviews.

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Mar 15, 2024
…#6268)

(cherry picked from commit b69674c)
Change-Id: I3dff7457c944e35560904f0e7497e69d407a5b3f
@ivandika3 ivandika3 self-assigned this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants