steveloughran commented on code in PR #6544:
URL: https://github.com/apache/hadoop/pull/6544#discussion_r1522069895
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java:
##########
@@ -401,4 +406,22 @@ private static Region getS3RegionFromEndpoint(final String
endpoint,
return Region.of(AWS_S3_DEFAULT_REGION);
}
+ private static <BuilderT extends S3BaseClientBuilder<BuilderT, ClientT>,
ClientT> void
+ applyS3AccessGrantsConfigurations(BuilderT builder, Configuration conf) {
+ boolean isS3AccessGrantsEnabled =
conf.getBoolean(AWS_S3_ACCESS_GRANTS_ENABLED, false);
+ if (!isS3AccessGrantsEnabled){
+ LOG.debug("S3 Access Grants plugin is not enabled.");
+ return;
+ }
+
+ boolean isFallbackEnabled =
+ conf.getBoolean(AWS_S3_ACCESS_GRANTS_FALLBACK_TO_IAM_ENABLED, false);
+ S3AccessGrantsPlugin accessGrantsPlugin =
+ S3AccessGrantsPlugin.builder()
+ .enableFallback(isFallbackEnabled)
+ .build();
+ builder.addPlugin(accessGrantsPlugin);
+ LOG.info("S3 Access Grants plugin is enabled with IAM fallback set to {}",
isFallbackEnabled);
Review Comment:
might be good to add once so that on a large system the logs don't get full
of noise. tricky choice
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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 java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+
+import org.assertj.core.api.AbstractStringAssert;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+import software.amazon.awssdk.awscore.AwsClient;
+import
software.amazon.awssdk.s3accessgrants.plugin.S3AccessGrantsIdentityProvider;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+
+import static org.apache.hadoop.fs.s3a.Constants.AWS_S3_ACCESS_GRANTS_ENABLED;
+
+
+/**
+ * Test S3 Access Grants configurations.
+ */
+public class TestS3AccessGrantConfiguration extends AbstractHadoopTestBase {
+ /**
+ * This credential provider will be attached to any client
+ * that has been configured with the S3 Access Grants plugin.
+ * {@link software.amazon.awssdk.s3accessgrants.plugin.S3AccessGrantsPlugin}.
Review Comment:
I don't think javadoc will resolve that; just use {@code}
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/DefaultS3ClientFactory.java:
##########
@@ -178,6 +181,8 @@ private <BuilderT extends S3BaseClientBuilder<BuilderT,
ClientT>, ClientT> Build
configureEndpointAndRegion(builder, parameters, conf);
+ applyS3AccessGrantsConfigurations(builder, conf);
Review Comment:
rename maybeApply... to make clear it isn't guaranteed to happen
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -5516,6 +5522,10 @@ public boolean hasPathCapability(final Path path, final
String capability)
case FIPS_ENDPOINT:
return fipsEnabled;
+ // is S3 Access Grants enabled
+ case AWS_S3_ACCESS_GRANTS_ENABLED:
Review Comment:
our bucket-info command has a list of capabilities to probe for -add this to
the list
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:
##########
@@ -494,6 +494,11 @@ public class S3AFileSystem extends FileSystem implements
StreamCapabilities,
*/
private String configuredRegion;
+ /**
+ * Is a S3 Access Grants Enabled?
Review Comment:
nit: "are S3 Access Grants Enabled?"
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.apache.hadoop.test.AbstractHadoopTestBase;
+import org.junit.Assert;
+import org.junit.Test;
+
+import software.amazon.awssdk.awscore.AwsClient;
+import
software.amazon.awssdk.s3accessgrants.plugin.S3AccessGrantsIdentityProvider;
Review Comment:
I mean a subclass of the S3AccessGrantsIdentityProvider the way we do for
env vars, IAM roles etc. lets us hide some of the implementation details (e.g.
how we moved from EC2 to container creds, upgraded to v2 sdk)
##########
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/authentication.md:
##########
@@ -0,0 +1,433 @@
+<!---
+ Licensed 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. See accompanying LICENSE file.
+-->
+
+# Authenticating with S3
+
+<!-- MACRO{toc|fromDepth=0|toDepth=2} -->
+
+Except when interacting with public S3 buckets, the S3A client
+needs the credentials needed to interact with buckets.
+
+The client supports multiple authentication mechanisms and can be configured
as to
+which mechanisms to use, and their order of use. Custom implementations
+of `com.amazonaws.auth.AWSCredentialsProvider` may also be used.
+However, with the upcoming upgrade to AWS Java SDK V2, these classes will need
to be
Review Comment:
no longer upcoming, replace with "the upgrade in Hadoop 3.4.0"
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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 java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+
+import org.assertj.core.api.AbstractStringAssert;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+import software.amazon.awssdk.awscore.AwsClient;
+import
software.amazon.awssdk.s3accessgrants.plugin.S3AccessGrantsIdentityProvider;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.test.AbstractHadoopTestBase;
+
+import static org.apache.hadoop.fs.s3a.Constants.AWS_S3_ACCESS_GRANTS_ENABLED;
+
+
+/**
+ * Test S3 Access Grants configurations.
+ */
+public class TestS3AccessGrantConfiguration extends AbstractHadoopTestBase {
+ /**
+ * This credential provider will be attached to any client
+ * that has been configured with the S3 Access Grants plugin.
+ * {@link software.amazon.awssdk.s3accessgrants.plugin.S3AccessGrantsPlugin}.
+ */
+ public static final String
S3_ACCESS_GRANTS_EXPECTED_CREDENTIAL_PROVIDER_CLASS =
+ S3AccessGrantsIdentityProvider.class.getName();
+
+ @Test
+ public void testS3AccessGrantsEnabled() throws IOException,
URISyntaxException {
+ assertCredentialProviderClass(
+ createConfig(true),
+ true,
+ "S3 Access Grants is explicitly enabled on an S3 Async Client",
+ true);
+
+ assertCredentialProviderClass(
+ createConfig(true),
+ false,
+ "S3 Access Grants is explicitly enabled on an S3 Non-Async Client",
+ true);
+ }
+
+ @Test
+ public void testS3AccessGrantsDisabled() throws IOException,
URISyntaxException {
+ assertCredentialProviderClass(
+ new Configuration(),
+ true,
+ "S3 Access Grants is implicitly disabled (default behavior) on an S3
Async Client",
+ false);
+
+ assertCredentialProviderClass(
+ new Configuration(),
+ false,
+ "S3 Access Grants is implicitly disabled (default behavior) on an S3
Non-Async Client",
+ false);
+
+ assertCredentialProviderClass(
+ createConfig(false),
+ true,
+ "S3 Access Grants is explicitly disabled on an S3 Async Client",
+ false);
+
+ assertCredentialProviderClass(
+ createConfig(false),
+ false,
+ "S3 Access Grants is explicitly disabled on an S3 Non-Async Client",
+ false);
+ }
+
+ private Configuration createConfig(boolean s3agEnabled) {
+ Configuration conf = new Configuration();
+ conf.setBoolean(AWS_S3_ACCESS_GRANTS_ENABLED, s3agEnabled);
+ return conf;
+ }
+
+ private String getCredentialProviderName(AwsClient awsClient) {
+ return
awsClient.serviceClientConfiguration().credentialsProvider().getClass().getName();
+ }
+
+ private AwsClient getAwsClient(Configuration conf, boolean asyncClient)
+ throws IOException, URISyntaxException {
+ DefaultS3ClientFactory factory = new DefaultS3ClientFactory();
+ factory.setConf(conf);
+ S3ClientFactory.S3ClientCreationParameters parameters =
+ new S3ClientFactory.S3ClientCreationParameters();
+ URI uri = new URI("any-uri");
+ return asyncClient ?
+ factory.createS3AsyncClient(uri, parameters):
factory.createS3Client(uri, parameters);
+ }
+
+ private void assertCredentialProviderClass(
+ Configuration configuration, boolean asyncClient, String message,
boolean shouldMatch)
+ throws IOException, URISyntaxException {
+ AwsClient awsClient = getAwsClient(configuration, asyncClient);
+ AbstractStringAssert<?> assertion =
Review Comment:
nice trick! always good to see advanced uses of AssertJ
--
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
