You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/06/13 10:54:56 UTC
[GitHub] [ozone] JyotinderSingh opened a new pull request, #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
JyotinderSingh opened a new pull request, #3508:
URL: https://github.com/apache/ozone/pull/3508
## What changes were proposed in this pull request?
In high concurrency scenarios (which will become more common once we introduced prefix-based locking), there is a possibility of the following race condition:
Take for instance the following scenario and 3 concurrent write requests:
Bucket `vol/buck1` exists with `LEGACY` layout.
Request 1: `CreateKey` by an older client (pre- bucket layout) on a bucket `vol/buck1`.
Request 2: `DeleteBucket` by a new client on the bucket `vol/buck1`.
Request 3: `CreateBucket` by a new client on the bucket `vol/buck1` with layout `FILE_SYSTEM_OPTIMIZED`.
Let's say that these requests are processed in the following order:
1. `Request 1` is picked up by one of the threads, which proceeds to run the `PRE_PROCESS` validations on this request. The validator we are interested in is called `blockCreateKeyWithBucketLayoutFromOldClient`. This validator will make sure that the bucket associated with this request is a `LEGACY` bucket - which is the pre-defined behavior in the case of old client/new cluster interactions since we do not want an old client operating on buckets using a new metadata layout.
One thing to know here is that at this stage, the OM does not hold a bucket lock (which only happens inside the `updateAndValidateCache` method associated with the write request's handler class).
2. While `Request 1` was being processed, another thread was processing `Request 2`. Let's say `Request2' managed to get hold of the bucket lock and successfully completed the bucket deletion.
3. Now before `Request 1` got a chance to acquire the bucket lock, `Request 3` manages to acquire it. It proceeds with the bucket creation and creates a new bucket `vol/buck1` with `FILE_SYSTEM_OPTIMIZED` bucket layout.
4. Finally, `Request 1` is able to acquire the bucket lock and proceeds to enter its `validateAndUpdateCache` method. However, even though it is able to find the bucket it is looking for, this is not the same bucket that was validated in its pre-processing hook. This new bucket has the same name, but a different bucket layout. The request ends up modifying a bucket that it should not be allowed to touch.
This race condition can lead to undefined behavior of the Ozone cluster, where older clients might be modifying information they do not understand.
This PR aims to add bucket ID validation to the request processing flow, which would make sure that the bucket that ends up being processed is the same one that was validated.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6682
## How was this patch tested?
Existing test cases.
--
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
[GitHub] [ozone] duongkame commented on pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
duongkame commented on PR #3508:
URL: https://github.com/apache/ozone/pull/3508#issuecomment-1306380508
As @errose28 already pointed out as per HDDS-6931, the common theme of the problems is: pre-process validation should only perform logic on data which is not supposed to change between pre-process and actual execution in `validateAndUpdateCache`. Otherwise, that validation should be moved to `validateAndUpdateCache` (and is done together with the actual data change in a proper lock scope) to ensure consistency.
This change introduces a new mechanism to solve a rare case when a bucket is re-created between pre-processing (a bucket needs to be empty to be deleted, so this case may not happen in a high concurrency). Why don't we just validate the bucket layout in `validateAndUpdateCache`?
--
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
[GitHub] [ozone] JyotinderSingh commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r945325588
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -324,6 +345,181 @@ private OMResponse createErrorResponse(
return omResponse.build();
}
+ /**
+ * Associate bucket ID with requests that have an associated bucket.
+ * Returns a new OMRequest object with the bucket ID associated.
+ *
+ * @param omRequest OMRequest
+ * @return OMRequest with bucket ID associated.
+ * @throws IOException
+ */
+ @SuppressWarnings("checkstyle:MethodLength")
+ private OMRequest associateBucketIdWithRequest(OMRequest omRequest)
+ throws IOException {
+ OMMetadataManager metadataManager = ozoneManager.getMetadataManager();
+ String volumeName = "";
+ String bucketName = "";
+
+ OzoneManagerProtocolProtos.KeyArgs keyArgs;
+ OzoneObj obj;
+ ObjectParser objectParser;
+ ObjectType type;
+
+ switch (omRequest.getCmdType()) {
+ case AddAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getAddAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case RemoveAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getRemoveAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case SetAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getSetAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case DeleteBucket:
+ volumeName = omRequest.getDeleteBucketRequest().getVolumeName();
+ bucketName = omRequest.getDeleteBucketRequest().getBucketName();
+ break;
+ case CreateDirectory:
+ keyArgs = omRequest.getCreateDirectoryRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CreateFile:
+ keyArgs = omRequest.getCreateFileRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CreateKey:
+ keyArgs = omRequest.getCreateKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case AllocateBlock:
+ keyArgs = omRequest.getAllocateBlockRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CommitKey:
+ keyArgs = omRequest.getCommitKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case DeleteKey:
+ keyArgs = omRequest.getDeleteKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case DeleteKeys:
+ OzoneManagerProtocolProtos.DeleteKeyArgs deleteKeyArgs =
+ omRequest.getDeleteKeysRequest()
+ .getDeleteKeys();
+ volumeName = deleteKeyArgs.getVolumeName();
+ bucketName = deleteKeyArgs.getBucketName();
+ break;
+ case RenameKey:
+ keyArgs = omRequest.getRenameKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case RenameKeys:
+ OzoneManagerProtocolProtos.RenameKeysArgs renameKeysArgs =
+ omRequest.getRenameKeysRequest().getRenameKeysArgs();
+ volumeName = renameKeysArgs.getVolumeName();
+ bucketName = renameKeysArgs.getBucketName();
+ break;
+ case InitiateMultiPartUpload:
+ keyArgs = omRequest.getInitiateMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CommitMultiPartUpload:
+ keyArgs = omRequest.getCommitMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case AbortMultiPartUpload:
+ keyArgs = omRequest.getAbortMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CompleteMultiPartUpload:
+ keyArgs = omRequest.getCompleteMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ default:
+ // do nothing in case of other requests.
+ break;
+ }
+
+ // Check if there is any bucket associated with this request.
+ if (bucketName.equals("") && volumeName.equals("")) {
+ return omRequest;
+ }
+
+ long bucketId;
+ try {
+ // Note: Even though this block of code is not executing under a bucket
+ // lock - it is still safe.
+ // For instance, consider the following link bucket chain:
+ // l1 -> l2 -> l3 -> abc (ID: 1000)
+ // Let's say we fetch the resolved bucket name for l1. This would mean
+ // the source bucket is resolved as 'abc'.
+ // Now, let's assume that before the next line of code (to fetch bucket
+ // ID) executes, the link structure changes as follows:
+ // l1- > l2 -> l3 -> xyz (ID: 1001)
+ // And we end up associating the bucket ID for l1 with abc (1000) even
+ // though the actual link has changed.
+ // This is not a problem, since we will anyway be validating this bucket
+ // ID inside validateAndUpdateCache method - and it will be caught there.
+ // This is a fail-slow approach.
+ ResolvedBucket resolvedBucket =
+ ozoneManager.resolveBucketLink(Pair.of(volumeName, bucketName));
+ bucketId = metadataManager.getBucketId(resolvedBucket.realVolume(),
+ resolvedBucket.realBucket());
+ } catch (OMException oe) {
+ // Ignore exceptions at this stage, let respective classes handle them.
Review Comment:
Done
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -324,6 +345,181 @@ private OMResponse createErrorResponse(
return omResponse.build();
}
+ /**
+ * Associate bucket ID with requests that have an associated bucket.
+ * Returns a new OMRequest object with the bucket ID associated.
+ *
+ * @param omRequest OMRequest
+ * @return OMRequest with bucket ID associated.
+ * @throws IOException
+ */
+ @SuppressWarnings("checkstyle:MethodLength")
+ private OMRequest associateBucketIdWithRequest(OMRequest omRequest)
+ throws IOException {
+ OMMetadataManager metadataManager = ozoneManager.getMetadataManager();
+ String volumeName = "";
+ String bucketName = "";
+
+ OzoneManagerProtocolProtos.KeyArgs keyArgs;
+ OzoneObj obj;
+ ObjectParser objectParser;
+ ObjectType type;
+
+ switch (omRequest.getCmdType()) {
+ case AddAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getAddAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case RemoveAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getRemoveAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case SetAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getSetAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case DeleteBucket:
+ volumeName = omRequest.getDeleteBucketRequest().getVolumeName();
+ bucketName = omRequest.getDeleteBucketRequest().getBucketName();
+ break;
+ case CreateDirectory:
+ keyArgs = omRequest.getCreateDirectoryRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CreateFile:
+ keyArgs = omRequest.getCreateFileRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CreateKey:
+ keyArgs = omRequest.getCreateKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case AllocateBlock:
+ keyArgs = omRequest.getAllocateBlockRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CommitKey:
+ keyArgs = omRequest.getCommitKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case DeleteKey:
+ keyArgs = omRequest.getDeleteKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case DeleteKeys:
+ OzoneManagerProtocolProtos.DeleteKeyArgs deleteKeyArgs =
+ omRequest.getDeleteKeysRequest()
+ .getDeleteKeys();
+ volumeName = deleteKeyArgs.getVolumeName();
+ bucketName = deleteKeyArgs.getBucketName();
+ break;
+ case RenameKey:
+ keyArgs = omRequest.getRenameKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case RenameKeys:
+ OzoneManagerProtocolProtos.RenameKeysArgs renameKeysArgs =
+ omRequest.getRenameKeysRequest().getRenameKeysArgs();
+ volumeName = renameKeysArgs.getVolumeName();
+ bucketName = renameKeysArgs.getBucketName();
+ break;
+ case InitiateMultiPartUpload:
+ keyArgs = omRequest.getInitiateMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CommitMultiPartUpload:
+ keyArgs = omRequest.getCommitMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case AbortMultiPartUpload:
+ keyArgs = omRequest.getAbortMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CompleteMultiPartUpload:
+ keyArgs = omRequest.getCompleteMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ default:
+ // do nothing in case of other requests.
Review Comment:
Done.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequestUtils.java:
##########
@@ -47,4 +53,50 @@ public static void checkClientRequestPrecondition(
OMException.ResultCodes.INTERNAL_ERROR);
}
}
+
+ /**
+ * Validates the bucket associated with the request - to make sure it did
+ * not change since the request started processing.
+ *
+ * @param bucketId - bucket ID of the associated bucket when the request
+ * is being processed.
+ * @param omRequest - request to be validated, contains the bucket ID of the
+ * associated bucket when the request was created.
+ * @throws OMException
+ */
+ public static void validateAssociatedBucketId(long bucketId,
+ OMRequest omRequest)
+ throws OMException {
+ if (omRequest.hasAssociatedBucketId()) {
+ if (bucketId != omRequest.getAssociatedBucketId()) {
+ throw new OMException(
+ "Bucket ID mismatch. Associated bucket was modified while this" +
Review Comment:
Done
--
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
[GitHub] [ozone] JyotinderSingh commented on pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3508:
URL: https://github.com/apache/ozone/pull/3508#issuecomment-1229467643
Hello, @errose28 @rakeshadr. I have rebased the PR on top of the latest master. Could you please review it?
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r942345956
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -324,6 +345,181 @@ private OMResponse createErrorResponse(
return omResponse.build();
}
+ /**
+ * Associate bucket ID with requests that have an associated bucket.
+ * Returns a new OMRequest object with the bucket ID associated.
+ *
+ * @param omRequest OMRequest
+ * @return OMRequest with bucket ID associated.
+ * @throws IOException
+ */
+ @SuppressWarnings("checkstyle:MethodLength")
+ private OMRequest associateBucketIdWithRequest(OMRequest omRequest)
+ throws IOException {
+ OMMetadataManager metadataManager = ozoneManager.getMetadataManager();
+ String volumeName = "";
+ String bucketName = "";
+
+ OzoneManagerProtocolProtos.KeyArgs keyArgs;
+ OzoneObj obj;
+ ObjectParser objectParser;
+ ObjectType type;
+
+ switch (omRequest.getCmdType()) {
+ case AddAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getAddAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case RemoveAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getRemoveAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case SetAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getSetAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case DeleteBucket:
+ volumeName = omRequest.getDeleteBucketRequest().getVolumeName();
+ bucketName = omRequest.getDeleteBucketRequest().getBucketName();
+ break;
+ case CreateDirectory:
+ keyArgs = omRequest.getCreateDirectoryRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CreateFile:
+ keyArgs = omRequest.getCreateFileRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CreateKey:
+ keyArgs = omRequest.getCreateKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case AllocateBlock:
+ keyArgs = omRequest.getAllocateBlockRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CommitKey:
+ keyArgs = omRequest.getCommitKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case DeleteKey:
+ keyArgs = omRequest.getDeleteKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case DeleteKeys:
+ OzoneManagerProtocolProtos.DeleteKeyArgs deleteKeyArgs =
+ omRequest.getDeleteKeysRequest()
+ .getDeleteKeys();
+ volumeName = deleteKeyArgs.getVolumeName();
+ bucketName = deleteKeyArgs.getBucketName();
+ break;
+ case RenameKey:
+ keyArgs = omRequest.getRenameKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case RenameKeys:
+ OzoneManagerProtocolProtos.RenameKeysArgs renameKeysArgs =
+ omRequest.getRenameKeysRequest().getRenameKeysArgs();
+ volumeName = renameKeysArgs.getVolumeName();
+ bucketName = renameKeysArgs.getBucketName();
+ break;
+ case InitiateMultiPartUpload:
+ keyArgs = omRequest.getInitiateMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CommitMultiPartUpload:
+ keyArgs = omRequest.getCommitMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case AbortMultiPartUpload:
+ keyArgs = omRequest.getAbortMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CompleteMultiPartUpload:
+ keyArgs = omRequest.getCompleteMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ default:
+ // do nothing in case of other requests.
+ break;
+ }
+
+ // Check if there is any bucket associated with this request.
+ if (bucketName.equals("") && volumeName.equals("")) {
+ return omRequest;
+ }
+
+ long bucketId;
+ try {
+ // Note: Even though this block of code is not executing under a bucket
+ // lock - it is still safe.
+ // For instance, consider the following link bucket chain:
+ // l1 -> l2 -> l3 -> abc (ID: 1000)
+ // Let's say we fetch the resolved bucket name for l1. This would mean
+ // the source bucket is resolved as 'abc'.
+ // Now, let's assume that before the next line of code (to fetch bucket
+ // ID) executes, the link structure changes as follows:
+ // l1- > l2 -> l3 -> xyz (ID: 1001)
+ // And we end up associating the bucket ID for l1 with abc (1000) even
+ // though the actual link has changed.
+ // This is not a problem, since we will anyway be validating this bucket
+ // ID inside validateAndUpdateCache method - and it will be caught there.
+ // This is a fail-slow approach.
+ ResolvedBucket resolvedBucket =
+ ozoneManager.resolveBucketLink(Pair.of(volumeName, bucketName));
+ bucketId = metadataManager.getBucketId(resolvedBucket.realVolume(),
+ resolvedBucket.realBucket());
+ } catch (OMException oe) {
+ // Ignore exceptions at this stage, let respective classes handle them.
Review Comment:
Please add debug message with exception details.
--
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
[GitHub] [ozone] github-actions[bot] closed pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
URL: https://github.com/apache/ozone/pull/3508
--
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
[GitHub] [ozone] JyotinderSingh commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r909239237
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -487,7 +487,13 @@ private OMRequest associateBucketIdWithRequest(OMRequest omRequest)
return omRequest;
}
- long bucketId = metadataManager.getBucketId(volumeName, bucketName);
+ long bucketId;
+ try {
+ bucketId = metadataManager.getBucketId(volumeName, bucketName);
+ } catch (OMException oe) {
+ // Ignore exceptions at this stage, let respective classes handle them.
+ return omRequest;
+ }
Review Comment:
Hi @rakeshadr, could you please take a look at this added logic inside the new method?
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r942333554
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequestUtils.java:
##########
@@ -47,4 +53,50 @@ public static void checkClientRequestPrecondition(
OMException.ResultCodes.INTERNAL_ERROR);
}
}
+
+ /**
+ * Validates the bucket associated with the request - to make sure it did
+ * not change since the request started processing.
+ *
+ * @param bucketId - bucket ID of the associated bucket when the request
+ * is being processed.
+ * @param omRequest - request to be validated, contains the bucket ID of the
+ * associated bucket when the request was created.
+ * @throws OMException
+ */
+ public static void validateAssociatedBucketId(long bucketId,
+ OMRequest omRequest)
+ throws OMException {
+ if (omRequest.hasAssociatedBucketId()) {
+ if (bucketId != omRequest.getAssociatedBucketId()) {
+ throw new OMException(
+ "Bucket ID mismatch. Associated bucket was modified while this" +
Review Comment:
Please include request type in the err message and modify the message like below:
`Bucket ID mismatch. Associated bucket was modified concurrently while " + omRequest..getCmdType() + " request was being processed. Please retry the request.`
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r942345555
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -324,6 +345,181 @@ private OMResponse createErrorResponse(
return omResponse.build();
}
+ /**
+ * Associate bucket ID with requests that have an associated bucket.
+ * Returns a new OMRequest object with the bucket ID associated.
+ *
+ * @param omRequest OMRequest
+ * @return OMRequest with bucket ID associated.
+ * @throws IOException
+ */
+ @SuppressWarnings("checkstyle:MethodLength")
+ private OMRequest associateBucketIdWithRequest(OMRequest omRequest)
+ throws IOException {
+ OMMetadataManager metadataManager = ozoneManager.getMetadataManager();
+ String volumeName = "";
+ String bucketName = "";
+
+ OzoneManagerProtocolProtos.KeyArgs keyArgs;
+ OzoneObj obj;
+ ObjectParser objectParser;
+ ObjectType type;
+
+ switch (omRequest.getCmdType()) {
+ case AddAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getAddAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case RemoveAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getRemoveAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case SetAcl:
+ type = omRequest.getAddAclRequest().getObj().getResType();
+ // No need for bucket ID validation in case of volume ACL
+ if (ObjectType.VOLUME == type) {
+ break;
+ }
+
+ obj =
+ OzoneObjInfo.fromProtobuf(omRequest.getSetAclRequest().getObj());
+ objectParser = new ObjectParser(obj.getPath(), type);
+
+ volumeName = objectParser.getVolume();
+ bucketName = objectParser.getBucket();
+ break;
+ case DeleteBucket:
+ volumeName = omRequest.getDeleteBucketRequest().getVolumeName();
+ bucketName = omRequest.getDeleteBucketRequest().getBucketName();
+ break;
+ case CreateDirectory:
+ keyArgs = omRequest.getCreateDirectoryRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CreateFile:
+ keyArgs = omRequest.getCreateFileRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CreateKey:
+ keyArgs = omRequest.getCreateKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case AllocateBlock:
+ keyArgs = omRequest.getAllocateBlockRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CommitKey:
+ keyArgs = omRequest.getCommitKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case DeleteKey:
+ keyArgs = omRequest.getDeleteKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case DeleteKeys:
+ OzoneManagerProtocolProtos.DeleteKeyArgs deleteKeyArgs =
+ omRequest.getDeleteKeysRequest()
+ .getDeleteKeys();
+ volumeName = deleteKeyArgs.getVolumeName();
+ bucketName = deleteKeyArgs.getBucketName();
+ break;
+ case RenameKey:
+ keyArgs = omRequest.getRenameKeyRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case RenameKeys:
+ OzoneManagerProtocolProtos.RenameKeysArgs renameKeysArgs =
+ omRequest.getRenameKeysRequest().getRenameKeysArgs();
+ volumeName = renameKeysArgs.getVolumeName();
+ bucketName = renameKeysArgs.getBucketName();
+ break;
+ case InitiateMultiPartUpload:
+ keyArgs = omRequest.getInitiateMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CommitMultiPartUpload:
+ keyArgs = omRequest.getCommitMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case AbortMultiPartUpload:
+ keyArgs = omRequest.getAbortMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ case CompleteMultiPartUpload:
+ keyArgs = omRequest.getCompleteMultiPartUploadRequest().getKeyArgs();
+ volumeName = keyArgs.getVolumeName();
+ bucketName = keyArgs.getBucketName();
+ break;
+ default:
+ // do nothing in case of other requests.
Review Comment:
Please add a debug log message about the uncovered or unknown command type, here we will not associate the bucketIds
--
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
[GitHub] [ozone] JyotinderSingh commented on pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by "JyotinderSingh (via GitHub)" <gi...@apache.org>.
JyotinderSingh commented on PR #3508:
URL: https://github.com/apache/ozone/pull/3508#issuecomment-1408756433
Hi @swamirishi, I'm afraid I no longer have bandwidth to work on this PR.
--
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
[GitHub] [ozone] adoroszlai commented on pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on PR #3508:
URL: https://github.com/apache/ozone/pull/3508#issuecomment-1398486663
/pending Why don't we just validate the bucket layout in `validateAndUpdateCache`?
--
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
[GitHub] [ozone] JyotinderSingh commented on pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3508:
URL: https://github.com/apache/ozone/pull/3508#issuecomment-1180571724
@rakeshadr could you please review this patch?
--
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
[GitHub] [ozone] rakeshadr commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
rakeshadr commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r896474886
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java:
##########
@@ -182,6 +183,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ String bucketKey = omMetadataManager.getBucketKey(volumeName, bucketName);
+
+ final long bucketId =
+ omMetadataManager.getBucketId(volumeName, bucketName);
+
+ // Bucket ID verification for in-flight requests.
+ validateAssociatedBucketId(bucketId, getOmRequest());
Review Comment:
@JyotinderSingh Can we move this to [OMKeyRequest#validateBucketAndVolume()](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java#L188), which already gets the bucket.
--
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
[GitHub] [ozone] errose28 commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r904239925
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -135,7 +141,9 @@ public OMResponse submitRequest(RpcController controller,
OMRequest request) throws ServiceException {
OMRequest validatedRequest;
try {
- validatedRequest = requestValidations.validateRequest(request);
+ OMRequest requestWithBucketId = associateBucketIdWithRequest(request);
Review Comment:
> but the issue is that we don't have validators for all kinds of requests.
Ah ok this jogged my memory. This type of race actually causes two different issues:
1. An old client could mistakenly reach an FSO bucket because the validator read stale bucket layout info when admitting the request.
2. The type Ratis request (FSO or OBS specific) could mismatch the actual type of the bucket when the request is applied.
For issue 1, the requests will have request validators blocking client requests. In these validators we can add the bucket ID to the request.
For issue 2, the requests must go through the `BucketLayoutAwareOMKeyRequestFactory`. In this class's `createRequest` method, we could add the bucket ID to the request before we call `getBucketLayout`.
Does this seem like a workable solution?
--
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
[GitHub] [ozone] swamirishi commented on pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #3508:
URL: https://github.com/apache/ozone/pull/3508#issuecomment-1407754922
@JyotinderSingh Are you working on this PR?
--
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
[GitHub] [ozone] JyotinderSingh commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r897569828
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java:
##########
@@ -182,6 +183,14 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+ String bucketKey = omMetadataManager.getBucketKey(volumeName, bucketName);
+
+ final long bucketId =
+ omMetadataManager.getBucketId(volumeName, bucketName);
+
+ // Bucket ID verification for in-flight requests.
+ validateAssociatedBucketId(bucketId, getOmRequest());
Review Comment:
Thanks for the suggestion @rakeshadr. I have addressed this in my latest commit.
--
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
[GitHub] [ozone] errose28 commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r903139108
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -135,7 +141,9 @@ public OMResponse submitRequest(RpcController controller,
OMRequest request) throws ServiceException {
OMRequest validatedRequest;
try {
- validatedRequest = requestValidations.validateRequest(request);
+ OMRequest requestWithBucketId = associateBucketIdWithRequest(request);
Review Comment:
Can we add the bucket ID to the request in the preprocess validator for each request? It may make this particular PR larger, but since every request pre-processor is already there in the code, it will be less invasive overall. The purpose of the request validators is to remove large switch blocks like what is in `associateBucketIdWithRequest` so we can group logic on a per-request basis.
--
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
[GitHub] [ozone] JyotinderSingh commented on a diff in pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on code in PR #3508:
URL: https://github.com/apache/ozone/pull/3508#discussion_r903315245
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -135,7 +141,9 @@ public OMResponse submitRequest(RpcController controller,
OMRequest request) throws ServiceException {
OMRequest validatedRequest;
try {
- validatedRequest = requestValidations.validateRequest(request);
+ OMRequest requestWithBucketId = associateBucketIdWithRequest(request);
Review Comment:
We had considered adding the bucket ID in the pre-process validators, but the issue is that we don't have validators for all kinds of requests. We'll end up validating bucket IDs only for requests that have upgrade validations in place.
But I am with you on the idea of reducing the switch-case-like code `associateBucketIdWithRequest`. We're also debating on a way to make sure that when new request classes are added that we raise some test errors if the person forgets to add the related logic to validate the bucket ID.
--
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
[GitHub] [ozone] JyotinderSingh commented on pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by GitBox <gi...@apache.org>.
JyotinderSingh commented on PR #3508:
URL: https://github.com/apache/ozone/pull/3508#issuecomment-1173111990
@rakeshadr Could you please review the changes I've made to this patch.
The number of files we are touching is high since there were a lot of unit test fixes.
There is also a change in the method signature for `OMKeyRequest.validateBucketAndVolume` since we now need to send an `OzoneManager` instance instead of `OMMetadataManager` instance because we need access to `OzoneManager.resolveBucketLink` method.
Most of these code changes are simple, however.
--
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
[GitHub] [ozone] github-actions[bot] commented on pull request #3508: HDDS-6682. Validate Bucket ID of bucket associated with in-flight requests.
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3508:
URL: https://github.com/apache/ozone/pull/3508#issuecomment-1437692803
Thank you very much for the patch. I am closing this PR __temporarily__ as there was no activity recently and it is waiting for response from its author.
It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.
It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.
If you need ANY help to finish this PR, please [contact the community](https://github.com/apache/hadoop-ozone#contact) on the mailing list or the slack channel."
--
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