bharatviswa504 commented on a change in pull request #1104:
URL: https://github.com/apache/hadoop-ozone/pull/1104#discussion_r454552913
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
##########
@@ -88,21 +88,20 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
DeleteKeyRequest deleteKeyRequest = getOmRequest().getDeleteKeyRequest();
- OzoneManagerProtocolProtos.KeyArgs deleteKeyArgs =
+ OzoneManagerProtocolProtos.KeyArgs keyArgs =
deleteKeyRequest.getKeyArgs();
+ Map<String, String> auditMap = buildKeyArgsAuditMap(keyArgs);
- String volumeName = deleteKeyArgs.getVolumeName();
- String bucketName = deleteKeyArgs.getBucketName();
- String keyName = deleteKeyArgs.getKeyName();
+ String volumeName = keyArgs.getVolumeName();
Review comment:
The same comment applies for all requests
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3314,4 +3396,59 @@ private void startJVMPauseMonitor() {
jvmPauseMonitor.init(configuration);
jvmPauseMonitor.start();
}
+
+ public ResolvedBucket resolveBucketLink(KeyArgs args) throws IOException {
+ return resolveBucketLink(
+ Pair.of(args.getVolumeName(), args.getBucketName()));
+ }
+
+ public ResolvedBucket resolveBucketLink(OmKeyArgs args)
+ throws IOException {
+ return resolveBucketLink(
+ Pair.of(args.getVolumeName(), args.getBucketName()));
+ }
+
+ public ResolvedBucket resolveBucketLink(Pair<String, String> requested)
+ throws IOException {
+ Pair<String, String> resolved =
+ resolveBucketLink(requested, new HashSet<>());
+ return new ResolvedBucket(requested, resolved);
+ }
+
+ /**
+ * Resolves bucket symlinks. Read permission is required for following links.
+ *
+ * @param volumeAndBucket the bucket to be resolved (if it is a link)
+ * @param visited collects link buckets visited during the resolution to
+ * avoid infinite loops
+ * @return bucket location possibly updated with its actual volume and bucket
+ * after following bucket links
+ * @throws IOException (most likely OMException) if ACL check fails, bucket
is
+ * not found, loop is detected in the links, etc.
+ */
+ private Pair<String, String> resolveBucketLink(
+ Pair<String, String> volumeAndBucket,
+ Set<Pair<String, String>> visited) throws IOException {
+
+ String volumeName = volumeAndBucket.getLeft();
+ String bucketName = volumeAndBucket.getRight();
+ OmBucketInfo info = bucketManager.getBucketInfo(volumeName, bucketName);
+ if (!info.isLink()) {
+ return volumeAndBucket;
+ }
+
+ if (!visited.add(volumeAndBucket)) {
+ throw new OMException("Detected loop in bucket links", INTERNAL_ERROR);
Review comment:
Can we change the error code here, the reason for this is in HA when
error code is INTERNAL_ERROR we terminate OM. (This is done to avoid DB
divergence)
##########
File path: hadoop-ozone/dist/src/main/compose/ozone/test.sh
##########
@@ -26,24 +26,24 @@ source "$COMPOSE_DIR/../testlib.sh"
start_docker_env
-#Due to the limitation of the current auditparser test, it should be the
-#first test in a clean cluster.
-
-#Disabling for now, audit parser tool during parse getting exception.
-#execute_robot_test om auditparser
-
execute_robot_test scm lib
+execute_robot_test scm ozone-lib
execute_robot_test scm basic
execute_robot_test scm gdpr
-execute_robot_test scm -v SCHEME:ofs ozonefs/ozonefs.robot
-execute_robot_test scm -v SCHEME:o3fs ozonefs/ozonefs.robot
+for scheme in ofs o3fs; do
+ for bucket in link bucket; do
+ execute_robot_test scm -v SCHEME:${scheme} -v BUCKET_TYPE:${bucket}
ozonefs/ozonefs.robot
+ done
+done
execute_robot_test scm security/ozone-secure-token.robot
-execute_robot_test scm s3
+for bucket in link generated; do
+ execute_robot_test scm -v BUCKET:${bucket} s3
Review comment:
S3 tests not using this bucket right?
For example
Delete existing bucket
# Bucket already is created in Test Setup.
${bucket} = Create bucket
Execute AWSS3APICli delete-bucket --bucket ${bucket}
And bucket is returned from
Delete existing bucket
# Bucket already is created in Test Setup.
${bucket} = Create bucket
Execute AWSS3APICli delete-bucket --bucket ${bucket}
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
##########
@@ -88,21 +88,20 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager,
long trxnLogIndex, OzoneManagerDoubleBufferHelper omDoubleBufferHelper) {
DeleteKeyRequest deleteKeyRequest = getOmRequest().getDeleteKeyRequest();
- OzoneManagerProtocolProtos.KeyArgs deleteKeyArgs =
+ OzoneManagerProtocolProtos.KeyArgs keyArgs =
deleteKeyRequest.getKeyArgs();
+ Map<String, String> auditMap = buildKeyArgsAuditMap(keyArgs);
- String volumeName = deleteKeyArgs.getVolumeName();
- String bucketName = deleteKeyArgs.getBucketName();
- String keyName = deleteKeyArgs.getKeyName();
+ String volumeName = keyArgs.getVolumeName();
Review comment:
Minor: We can skip getting from original Args, as anyway final
Volume/Bucket we get is from resolvedBucket returned keyArgs.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java
##########
@@ -111,6 +110,10 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
OMClientResponse omClientResponse = null;
Result result = null;
try {
+ keyArgs = resolveBucketLink(ozoneManager, keyArgs, auditMap);
Review comment:
Now with this approach, we validate bucket exist twice. Once is
resolvedBucketLink, and again in ValidateVolumeAndBucket. If this is link, can
we skip 2nd-time check?
(Now bucket/volume is in the full cache, but I think it will be better to
avoid still)
Might be in non-HA, this might create a problem as we release lock and then
re-acquire some other thread might delete the bucket. But once HA becomes the
default, we can optimize this, as in HA there is only a single thread executor.
If you also think the same open the Jira for improvement
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3InitiateMultipartUploadRequest.java
##########
@@ -114,12 +117,15 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
getOmRequest());
OMClientResponse omClientResponse = null;
try {
+ keyArgs = resolveBucketLink(ozoneManager, keyArgs, auditMap);
+
// TODO to support S3 ACL later.
acquiredBucketLock =
- omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK, volumeName,
- bucketName);
+ omMetadataManager.getLock().acquireWriteLock(BUCKET_LOCK,
Review comment:
Minor: Can we assign to volumeName and bucketName, similar to Key
requests and use that, instead of getting multiple times.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java
##########
@@ -85,10 +87,11 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager,
OMMetrics omMetrics = ozoneManager.getMetrics();
omMetrics.incNumKeyDeletes();
- Map<String, String> auditMap = null;
String volumeName = deleteKeyArgs.getVolumeName();
String bucketName = deleteKeyArgs.getBucketName();
- String keyName = "";
+ Map<String, String> auditMap = new LinkedHashMap<>();
+ auditMap.put(VOLUME, volumeName);
Review comment:
Minor: We can skip this, as anyway bucket.audit has already taken care
of this.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadAbortRequest.java
##########
@@ -152,27 +158,29 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
omDoubleBufferHelper);
if (acquiredLock) {
- omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK, volumeName,
- bucketName);
+ omMetadataManager.getLock().releaseWriteLock(BUCKET_LOCK,
+ keyArgs.getVolumeName(), keyArgs.getBucketName());
}
}
// audit log
auditLog(ozoneManager.getAuditLogger(), buildAuditMessage(
- OMAction.ABORT_MULTIPART_UPLOAD, buildKeyArgsAuditMap(keyArgs),
+ OMAction.ABORT_MULTIPART_UPLOAD, auditMap,
exception, getOmRequest().getUserInfo()));
switch (result) {
case SUCCESS:
LOG.debug("Abort Multipart request is successfully completed for " +
- "KeyName {} in VolumeName/Bucket {}/{}", keyName, volumeName,
- bucketName);
+ "KeyName {} in VolumeName/Bucket {}/{}", keyName,
requestedVolume,
Review comment:
I see for all key requests we logged resolvedVolume/resolvedBucket. For
MPU we log requested volume/bucket any reason for this?. Can we follow one
approach for all requests?
##########
File path: hadoop-ozone/dist/src/main/smoketest/basic/links.robot
##########
@@ -0,0 +1,152 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+*** Settings ***
+Documentation Test bucket links via Ozone CLI
+Library OperatingSystem
+Resource ../commonlib.robot
+Resource ../ozone-lib/shell.robot
+Test Setup Run Keyword if '${SECURITY_ENABLED}' == 'true' Kinit
test user testuser testuser.keytab
+Test Timeout 2 minute
+Suite Setup Create volumes
+
+*** Variables ***
+${prefix} generated
+
+*** Keywords ***
+Create volumes
+ ${random} = Generate Random String 5 [NUMBERS]
+ Set Suite Variable ${source} ${random}-source
+ Set Suite Variable ${target} ${random}-target
+ Execute ozone sh volume create ${source}
+ Execute ozone sh volume create ${target}
+ Run Keyword if '${SECURITY_ENABLED}' == 'true' Setup ACL tests
+
+Setup ACL tests
+ Execute ozone sh bucket create ${source}/readable-bucket
+ Execute ozone sh key put
${source}/readable-bucket/key-in-readable-bucket /etc/passwd
+ Execute ozone sh bucket create ${source}/unreadable-bucket
+ Execute ozone sh bucket link ${source}/readable-bucket
${target}/readable-link
+ Execute ozone sh bucket link ${source}/readable-bucket
${target}/unreadable-link
+ Execute ozone sh bucket link ${source}/unreadable-bucket
${target}/link-to-unreadable-bucket
+ Execute ozone sh volume addacl --acl
user:testuser2/s...@example.com:r ${target}
+ Execute ozone sh volume addacl --acl
user:testuser2/s...@example.com:rl ${source}
+ Execute ozone sh bucket addacl --acl
user:testuser2/s...@example.com:rl ${source}/readable-bucket
+ Execute ozone sh bucket addacl --acl
user:testuser2/s...@example.com:r ${target}/readable-link
+ Execute ozone sh bucket addacl --acl
user:testuser2/s...@example.com:r ${target}/link-to-unreadable-bucket
+
+Can follow link with read access
+ Execute kdestroy
+ Run Keyword Kinit test user testuser2
testuser2.keytab
+ ${result} = Execute And Ignore Error ozone sh key list
${target}/readable-link
+ Should Contain ${result}
key-in-readable-bucket
+
+Cannot follow link without read access
+ Execute kdestroy
+ Run Keyword Kinit test user testuser2
testuser2.keytab
+ ${result} = Execute And Ignore Error ozone sh key list
${target}/unreadable-link
+ Should Contain ${result}
PERMISSION_DENIED
+
+ACL verified on source bucket
+ Execute kdestroy
+ Run Keyword Kinit test user testuser2
testuser2.keytab
+ ${result} = Execute ozone sh bucket info
${target}/link-to-unreadable-bucket
+ Should Contain ${result}
link-to-unreadable-bucket
+ Should Not Contain ${result}
PERMISSION_DENIED
+ ${result} = Execute And Ignore Error ozone sh key list
${target}/link-to-unreadable-bucket
+ Should Contain ${result}
PERMISSION_DENIED
+
+*** Test Cases ***
+Link to non-existent bucket
+ Execute ozone sh bucket link
${source}/no-such-bucket ${target}/dangling-link
+ ${result} = Execute And Ignore Error ozone sh key list
${target}/dangling-link
+ Should Contain ${result}
BUCKET_NOT_FOUND
+
+Key create passthrough
+ Execute ozone sh bucket link
${source}/bucket1 ${target}/link1
+ Execute ozone sh bucket create
${source}/bucket1
+ Execute ozone sh key put
${target}/link1/key1 /etc/passwd
+ Key Should Match Local File ${target}/link1/key1
/etc/passwd
+
+Key read passthrough
+ Execute ozone sh key put
${source}/bucket1/key2 /opt/hadoop/NOTICE.txt
+ Key Should Match Local File ${source}/bucket1/key2
/opt/hadoop/NOTICE.txt
+
+Key list passthrough
+ ${target_list} = Execute ozone sh key list
${target}/link1 | jq -r '.name'
+ ${source_list} = Execute ozone sh key list
${source}/bucket1 | jq -r '.name'
+ Should Be Equal ${target_list}
${source_list}
+ Should Contain ${source_list} key1
+ Should Contain ${source_list} key2
+
+Key delete passthrough
+ Execute ozone sh key delete
${target}/link1/key2
+ ${source_list} = Execute ozone sh key list
${source}/bucket1 | jq -r '.name'
+ Should Not Contain ${source_list} key2
+
+Bucket list contains links
+ ${result} = Execute ozone sh bucket list
${target}
+ Should Contain ${result} link1
+ Should Contain ${result}
dangling-link
+
+Bucket info shows source
+ ${result} = Execute ozone sh bucket info
${target}/link1 | jq -r '.sourceVolume, .sourceBucket' | xargs
+ Should Be Equal ${result} ${source}
bucket1
+
+Source and target have separate ACLs
+ Execute ozone sh bucket addacl --acl user:user1:rwxy ${target}/link1
+ Verify ACL bucket ${target}/link1 USER user1 READ WRITE
READ_ACL WRITE_ACL
+ Verify ACL bucket ${source}/bucket1 USER user1 ${EMPTY}
+
+ Execute ozone sh bucket addacl --acl group:group2:r ${source}/bucket1
+ Verify ACL bucket ${target}/link1 GROUP group2 ${EMPTY}
+ Verify ACL bucket ${source}/bucket1 GROUP group2 READ
+
+Buckets and links share namespace
+ Execute ozone sh bucket link
${source}/bucket2 ${target}/link2
+ ${result} = Execute And Ignore Error ozone sh bucket create
${target}/link2
+ Should Contain ${result}
BUCKET_ALREADY_EXISTS
+
+ Execute ozone sh bucket create
${target}/bucket3
+ ${result} = Execute And Ignore Error ozone sh bucket link
${source}/bucket1 ${target}/bucket3
+ Should Contain ${result}
BUCKET_ALREADY_EXISTS
+
+Can follow link with read access
+ Run Keyword if '${SECURITY_ENABLED}' == 'true' Can follow link with
read access
+
+Cannot follow link without read access
+ Run Keyword if '${SECURITY_ENABLED}' == 'true' Cannot follow link
without read access
+
+ACL verified on source bucket
+ Run Keyword if '${SECURITY_ENABLED}' == 'true' ACL verified on
source bucket
+
+Loop in link chain is detected
+ Execute ozone sh bucket link
${target}/loop1 ${target}/loop2
+ Execute ozone sh bucket link
${target}/loop2 ${target}/loop3
+ Execute ozone sh bucket link
${target}/loop3 ${target}/loop1
+ ${result} = Execute And Ignore Error ozone sh key list
${target}/loop2
+ Should Contain ${result} INTERNAL_ERROR
Review comment:
This needs to be updated, once error code is modified.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
