You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ivandika3 (via GitHub)" <gi...@apache.org> on 2024/02/24 08:27:03 UTC
[PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
ivandika3 opened a new pull request, #6268:
URL: https://github.com/apache/ozone/pull/6268
## What changes were proposed in this pull request?
Following up on [HDDS-4715](https://issues.apache.org/jira/browse/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #6268:
URL: https://github.com/apache/ozone/pull/6268#issuecomment-1962388223
@whbing @ChenSammi Could you help take a look when you have time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6268:
URL: https://github.com/apache/ozone/pull/6268#issuecomment-1978248776
@ChenSammi would you like to take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #6268:
URL: https://github.com/apache/ozone/pull/6268#discussion_r1511245070
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java:
##########
@@ -145,16 +142,21 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
} finally {
if (lockAcquired) {
mergeOmLockDetails(omMetadataManager.getLock()
- .releaseWriteLock(PREFIX_LOCK, getOzoneObj().getPath()));
+ .releaseWriteLock(PREFIX_LOCK, prefixPath));
}
if (omClientResponse != null) {
omClientResponse.setOmLockDetails(getOmLockDetails());
}
}
- OzoneObj obj = getOzoneObj();
+ OzoneObj obj = resolvedPrefixObj;
+ if (obj == null) {
Review Comment:
If `IOException` is thrown during the `getResolvedPrefixObj`, the `resolvedPrefixObj` should be null and we should fall back to the initial ozone object from `getOzoneObj`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #6268:
URL: https://github.com/apache/ozone/pull/6268#discussion_r1510893100
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java:
##########
@@ -322,23 +331,53 @@ public OMPrefixAclOpResult setAcl(OzoneObj ozoneObj, List<OzoneAcl> ozoneAcls,
prefixInfoBuilder.setUpdateID(transactionLogIndex);
}
prefixInfo = prefixInfoBuilder.build();
+ newPrefix = true;
}
boolean changed = prefixInfo.setAcls(ozoneAcls);
- inheritParentAcl(ozoneObj, prefixInfo);
+ if (newPrefix) {
+ inheritParentAcl(ozoneObj, prefixInfo);
+ }
prefixTree.insert(ozoneObj.getPath(), prefixInfo);
if (!isRatisEnabled) {
metadataManager.getPrefixTable().put(ozoneObj.getPath(), prefixInfo);
}
return new OMPrefixAclOpResult(prefixInfo, changed);
}
+ /**
+ * Get the resolved prefix object to handle prefix that is under a link bucket.
+ * @param obj prefix object
+ * @return the resolved prefix object if the object belongs under a link bucket.
+ * Otherwise, return the same prefix object.
+ * @throws IOException Exception thrown when resolving the bucket link.
+ */
+ private OzoneObj getResolvedPrefixObj(OzoneObj obj) throws IOException {
+ if (StringUtils.isEmpty(obj.getVolumeName()) || StringUtils.isEmpty(obj.getBucketName())) {
+ return obj;
+ }
+
+ ResolvedBucket resolvedBucket = ozoneManager.resolveBucketLink(
+ Pair.of(obj.getVolumeName(), obj.getBucketName()));
+ String realVolume = resolvedBucket.realVolume();
+ String realBucket = resolvedBucket.realBucket();
+ // If the bucket is not a link bucket, just return the Ozone object
+ if (realVolume.equals(obj.getVolumeName()) && realBucket.equals(obj.getBucketName())) {
Review Comment:
Thank you for the review. It's superseded by the suggested extraction in the other suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6268:
URL: https://github.com/apache/ozone/pull/6268#discussion_r1510839456
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java:
##########
@@ -322,23 +331,53 @@ public OMPrefixAclOpResult setAcl(OzoneObj ozoneObj, List<OzoneAcl> ozoneAcls,
prefixInfoBuilder.setUpdateID(transactionLogIndex);
}
prefixInfo = prefixInfoBuilder.build();
+ newPrefix = true;
}
boolean changed = prefixInfo.setAcls(ozoneAcls);
- inheritParentAcl(ozoneObj, prefixInfo);
+ if (newPrefix) {
+ inheritParentAcl(ozoneObj, prefixInfo);
+ }
prefixTree.insert(ozoneObj.getPath(), prefixInfo);
if (!isRatisEnabled) {
metadataManager.getPrefixTable().put(ozoneObj.getPath(), prefixInfo);
}
return new OMPrefixAclOpResult(prefixInfo, changed);
}
+ /**
+ * Get the resolved prefix object to handle prefix that is under a link bucket.
+ * @param obj prefix object
+ * @return the resolved prefix object if the object belongs under a link bucket.
+ * Otherwise, return the same prefix object.
+ * @throws IOException Exception thrown when resolving the bucket link.
+ */
+ private OzoneObj getResolvedPrefixObj(OzoneObj obj) throws IOException {
+ if (StringUtils.isEmpty(obj.getVolumeName()) || StringUtils.isEmpty(obj.getBucketName())) {
+ return obj;
+ }
+
+ ResolvedBucket resolvedBucket = ozoneManager.resolveBucketLink(
+ Pair.of(obj.getVolumeName(), obj.getBucketName()));
+ String realVolume = resolvedBucket.realVolume();
+ String realBucket = resolvedBucket.realBucket();
+ // If the bucket is not a link bucket, just return the Ozone object
+ if (realVolume.equals(obj.getVolumeName()) && realBucket.equals(obj.getBucketName())) {
+ return obj;
+ }
+ // If the bucket is a link bucket, return a new OzoneObj with resolved volume and bucket name
+ return OzoneObjInfo.Builder.fromOzoneObj(obj)
+ .setVolumeName(realVolume).setBucketName(realBucket).build();
+ }
Review Comment:
We can extract this part as `ResolvedBucket#update(OzoneObj)`, similar to:
https://github.com/apache/ozone/blob/11c5eb86a4f43c4297a39a496051173e420b9edc/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ResolvedBucket.java#L105-L112
It can then be reused in:
https://github.com/apache/ozone/blob/fb1ad58359de34e44e0e9ddfca3314f369011d1a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java#L84-L91
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java:
##########
@@ -322,23 +331,53 @@ public OMPrefixAclOpResult setAcl(OzoneObj ozoneObj, List<OzoneAcl> ozoneAcls,
prefixInfoBuilder.setUpdateID(transactionLogIndex);
}
prefixInfo = prefixInfoBuilder.build();
+ newPrefix = true;
}
boolean changed = prefixInfo.setAcls(ozoneAcls);
- inheritParentAcl(ozoneObj, prefixInfo);
+ if (newPrefix) {
+ inheritParentAcl(ozoneObj, prefixInfo);
+ }
prefixTree.insert(ozoneObj.getPath(), prefixInfo);
if (!isRatisEnabled) {
metadataManager.getPrefixTable().put(ozoneObj.getPath(), prefixInfo);
}
return new OMPrefixAclOpResult(prefixInfo, changed);
}
+ /**
+ * Get the resolved prefix object to handle prefix that is under a link bucket.
+ * @param obj prefix object
+ * @return the resolved prefix object if the object belongs under a link bucket.
+ * Otherwise, return the same prefix object.
+ * @throws IOException Exception thrown when resolving the bucket link.
+ */
+ private OzoneObj getResolvedPrefixObj(OzoneObj obj) throws IOException {
+ if (StringUtils.isEmpty(obj.getVolumeName()) || StringUtils.isEmpty(obj.getBucketName())) {
+ return obj;
+ }
+
+ ResolvedBucket resolvedBucket = ozoneManager.resolveBucketLink(
+ Pair.of(obj.getVolumeName(), obj.getBucketName()));
+ String realVolume = resolvedBucket.realVolume();
+ String realBucket = resolvedBucket.realBucket();
+ // If the bucket is not a link bucket, just return the Ozone object
+ if (realVolume.equals(obj.getVolumeName()) && realBucket.equals(obj.getBucketName())) {
Review Comment:
nit: `ResolvedBucket` has a method to check the same.
```suggestion
if (!resolvedBucket.isLink()) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "whbing (via GitHub)" <gi...@apache.org>.
whbing commented on code in PR #6268:
URL: https://github.com/apache/ozone/pull/6268#discussion_r1511052255
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java:
##########
@@ -145,16 +142,21 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
} finally {
if (lockAcquired) {
mergeOmLockDetails(omMetadataManager.getLock()
- .releaseWriteLock(PREFIX_LOCK, getOzoneObj().getPath()));
+ .releaseWriteLock(PREFIX_LOCK, prefixPath));
}
if (omClientResponse != null) {
omClientResponse.setOmLockDetails(getOmLockDetails());
}
}
- OzoneObj obj = getOzoneObj();
+ OzoneObj obj = resolvedPrefixObj;
+ if (obj == null) {
Review Comment:
Is null condition necessary here ? I see a call to `resolvedPrefixObj.getpath ()` on line 81, 82.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java:
##########
@@ -68,28 +66,27 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
boolean lockAcquired = false;
String volume = null;
String bucket = null;
- String key = null;
+ String prefix = null;
Review Comment:
`String prefix` or `String prefixPath` maybe only need one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "whbing (via GitHub)" <gi...@apache.org>.
whbing commented on PR #6268:
URL: https://github.com/apache/ozone/pull/6268#issuecomment-1977919186
@ivandika3 Thanks ! LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #6268:
URL: https://github.com/apache/ozone/pull/6268#discussion_r1511246788
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java:
##########
@@ -68,28 +66,27 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
boolean lockAcquired = false;
String volume = null;
String bucket = null;
- String key = null;
+ String prefix = null;
Review Comment:
Thank you for the review. I removed all the unnecessary variables (`volume`, `bucket`, and `prefix`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #6268:
URL: https://github.com/apache/ozone/pull/6268#discussion_r1510893463
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/PrefixManagerImpl.java:
##########
@@ -322,23 +331,53 @@ public OMPrefixAclOpResult setAcl(OzoneObj ozoneObj, List<OzoneAcl> ozoneAcls,
prefixInfoBuilder.setUpdateID(transactionLogIndex);
}
prefixInfo = prefixInfoBuilder.build();
+ newPrefix = true;
}
boolean changed = prefixInfo.setAcls(ozoneAcls);
- inheritParentAcl(ozoneObj, prefixInfo);
+ if (newPrefix) {
+ inheritParentAcl(ozoneObj, prefixInfo);
+ }
prefixTree.insert(ozoneObj.getPath(), prefixInfo);
if (!isRatisEnabled) {
metadataManager.getPrefixTable().put(ozoneObj.getPath(), prefixInfo);
}
return new OMPrefixAclOpResult(prefixInfo, changed);
}
+ /**
+ * Get the resolved prefix object to handle prefix that is under a link bucket.
+ * @param obj prefix object
+ * @return the resolved prefix object if the object belongs under a link bucket.
+ * Otherwise, return the same prefix object.
+ * @throws IOException Exception thrown when resolving the bucket link.
+ */
+ private OzoneObj getResolvedPrefixObj(OzoneObj obj) throws IOException {
+ if (StringUtils.isEmpty(obj.getVolumeName()) || StringUtils.isEmpty(obj.getBucketName())) {
+ return obj;
+ }
+
+ ResolvedBucket resolvedBucket = ozoneManager.resolveBucketLink(
+ Pair.of(obj.getVolumeName(), obj.getBucketName()));
+ String realVolume = resolvedBucket.realVolume();
+ String realBucket = resolvedBucket.realBucket();
+ // If the bucket is not a link bucket, just return the Ozone object
+ if (realVolume.equals(obj.getVolumeName()) && realBucket.equals(obj.getBucketName())) {
+ return obj;
+ }
+ // If the bucket is a link bucket, return a new OzoneObj with resolved volume and bucket name
+ return OzoneObjInfo.Builder.fromOzoneObj(obj)
+ .setVolumeName(realVolume).setBucketName(realBucket).build();
+ }
Review Comment:
Thank you for the suggestion. Updated. PTAL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #6268:
URL: https://github.com/apache/ozone/pull/6268#discussion_r1511245070
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java:
##########
@@ -145,16 +142,21 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
} finally {
if (lockAcquired) {
mergeOmLockDetails(omMetadataManager.getLock()
- .releaseWriteLock(PREFIX_LOCK, getOzoneObj().getPath()));
+ .releaseWriteLock(PREFIX_LOCK, prefixPath));
}
if (omClientResponse != null) {
omClientResponse.setOmLockDetails(getOmLockDetails());
}
}
- OzoneObj obj = getOzoneObj();
+ OzoneObj obj = resolvedPrefixObj;
+ if (obj == null) {
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6268:
URL: https://github.com/apache/ozone/pull/6268#issuecomment-1978594688
Thanks @ivandika3 for the patch, @whbing for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #6268:
URL: https://github.com/apache/ozone/pull/6268
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10412. Prefix ACL check needs to resolve the bucket link [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #6268:
URL: https://github.com/apache/ozone/pull/6268#issuecomment-1978636568
Thank you @adoroszlai @whbing for the reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org