adoroszlai commented on code in PR #6756:
URL: https://github.com/apache/ozone/pull/6756#discussion_r1828937527
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java:
##########
@@ -1381,6 +1412,75 @@ public static boolean checkCopySourceModificationTime(
(lastModificationTime <= copySourceIfUnmodifiedSince);
}
+ private Response putObjectTagging(OzoneVolume volume, String bucketName,
String keyName, InputStream body)
+ throws IOException, OS3Exception {
+ long startNanos = Time.monotonicNowNanos();
+ S3Tagging tagging = null;
+ try {
+ tagging = new PutTaggingUnmarshaller().readFrom(body);
+ tagging.validate();
+ } catch (WebApplicationException ex) {
+ OS3Exception exception =
S3ErrorTable.newError(S3ErrorTable.MALFORMED_XML, keyName);
+ exception.setErrorMessage(exception.getErrorMessage() + ". " +
ex.getMessage());
+ throw exception;
+ }
+
+ Map<String, String> tags = validateAndGetTagging(
+ tagging.getTagSet().getTags(), // Nullity check was done in previous
parsing step
+ Tag::getKey,
+ Tag::getValue
+ );
+
+ try {
+ volume.getBucket(bucketName).putObjectTagging(keyName, tags);
+ } catch (OMException ex) {
+ if (ex.getResult() == ResultCodes.INVALID_REQUEST) {
+ throw S3ErrorTable.newError(S3ErrorTable.INVALID_REQUEST, bucketName);
+ } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
+ throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucketName);
+ } else if (ex.getResult() == ResultCodes.KEY_NOT_FOUND) {
+ throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_KEY, keyName);
+ } else if (ex.getResult() == ResultCodes.NOT_SUPPORTED_OPERATION) {
+ // When putObjectTagging operation is applied on FSO directory
+ throw S3ErrorTable.newError(S3ErrorTable.NOT_IMPLEMENTED, keyName);
+ }
Review Comment:
Why do some of these include `bucketName` in the error, while others
`keyName`?
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java:
##########
@@ -1145,11 +1145,37 @@ void setTimes(OmKeyArgs keyArgs, long mtime, long atime)
boolean setSafeMode(SafeModeAction action, boolean isChecked)
throws IOException;
+ /**
+ * Gets the tags for the specified key.
+ * @param args Key args
+ * @return Tags associated with the key.
+ */
+ Map<String, String> getObjectTagging(OmKeyArgs args) throws IOException;
+
+ /**
+ * Sets the tags to an existing key.
+ * @param args Key args
+ */
+ default void putObjectTagging(OmKeyArgs args) throws IOException {
+ throw new UnsupportedOperationException("OzoneManager does not require " +
+ "this to be implemented, as write requests use a new approach.");
+ }
+
+ /**
+ * Removes all the tags from the specified key.
+ * @param args Key args
+ */
+ default void deleteObjectTagging(OmKeyArgs args) throws IOException {
+ throw new UnsupportedOperationException("OzoneManager does not require " +
+ "this to be implemented, as write requests use a new approach.");
+ }
+
/**
* Get status of last triggered quota repair in OM.
* @return String
* @throws IOException
*/
+
String getQuotaRepairStatus() throws IOException;
Review Comment:
nit: I guess this new line is unintentional
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -4767,6 +4767,18 @@ public void startQuotaRepair(List<String> buckets)
throws IOException {
new QuotaRepairTask(this).repair(buckets);
}
+ /**
+ * {@inheritDoc}
+ */
Review Comment:
nit: `{@inheritDoc}` is needed only when adding extra doc to the parent's;
otherwise doc is inherited automatically.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -2727,6 +2727,61 @@ public void recoverKey(OmKeyArgs args, long clientID)
throws IOException {
ozoneManagerClient.recoverKey(args, clientID);
}
+ @Override
+ public Map<String, String> getObjectTagging(String volumeName, String
bucketName, String keyName)
+ throws IOException {
+ if (omVersion.compareTo(OzoneManagerVersion.OBJECT_TAG) < 0) {
+ throw new IOException("OzoneManager does not support object tags");
+ }
Review Comment:
`OzoneManagerVersion.OBJECT_TAG` was introduced in
8a239912378f8b2aa5de84d9f913821096b74fcb. Any OM built since that commit but
preceding this change does not support these methods. So request from client
to such OM would fail with `InvalidProtocolBufferException`. Failing with
`IOException("OzoneManager does not support object tags")` might be friendlier,
but we need a new `OzoneManagerVersion` for that.
CC @errose28
##########
hadoop-ozone/dist/src/main/smoketest/s3/objecttagging.robot:
##########
@@ -0,0 +1,69 @@
+# 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 S3 gateway test with aws cli
+Library OperatingSystem
+Library String
+Resource ../commonlib.robot
+Resource commonawslib.robot
+Test Timeout 5 minutes
+Suite Setup Setup s3 tests
+
+*** Variables ***
+${ENDPOINT_URL} http://s3g:9878
+${OZONE_TEST} true
+${BUCKET} generated
+
+
+*** Test Cases ***
+
+Put object tagging
Review Comment:
Would be nice to add negative test cases (e.g. request for non-existent
key). Can be done in follow-up.
Also would be nice to create reusable keywords to avoid duplication. Also
can be in follow-up.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Tagging.java:
##########
@@ -0,0 +1,156 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.
+ */
+
+package org.apache.hadoop.ozone.s3.endpoint;
+
+import javax.ws.rs.WebApplicationException;
+import javax.xml.bind.annotation.XmlAccessType;
+import javax.xml.bind.annotation.XmlAccessorType;
+import javax.xml.bind.annotation.XmlElement;
+import javax.xml.bind.annotation.XmlRootElement;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+/**
+ * S3 tagging.
+ */
+@XmlAccessorType(XmlAccessType.FIELD)
+@XmlRootElement(name = "Tagging",
+ namespace = "http://s3.amazonaws.com/doc/2006-03-01/";)
+public class S3Tagging {
+
+ @XmlElement(name = "TagSet")
+ private TagSet tagSet;
+
+ public S3Tagging() {
+
+ }
+
+ public S3Tagging(TagSet tagSet) {
+ this.tagSet = tagSet;
+ }
+
+ public TagSet getTagSet() {
+ return tagSet;
+ }
+
+ public void setTagSet(TagSet tagSet) {
+ this.tagSet = tagSet;
+ }
+
+ /**
+ * Entity for child element TagSet.
+ */
+ @XmlAccessorType(XmlAccessType.FIELD)
+ @XmlRootElement(name = "TagSet")
+ public static class TagSet {
+ @XmlElement(name = "Tag")
+ private List<Tag> tags = new ArrayList<>();
+
+ public TagSet() {
+ }
+
+ public TagSet(List<Tag> tags) {
+ this.tags = tags;
+ }
+
+ public List<Tag> getTags() {
+ return tags;
+ }
+
+ public void setTags(List<Tag> tags) {
+ this.tags = tags;
+ }
+ }
+
+ /**
+ * Entity for child element Tag.
+ */
+ @XmlAccessorType(XmlAccessType.FIELD)
+ @XmlRootElement(name = "Tag")
+ public static class Tag {
+ @XmlElement(name = "Key")
+ private String key;
+
+ @XmlElement(name = "Value")
+ private String value;
+
+ public Tag() {
+ }
+
+ public Tag(String key, String value) {
+ this.key = key;
+ this.value = value;
+ }
+
+ public String getKey() {
+ return key;
+ }
+
+ public void setKey(String key) {
+ this.key = key;
+ }
+
+ public String getValue() {
+ return value;
+ }
+
+ public void setValue(String value) {
+ this.value = value;
+ }
+ }
+
+ /**
+ * Creates a S3 tagging instance (xml representation) from a Map retrieved
+ * from OM.
+ * @param tagMap Map representing the tags.
+ * @return {@link S3Tagging}
+ */
+ public static S3Tagging fromMap(Map<String, String> tagMap) {
+ List<Tag> tags = tagMap.entrySet()
+ .stream()
+ .map(
+ tagEntry -> new Tag(tagEntry.getKey(), tagEntry.getValue())
+ )
+ .collect(Collectors.toList());
+ return new S3Tagging(new TagSet(tags));
+ }
+
+ /**
+ * Additional XML validation logic for S3 tagging.
+ */
+ public void validate() {
+ if (tagSet == null) {
+ throw new WebApplicationException("TagSet needs to be specified");
+ }
+
+ if (tagSet.getTags().isEmpty()) {
+ throw new WebApplicationException("Tags need to be specified and cannot
be empty");
+ }
+
+ for (Tag tag: tagSet.getTags()) {
+ if (tag.getKey() == null) {
+ throw new WebApplicationException("Some tag keys are not specified");
+ }
+ if (tag.getValue() == null) {
+ throw new WebApplicationException("Tag value for tag " + tag.getKey()
+ " is not specified");
+ }
+ }
+ }
Review Comment:
nit: I would use `WebApplicationException` if a specific HTTP response
status needs to be set, or if it is to be handled by some framework not by
application code. However, these instances are created with
`INTERNAL_SERVER_ERROR` (default value), which does not reflect that these are
thrown due to invalid request content. Also, we are catching it in
`ObjectEndpoint` and translating to `OS3Exception`. So I think we may throw
regular `IllegalStateException` or `IllegalArgumentException` here and in
`PutTaggingUnmarshaller`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tagging/S3DeleteObjectTaggingRequest.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * 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
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * 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.
+ */
+
+package org.apache.hadoop.ozone.om.request.s3.tagging;
+
+import com.google.common.base.Preconditions;
+import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
+import org.apache.hadoop.hdds.utils.db.cache.CacheValue;
+import org.apache.hadoop.ozone.audit.OMAction;
+import org.apache.hadoop.ozone.om.OMMetadataManager;
+import org.apache.hadoop.ozone.om.OMMetrics;
+import org.apache.hadoop.ozone.om.OzoneManager;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.helpers.BucketLayout;
+import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
+import org.apache.hadoop.ozone.om.request.key.OMKeyRequest;
+import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
+import org.apache.hadoop.ozone.om.response.OMClientResponse;
+import
org.apache.hadoop.ozone.om.response.s3.tagging.S3DeleteObjectTaggingResponse;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.DeleteObjectTaggingRequest;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.DeleteObjectTaggingResponse;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.KeyArgs;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+import
org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse;
+import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer.ACLType;
+import org.apache.ratis.server.protocol.TermIndex;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.Map;
+
+import static
org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.KEY_NOT_FOUND;
+import static
org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.BUCKET_LOCK;
+
+/**
+ * Handles delete object tagging request.
+ */
+public class S3DeleteObjectTaggingRequest extends OMKeyRequest {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(S3DeleteObjectTaggingRequest.class);
+
+ public S3DeleteObjectTaggingRequest(OMRequest omRequest, BucketLayout
bucketLayout) {
+ super(omRequest, bucketLayout);
+ }
+
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ DeleteObjectTaggingRequest deleteObjectTaggingRequest =
+ super.preExecute(ozoneManager).getDeleteObjectTaggingRequest();
+ Preconditions.checkNotNull(deleteObjectTaggingRequest);
+
+ KeyArgs keyArgs = deleteObjectTaggingRequest.getKeyArgs();
+
+ String keyPath = keyArgs.getKeyName();
+ keyPath = validateAndNormalizeKey(ozoneManager.getEnableFileSystemPaths(),
+ keyPath, getBucketLayout());
+
+ KeyArgs.Builder newKeyArgs =
+ keyArgs.toBuilder()
+ .setKeyName(keyPath);
+
+ KeyArgs resolvedArgs = resolveBucketAndCheckKeyAcls(newKeyArgs.build(),
+ ozoneManager, ACLType.WRITE);
+ return getOmRequest().toBuilder()
+ .setUserInfo(getUserInfo())
+ .setDeleteObjectTaggingRequest(
+ deleteObjectTaggingRequest.toBuilder().setKeyArgs(resolvedArgs))
+ .build();
+ }
+
+ @Override
+ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
TermIndex termIndex) {
+ final long trxnLogIndex = termIndex.getIndex();
+
+ DeleteObjectTaggingRequest deleteObjectTaggingRequest =
getOmRequest().getDeleteObjectTaggingRequest();
+
+ KeyArgs keyArgs = deleteObjectTaggingRequest.getKeyArgs();
+ String volumeName = keyArgs.getVolumeName();
+ String bucketName = keyArgs.getBucketName();
+ String keyName = keyArgs.getKeyName();
+
+ OMMetrics omMetrics = ozoneManager.getMetrics();
+ omMetrics.incNumDeleteObjectTagging();
+
+ Map<String, String> auditMap = buildKeyArgsAuditMap(keyArgs);
+
+ OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
+ getOmRequest());
+
+ OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ boolean acquiredLock = false;
+ OMClientResponse omClientResponse = null;
+ IOException exception = null;
+ Result result = null;
+ try {
+ mergeOmLockDetails(
+ omMetadataManager.getLock()
+ .acquireWriteLock(BUCKET_LOCK, volumeName, bucketName)
+ );
+ acquiredLock = getOmLockDetails().isLockAcquired();
+
+ validateBucketAndVolume(omMetadataManager, volumeName, bucketName);
+
+ String dbOzoneKey =
+ omMetadataManager.getOzoneKey(volumeName, bucketName, keyName);
+
+ OmKeyInfo omKeyInfo =
+ omMetadataManager.getKeyTable(getBucketLayout()).get(dbOzoneKey);
+ if (omKeyInfo == null) {
+ throw new OMException("Key not found", KEY_NOT_FOUND);
+ }
+
+ // Clear / delete the tags
+ omKeyInfo.getTags().clear();
+ // Set the UpdateID to the current transactionLogIndex
+ omKeyInfo.setUpdateID(trxnLogIndex, ozoneManager.isRatisEnabled());
+
+ // Note: Key modification time is not changed because S3 last modified
+ // time only changes when there are changes in the object content
+
+ // Update table cache
+ omMetadataManager.getKeyTable(getBucketLayout()).addCacheEntry(
+ new CacheKey<>(dbOzoneKey),
+ CacheValue.get(trxnLogIndex, omKeyInfo)
+ );
+
+ omClientResponse = new S3DeleteObjectTaggingResponse(
+
omResponse.setDeleteObjectTaggingResponse(DeleteObjectTaggingResponse.newBuilder()).build(),
+ omKeyInfo
+ );
+
+ result = Result.SUCCESS;
+ } catch (IOException ex) {
+ result = Result.FAILURE;
+ exception = ex;
+ omClientResponse = new S3DeleteObjectTaggingResponse(
+ createErrorOMResponse(omResponse, exception),
+ getBucketLayout()
+ );
+ } finally {
+ if (acquiredLock) {
+ mergeOmLockDetails(omMetadataManager.getLock()
+ .releaseWriteLock(BUCKET_LOCK, volumeName, bucketName));
+ }
+ if (omClientResponse != null) {
+ omClientResponse.setOmLockDetails(getOmLockDetails());
+ }
+ }
+
+ markForAudit(ozoneManager.getAuditLogger(), buildAuditMessage(
+ OMAction.DELETE_OBJECT_TAGGING, auditMap, exception,
getOmRequest().getUserInfo()
+ ));
+
+ switch (result) {
+ case SUCCESS:
+ LOG.debug("Delete object tagging success. Volume:{}, Bucket:{},
Key:{}.", volumeName,
+ bucketName, keyName);
+ break;
+ case FAILURE:
+ omMetrics.incNumDeleteObjectTaggingFails();
+ LOG.error("Delete object tagging failed. Volume:{}, Bucket:{}, Key:{}.",
volumeName,
+ bucketName, keyName, exception);
Review Comment:
I don't think we should log all failures at `ERROR` level. System errors
are OK, but requests can also fail due to input from user:
```
2024-11-05 11:27:58,742 [om1-OMStateMachineApplyTransactionThread - 0] ERROR
tagging.S3DeleteObjectTaggingRequest: Delete object tagging failed. Volume:s3v,
Bucket:bucket-azwyiqbquu, Key:ozone-test-2023699972/putobject/key=value/f2.
KEY_NOT_FOUND org.apache.hadoop.ozone.om.exceptions.OMException: Key not
found
at
org.apache.hadoop.ozone.om.request.s3.tagging.S3DeleteObjectTaggingRequest.validateAndUpdateCache(S3DeleteObjectTaggingRequest.java:126)
at
org.apache.hadoop.ozone.protocolPB.OzoneManagerRequestHandler.lambda$0(OzoneManagerRequestHandler.java:421)
at
org.apache.hadoop.ozone.util.MetricUtil.captureLatencyNs(MetricUtil.java:46)
at
org.apache.hadoop.ozone.protocolPB.OzoneManagerRequestHandler.handleWriteRequestImpl(OzoneManagerRequestHandler.java:419)
at
org.apache.hadoop.ozone.protocolPB.RequestHandler.handleWriteRequest(RequestHandler.java:63)
at
org.apache.hadoop.ozone.om.ratis.OzoneManagerStateMachine.runCommand(OzoneManagerStateMachine.java:551)
at
org.apache.hadoop.ozone.om.ratis.OzoneManagerStateMachine.lambda$1(OzoneManagerStateMachine.java:367)
at
java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768)
at
java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
at
java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
at java.base/java.lang.Thread.run(Thread.java:833)
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
