mehakmeet commented on a change in pull request #3260:
URL: https://github.com/apache/hadoop/pull/3260#discussion_r684101247
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/auth/ITestCustomSigner.java
##########
@@ -214,6 +220,31 @@ public void sign(SignableRequest<?> request,
AWSCredentials credentials) {
}
}
+ private String parseBucketFromHost(String host) {
+ // host: {bucket || accesspoint}.{s3 ||
s3-accesspoint}.{region}.amazonaws.com
+ String[] hostBits = host.split("\\.");
+ String bucketName = hostBits[0];
+ String service = hostBits[1];
+
+ if (service.contains("s3-accesspoint") ||
service.contains("s3-outposts") ||
+ service.contains("s3-object-lambda")) {
+ // If AccessPoint then bucketName is of format `accessPoint-accountId`;
+ String[] accessPointBits = hostBits[0].split("\\-");
Review comment:
what if AccessPoint Name have a "-" in it?
for eg: AP name = "mmt-ap", then `bucketName = "mmt-ap-ACCOUNT_ID"`, with
this split, we would end up with "mmt" as `accessPointName` and "ap" as
`accountID`.
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/AbstractS3ATestBase.java
##########
@@ -41,6 +41,7 @@
import static org.apache.hadoop.fs.contract.ContractTestUtils.dataset;
import static org.apache.hadoop.fs.contract.ContractTestUtils.writeDataset;
+import static org.apache.hadoop.fs.s3a.Constants.ACCESS_POINT_ARN;
Review comment:
nit: unused import.
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEKMSUserDefinedKey.java
##########
@@ -25,6 +25,8 @@
import static
org.apache.hadoop.fs.s3a.Constants.SERVER_SIDE_ENCRYPTION_ALGORITHM;
Review comment:
nit: after refactoring, these would be becoming unused imports.
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AEncryptionSSEKMSUserDefinedKey.java
##########
@@ -39,12 +41,10 @@ protected Configuration createConfiguration() {
// get the KMS key for this test.
Configuration c = new Configuration();
String kmsKey = c.get(SERVER_SIDE_ENCRYPTION_KEY);
- if (StringUtils.isBlank(kmsKey) || !c.get(SERVER_SIDE_ENCRYPTION_ALGORITHM)
- .equals(S3AEncryptionMethods.CSE_KMS.name())) {
- skip(SERVER_SIDE_ENCRYPTION_KEY + " is not set for " +
- SSE_KMS.getMethod() + " or CSE-KMS algorithm is used instead of "
- + "SSE-KMS");
- }
+
+ skipIfKmsKeyIdIsNotSet(c);
+ skipIfCSEIsNotEnabled(c);
Review comment:
Seems like my code had a bug in it. This should be skipping if CSE is
enabled or if KMS key is not set.
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AContractGetFileStatusV1List.java
##########
@@ -18,11 +18,17 @@
package org.apache.hadoop.fs.s3a;
+import java.io.IOException;
+
+import org.junit.Assume;
+import org.junit.Test;
Review comment:
nit: unused imports.
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/ArnResource.java
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.
+ */
+
+package org.apache.hadoop.fs.s3a;
+
+import javax.annotation.Nonnull;
+
+import com.amazonaws.arn.Arn;
+import com.amazonaws.regions.RegionUtils;
+
+/**
+ * Represents an Arn Resource, this can be an accesspoint or bucket.
+ */
+public class ArnResource {
+
+ /**
+ * Resource name.
+ */
+ private final String name;
+
+ /**
+ * Resource owner account id.
+ */
+ private final String ownerAccountId;
+
+ /**
+ * Resource region.
+ */
+ private final String region;
+
+ /**
+ * Full Arn for the resource.
+ */
+ private final String fullArn;
+
+ /**
+ * Partition for the resource. Allowed partitions: aws, aws-cn, aws-us-gov
+ */
+ private final String partition;
+
+ /**
+ * Because of the different ways an endpoint can be constructed depending on
partition we're
+ * relying on the AWS SDK to produce the endpoint. In this case we need a
region key of the form
+ * {@code String.format("accesspoint-%s", awsRegion)}
+ */
+ private final String accessPointRegionKey;
+
+ private ArnResource(String name, String owner, String region, String
partition, String fullArn) {
+ this.name = name;
+ this.ownerAccountId = owner;
+ this.region = region;
+ this.partition = partition;
+ this.fullArn = fullArn;
+ this.accessPointRegionKey = String.format("accesspoint-%s", region);
+ }
+
+ /**
+ * Resource name.
+ * @return resource name.
+ */
+ public String getName() {
+ return name;
+ }
+
+ /**
+ * Return owner's account id.
+ */
+ public String getOwnerAccountId() {
+ return ownerAccountId;
+ }
+
+ /**
+ * Resource region.
+ * @return resource region.
+ */
+ public String getRegion() {
+ return region;
+ }
+
+ /**
+ * Full arn for resource.
+ * @return arn for resource.
+ */
+ public String getFullArn() {
+ return fullArn;
+ }
+
+ /**
+ * Formatted endpoint for the resource.
+ * @return resource endpoint.
+ */
+ public String getEndpoint() {
+ return RegionUtils.getRegion(accessPointRegionKey)
+ .getServiceEndpoint("s3");
+ }
+
+ /**
+ * Parses the passed `arn` string into a full ArnResource
Review comment:
nit: end with "."
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/S3ATestUtils.java
##########
@@ -1507,4 +1507,13 @@ public static void skipIfKmsKeyIdIsNotSet(Configuration
configuration) {
}
}
+ /**
+ * Skip if a test doesn't use CSE.
+ */
+ public static void skipIfCSEIsNotEnabled(Configuration configuration) {
+ String encryption =
configuration.get(Constants.SERVER_SIDE_ENCRYPTION_ALGORITHM, "");
+ if (!encryption.equals(S3AEncryptionMethods.CSE_KMS.getMethod())) {
Review comment:
change this to skip if CSE is enabled.
##########
File path:
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/ArnResource.java
##########
@@ -0,0 +1,132 @@
+/*
+ * 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.
+ */
+
+package org.apache.hadoop.fs.s3a;
+
+import javax.annotation.Nonnull;
+
+import com.amazonaws.arn.Arn;
+import com.amazonaws.regions.RegionUtils;
+
+/**
+ * Represents an Arn Resource, this can be an accesspoint or bucket.
+ */
+public class ArnResource {
+
+ /**
+ * Resource name.
+ */
+ private final String name;
+
+ /**
+ * Resource owner account id.
+ */
+ private final String ownerAccountId;
+
+ /**
+ * Resource region.
+ */
+ private final String region;
+
+ /**
+ * Full Arn for the resource.
+ */
+ private final String fullArn;
+
+ /**
+ * Partition for the resource. Allowed partitions: aws, aws-cn, aws-us-gov
+ */
+ private final String partition;
+
+ /**
+ * Because of the different ways an endpoint can be constructed depending on
partition we're
+ * relying on the AWS SDK to produce the endpoint. In this case we need a
region key of the form
+ * {@code String.format("accesspoint-%s", awsRegion)}
+ */
+ private final String accessPointRegionKey;
+
+ private ArnResource(String name, String owner, String region, String
partition, String fullArn) {
+ this.name = name;
+ this.ownerAccountId = owner;
+ this.region = region;
+ this.partition = partition;
+ this.fullArn = fullArn;
+ this.accessPointRegionKey = String.format("accesspoint-%s", region);
+ }
+
+ /**
+ * Resource name.
+ * @return resource name.
+ */
+ public String getName() {
+ return name;
+ }
+
+ /**
+ * Return owner's account id.
Review comment:
add `@return` to the javadoc.
##########
File path:
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AConfiguration.java
##########
@@ -32,6 +32,7 @@
import org.apache.hadoop.fs.s3native.S3xLoginHelper;
import org.apache.hadoop.test.GenericTestUtils;
import org.junit.Assert;
+import org.junit.Assume;
Review comment:
nit: unused import.
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
