steveloughran commented on code in PR #6507:
URL: https://github.com/apache/hadoop/pull/6507#discussion_r1472896913
##########
hadoop-tools/hadoop-aws/pom.xml:
##########
@@ -508,6 +508,29 @@
<artifactId>bundle</artifactId>
<scope>compile</scope>
</dependency>
+ <dependency>
+ <groupId>software.amazon.s3.accessgrants</groupId>
Review Comment:
why isn't this in bundle.jar?
* if it is: it's not needed.
* If it isn't, why not?
is this new jar going to be mandatory, or optional?
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java:
##########
@@ -1600,4 +1600,20 @@ private Constants() {
*/
public static final boolean CHECKSUM_VALIDATION_DEFAULT = false;
+
+ /**
+ * Flag to enable S3 Access Grants to control authorization to S3 data. More
information:
+ * https://aws.amazon.com/s3/features/access-grants/
+ * and
+ * https://github.com/aws/aws-s3-accessgrants-plugin-java-v2/
+ */
+ public static final String AWS_S3_ACCESS_GRANTS_ENABLED =
"fs.s3a.access-grants.enabled";
+
+ /**
+ * Flag to enable jobs fall back to the Job Execution IAM role in
+ * case they get Access Denied from the S3 Access Grants call. More
information:
+ * https://github.com/aws/aws-s3-accessgrants-plugin-java-v2/
+ */
+ public static final String AWS_S3_ACCESS_GRANTS_FALLBACK_TO_IAM_ENABLED =
+ "fs.s3a.access-grants.fallback-to-iam";
Review Comment:
nit; can you use "." over "-"
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java:
##########
@@ -370,4 +375,18 @@ private static Region getS3RegionFromEndpoint(String
endpoint) {
return Region.US_EAST_1;
}
+ public static <BuilderT extends S3BaseClientBuilder<BuilderT, ClientT>,
ClientT> void
+ applyS3AccessGrantsConfigurations(BuilderT builder, Configuration conf) {
+ boolean s3agEnabled = conf.getBoolean(AWS_S3_ACCESS_GRANTS_ENABLED, false);
+ if (s3agEnabled) {
+ boolean s3agFallbackEnabled = conf.getBoolean(
+ AWS_S3_ACCESS_GRANTS_FALLBACK_TO_IAM_ENABLED, false);
+ S3AccessGrantsPlugin accessGrantsPlugin =
+
S3AccessGrantsPlugin.builder().enableFallback(s3agFallbackEnabled).build();
+ builder.addPlugin(accessGrantsPlugin);
+ LOG.info("s3ag plugin is added to s3 client with fallback: {}",
s3agFallbackEnabled);
Review Comment:
use a LogExactlyOnce. this will get oververbose in processes which
create/destroy fs instances
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 org.apache.hadoop.conf.Configuration;
+import org.junit.Test;
+
+import software.amazon.awssdk.services.s3.S3AsyncClientBuilder;
+import software.amazon.awssdk.services.s3.S3AsyncClient;
+import software.amazon.awssdk.services.s3.S3BaseClientBuilder;
+import software.amazon.awssdk.services.s3.S3ClientBuilder;
+import software.amazon.awssdk.services.s3.S3Client;
+
+import static org.apache.hadoop.fs.s3a.Constants.AWS_S3_ACCESS_GRANTS_ENABLED;
+import static org.junit.Assert.assertEquals;
+
+
+/**
+ * Test S3 Access Grants configurations.
+ */
+public class TestS3AccessGrantConfiguration {
Review Comment:
`extends AbstractHadoopTestBase`
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 org.apache.hadoop.conf.Configuration;
+import org.junit.Test;
+
+import software.amazon.awssdk.services.s3.S3AsyncClientBuilder;
+import software.amazon.awssdk.services.s3.S3AsyncClient;
+import software.amazon.awssdk.services.s3.S3BaseClientBuilder;
+import software.amazon.awssdk.services.s3.S3ClientBuilder;
+import software.amazon.awssdk.services.s3.S3Client;
+
+import static org.apache.hadoop.fs.s3a.Constants.AWS_S3_ACCESS_GRANTS_ENABLED;
+import static org.junit.Assert.assertEquals;
+
+
+/**
+ * Test S3 Access Grants configurations.
+ */
+public class TestS3AccessGrantConfiguration {
+
+ @Test
+ public void testS3AccessGrantsEnabled() {
+ Configuration conf = new Configuration();
+ conf.set(AWS_S3_ACCESS_GRANTS_ENABLED, "true");
Review Comment:
`setBoolean(.., true)`
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 org.apache.hadoop.conf.Configuration;
+import org.junit.Test;
+
+import software.amazon.awssdk.services.s3.S3AsyncClientBuilder;
+import software.amazon.awssdk.services.s3.S3AsyncClient;
+import software.amazon.awssdk.services.s3.S3BaseClientBuilder;
+import software.amazon.awssdk.services.s3.S3ClientBuilder;
+import software.amazon.awssdk.services.s3.S3Client;
+
+import static org.apache.hadoop.fs.s3a.Constants.AWS_S3_ACCESS_GRANTS_ENABLED;
+import static org.junit.Assert.assertEquals;
+
+
+/**
+ * Test S3 Access Grants configurations.
+ */
+public class TestS3AccessGrantConfiguration {
+
+ @Test
+ public void testS3AccessGrantsEnabled() {
+ Configuration conf = new Configuration();
Review Comment:
use new Configuration(false) to avoid loading any default values -this
avoids integration test settings breaking unit tests. even better: make it a
final field
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 org.apache.hadoop.conf.Configuration;
+import org.junit.Test;
+
+import software.amazon.awssdk.services.s3.S3AsyncClientBuilder;
+import software.amazon.awssdk.services.s3.S3AsyncClient;
+import software.amazon.awssdk.services.s3.S3BaseClientBuilder;
+import software.amazon.awssdk.services.s3.S3ClientBuilder;
+import software.amazon.awssdk.services.s3.S3Client;
+
+import static org.apache.hadoop.fs.s3a.Constants.AWS_S3_ACCESS_GRANTS_ENABLED;
+import static org.junit.Assert.assertEquals;
+
+
+/**
+ * Test S3 Access Grants configurations.
+ */
+public class TestS3AccessGrantConfiguration {
+
+ @Test
+ public void testS3AccessGrantsEnabled() {
+ Configuration conf = new Configuration();
+ conf.set(AWS_S3_ACCESS_GRANTS_ENABLED, "true");
+ S3ClientBuilder builder = S3Client.builder();
+ DefaultS3ClientFactory.applyS3AccessGrantsConfigurations(builder, conf);
+ verifyS3AGPluginEnabled(builder);
+ }
+
+ @Test
+ public void testS3AccessGrantsEnabledAsync() {
+ Configuration conf = new Configuration();
+ conf.set(AWS_S3_ACCESS_GRANTS_ENABLED, "true");
+ S3AsyncClientBuilder builder = S3AsyncClient.builder();
+ DefaultS3ClientFactory.applyS3AccessGrantsConfigurations(builder, conf);
+ verifyS3AGPluginEnabled(builder);
+ }
+
+ @Test
+ public void testS3AccessGrantsDisabled() {
+ Configuration conf = new Configuration();
+ conf.set(AWS_S3_ACCESS_GRANTS_ENABLED, "false");
+ S3ClientBuilder builder = S3Client.builder();
+ DefaultS3ClientFactory.applyS3AccessGrantsConfigurations(builder, conf);
+ verifyS3AGPluginDisabled(builder);
+ }
+
+ @Test
+ public void testS3AccessGrantsDisabledByDefault() {
+ Configuration conf = new Configuration();
+ S3ClientBuilder builder = S3Client.builder();
+ DefaultS3ClientFactory.applyS3AccessGrantsConfigurations(builder, conf);
+ verifyS3AGPluginDisabled(builder);
+ }
+
+ @Test
+ public void testS3AccessGrantsDisabledAsync() {
+ Configuration conf = new Configuration();
+ conf.set(AWS_S3_ACCESS_GRANTS_ENABLED, "false");
+ S3AsyncClientBuilder builder = S3AsyncClient.builder();
+ DefaultS3ClientFactory.applyS3AccessGrantsConfigurations(builder, conf);
+ verifyS3AGPluginDisabled(builder);
+ }
+
+ @Test
+ public void testS3AccessGrantsDisabledByDefaultAsync() {
+ Configuration conf = new Configuration();
+ S3AsyncClientBuilder builder = S3AsyncClient.builder();
+ DefaultS3ClientFactory.applyS3AccessGrantsConfigurations(builder, conf);
+ verifyS3AGPluginDisabled(builder);
+ }
+
+ private <BuilderT extends S3BaseClientBuilder<BuilderT, ClientT>, ClientT>
void
+ verifyS3AGPluginEnabled(BuilderT builder) {
+ assertEquals(builder.plugins().size(), 1);
Review Comment:
use assertJ assertions on list sizes here and below
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 org.apache.hadoop.conf.Configuration;
Review Comment:
nit: review import ordering
##########
hadoop-project/pom.xml:
##########
@@ -187,7 +187,7 @@
<make-maven-plugin.version>1.0-beta-1</make-maven-plugin.version>
<surefire.fork.timeout>900</surefire.fork.timeout>
<aws-java-sdk.version>1.12.599</aws-java-sdk.version>
- <aws-java-sdk-v2.version>2.23.5</aws-java-sdk-v2.version>
+ <aws-java-sdk-v2.version>2.23.7</aws-java-sdk-v2.version>
Review Comment:
SDK updates need to be self contained PRs for isolated cherrypick and
revert. section in testing.md on the process. Yes, you do have to follow it,
including looking at accidental dependency exports and new error messages in
the text output.
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/Constants.java:
##########
@@ -1600,4 +1600,20 @@ private Constants() {
*/
public static final boolean CHECKSUM_VALIDATION_DEFAULT = false;
+
+ /**
+ * Flag to enable S3 Access Grants to control authorization to S3 data. More
information:
+ * https://aws.amazon.com/s3/features/access-grants/
+ * and
+ * https://github.com/aws/aws-s3-accessgrants-plugin-java-v2/
Review Comment:
use {@value} in the comments so ides will show the value
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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 org.apache.hadoop.conf.Configuration;
+import org.junit.Test;
+
+import software.amazon.awssdk.services.s3.S3AsyncClientBuilder;
+import software.amazon.awssdk.services.s3.S3AsyncClient;
+import software.amazon.awssdk.services.s3.S3BaseClientBuilder;
+import software.amazon.awssdk.services.s3.S3ClientBuilder;
+import software.amazon.awssdk.services.s3.S3Client;
+
+import static org.apache.hadoop.fs.s3a.Constants.AWS_S3_ACCESS_GRANTS_ENABLED;
+import static org.junit.Assert.assertEquals;
+
+
+/**
+ * Test S3 Access Grants configurations.
+ */
+public class TestS3AccessGrantConfiguration {
+
+ @Test
+ public void testS3AccessGrantsEnabled() {
Review Comment:
There is a lot of duplication in these tests: you should factor it out as
much as possible to keep test maintenance down.
--
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]
