HADOOP-19050. SDK Add Support for AWS S3 Access Grants #6507
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
The failed junit test is not related to this change. (hadoop.yarn.server.timelineservice.security.TestTimelineAuthFilterForV2) |
|
Trunk compile tests all passed, but some of the patch compile tests failed. Could someone help to give some pointer how to fix this? Thanks. |
|
💔 -1 overall
This message was automatically generated. |
|
We realized the failures are likely due to the dependencies being brought in by the S3 Access Grants plugin. We've removed them, as users will be using their own S3 Access Grants plugin JAR in their environments if they'd like this functionality (we should not be shading this plugin with Hadoop). Local builds are passing after making the dependency exclusions. |
|
@jxhan3 - BTW we have the wrong ticket number here. Correct ticket is HADOOP-19050. Please update this, as the updates from this PR are not making it to the correct ticket. |
jxhan3
changed the title
ahmarsuhail
requested changes
Jan 31, 2024
There was a problem hiding this comment.
Thanks, suggested some changes. the code changes are simple enough, but I believe this should be separated from the DefaultS3ClientFactory. Would be good know @steveloughran's opinion.
Took a quick look at unit test, looks ok but still need to review that properly. Wondering if we need an ITest, but the only change we're making in S3A is a configuration one..so maybe not.
LICENSE-binary
Outdated
| @@ -363,7 +363,7 @@ org.objenesis:objenesis:2.6 | |||
| org.xerial.snappy:snappy-java:1.1.10.4 | |||
| org.yaml:snakeyaml:2.0 | |||
| org.wildfly.openssl:wildfly-openssl:1.1.3.Final | |||
| software.amazon.awssdk:bundle:jar:2.23.5 | |||
There was a problem hiding this comment.
this shouldn't be here. it's already part of your SDK upgrade PR so cut from here
hadoop-project/pom.xml
Outdated
| @@ -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> | |||
There was a problem hiding this comment.
cut, SDK upgrade needs to happen separately
| @@ -508,6 +508,29 @@ | |||
| <artifactId>bundle</artifactId> | |||
| <scope>compile</scope> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
@steveloughran do you have any advice here? I think we should do what we did for Client Side Encryption, have this S3 access grants jar as optional, and create a new client factory which will add the S3 access grants plugin.
If there are other plugins that we want to add in the future, this new client factory can be generalised for that.
There was a problem hiding this comment.
the plugin should go in bundle.jar. it's meant to be a bundle.
There was a problem hiding this comment.
these should all be moved into the pom.xml in hadoop-project as that where we define dependencies. look at how we import the sdk dependency in this pom.xml for example.
steveloughran
requested changes
Jan 31, 2024
There was a problem hiding this comment.
Commented.
We cannot have any more unshaded aws sdk jars as required on our classpath; removing s3 select in #6144 has simplified our life by removing another optional one.
Do you have a timetable for incorporating this plugging into bundle.jar?
Otherwise, it is critical that if the jar is not on the cross path normal S3 clients can be constructed and used.
This will need documentation. Either in connecting.md or a new file in the same directory src/site/markdown/tools/hadoop-aws
I do not see any integration tests. What is the story here? Is it possible to run the whole mvn verify test run with access grants? if so, adding a paragraph in testing.md would be good, and particular: how to set it up. I am particularly curious about how well the delegation tokens worked...are session credentials supported?
The feature probably also needs an extra line in the "qualifying an SDK" section.
I have not played with S3 access grants and so cannot suggest test cases myself. Given its purpose, it would be good to have tests which not only verify that the cooler can access data, but that in some places they cannot.
hadoop-project/pom.xml
Outdated
| @@ -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> | |||
There was a problem hiding this 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.
| @@ -508,6 +508,29 @@ | |||
| <artifactId>bundle</artifactId> | |||
| <scope>compile</scope> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>software.amazon.s3.accessgrants</groupId> | |||
There was a problem hiding this 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?
There was a problem hiding this comment.
This new JAR should be optional - and as per the original instructions the AWS S3 Access Grants team was given by the AWS Java SDK team, this plugin should not be part of the AWS Java SDK nor the SDK bundle. The reasoning behind this was that Java SDKv2 Plugins should be considered as "open source" for the most part as they are only interfaces that anyone can implement and then use wherever they'd like. In other words, the S3 Access Grants plugin should be, in theory, treated as any other open source dependency that we would be utilizing if a customer explicitly enables this in S3A.
So, to further answer the question, we need to find a way to optionally load these classes if a user specifies that they'd like to use the plugin AND provides the JAR on the classpath. That is missing from this PR as of now and @jxhan3 and I will work on it. I think @ahmarsuhail's link above has a good call pattern for doing this - we'll follow this unless you have any other suggestion.
One interesting thing to note - I've seen the S3ExpressPlugin being merged into the AWS Java SDK (which was explicitly not the recommended option by the AWS Java SDK team, per my understanding). I've started further inquiries to find why that's the case - and how this is different than S3 Access Grants. Will report my findings here.
There was a problem hiding this comment.
@ahmarsuhail put in the work to get s3 express in
| * 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"; |
There was a problem hiding this comment.
nit; can you use "." over "-"
| S3AccessGrantsPlugin accessGrantsPlugin = | ||
| S3AccessGrantsPlugin.builder().enableFallback(s3agFallbackEnabled).build(); | ||
| builder.addPlugin(accessGrantsPlugin); | ||
| LOG.info("s3ag plugin is added to s3 client with fallback: {}", s3agFallbackEnabled); |
There was a problem hiding this comment.
use a LogExactlyOnce. this will get oververbose in processes which create/destroy fs instances
|
|
||
| package org.apache.hadoop.fs.s3a; | ||
|
|
||
| import org.apache.hadoop.conf.Configuration; |
There was a problem hiding this comment.
nit: review import ordering
| /** | ||
| * Test S3 Access Grants configurations. | ||
| */ | ||
| public class TestS3AccessGrantConfiguration { |
There was a problem hiding this comment.
extends AbstractHadoopTestBase
...-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3AccessGrantConfiguration.java
Show resolved
Hide resolved
|
|
||
| @Test | ||
| public void testS3AccessGrantsEnabled() { | ||
| Configuration conf = new Configuration(); |
There was a problem hiding this 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
| @Test | ||
| public void testS3AccessGrantsEnabled() { | ||
| Configuration conf = new Configuration(); | ||
| conf.set(AWS_S3_ACCESS_GRANTS_ENABLED, "true"); |
There was a problem hiding this comment.
setBoolean(.., true)
| * 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/ |
There was a problem hiding this comment.
use {@value} in the comments so ides will show the value
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Hi @steveloughran - thanks for your time on this. We appreciate it a lot. I've responded to your comments inline below:
Totally agreed with you on this. @jxhan3 and I will work to make sure this is optional as we do not have any timeline (or known plans) for getting the plugin incorporated into the AWS Java SDKv2 bundle.jar. (More on this in my comment here). If this changes in the future, we'd be glad to reverse any classloading code we may need for now.
Noted. @jxhan3 will work on this in the next PR revision.
The story currently is, if we treat the plugin as yet another third-party (and optional) dependency, then this PR is only going to be providing the bare minimum code for users to be able to enable the plugin if they explicitly choose to do so. Any issues with the actual functionality of the plugin should be addressed by the plugin itself at their open source GitHub. So then, the only testing that we'd require would be to ensure that if users are explicitly enabling this feature, that S3A is ensuring its S3 clients have the plugin attached. Other open-source contributions (e.g. Iceberg) have accepted this testing model - and I'd also recommend it to reduce the need for redundant test coverage between S3A and the S3 Access Grants plugin. If you don't agree with this testing model, we can surely try to add additional ITest cases that will both setup and tear down the S3 Access Grants instance, locations, and required grants (to be noted: S3 Access Grants APIs are not free) - and then test both when users should and should not be able to receive access. However, running all existing test cases under this model will be a heavy task that will likely require lots of test case refactoring, as Access Grants are defined on a location-by-location basis. In order to test both when users should and should not have access for each test case will require both additional setup and test code to ensure that those situations can be adequately tested with multiple data locations. I'm not sure that the ROI on making such a large change will be there. Please let me know your thoughts on this. As for how the feature works, S3 Access Grants will authenticate the credentials to find the IAM user associated with it - then use that identity for the authorization before returning a new set of scoped credentials to actually access the data (in other words, the credentials that are inputted to the S3 client will not be the credentials used to actually access the data). The S3 Access Grants plugin is the mechanism that will do the entire credential vending process and using the vended credentials properly in any calls made from the attached S3 client. As such, session credentials and delegation tokens will work given that the credentials that are passed to the S3 client (using any mechanism) are valid and can be authenticated properly.
Noted. @jxhan3 will work on this in the next PR revision. |
|
Some tests done in EMR cluster: (all successful with expected behavior)
Some sample test results: Hadoop CLIDisable s3ag:User can only access prefixes allowed by IAM role. User can not access prefixes through S3 Access Grants.s3ag is enabled with fallback:User can access prefixes through s3ag. If s3ag failed to give credentials, user can access prefixes through IAM role policy.Sparks3ag is enabled without fallback:User can only access prefixes through s3ag, not IAM role policy.s3ag is disabled:User can only access prefixes through IAM role policy, not s3ag. |
|
Local verification is running, update soon. |
|
@jxhan3 can you please put the output from your console testing onto the comments here? No need to paste the actual grants but just say which ones have grants and should be successful or not. It would be nice to have the actual output noted on this CR itself as proof. |
|
💔 -1 overall
This message was automatically generated. |
jxhan3
force-pushed
the
HDFS-17351
branch
2 times, most recently
from
8382af8
to
347cc9f
Compare
|
Test optional plugin:
|
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
adnanhemani
reviewed
Feb 5, 2024
| applyS3AccessGrantsConfigurations(BuilderT builder, Configuration conf) { | ||
| boolean s3agEnabled = conf.getBoolean(AWS_S3_ACCESS_GRANTS_ENABLED, false); | ||
| if (!s3agEnabled){ | ||
| LOG_EXACTLY_ONCE.debug("s3ag plugin is not enabled."); |
There was a problem hiding this comment.
On all logs, can we use the full name: "S3 Access Grants..." to make it clear for users looking through the logs?
There was a problem hiding this comment.
this won't work, so basically it will log s3ag plugin is not enabled. once, but then your LOG_EXACTLY_ONCE.debug( "Class {} is not found exception: {}." and wherever else you use it will never log. If you want to log those they will need their own instances of log exactly once.
But also these are debug logs, you don't need to use log exactly once. We do that more when we need to make user logs clearer when logging at warn or info. You can just use a regular logger here in my opinion.
I also don't think you need this s3ag plugin is not enabled. log specifically. it's getting logged for any who is not using S3AG, so not adding any value. instead maybe add a log in the try block Configuring S3 access grants plugin
|
Hi @steveloughran and @ahmarsuhail - I think this code is in a much more ready state than before and we've attempted to answer the questions you had earlier. Please let us know what other thoughts you have on this. (Also, I think the Unit Tests are a bit flaky here - not sure what to do about those) |
ahmarsuhail
requested changes
Feb 5, 2024
There was a problem hiding this comment.
thanks, left some comments.
Also please add documentation. you'll need to add the new config option in index.md and then a new section there explaining how this access grants stuff works. also that if there are any S3 access grant related issues, they should be reported on the plugin github. As all we do is enable it on the S3Client.
| applyS3AccessGrantsConfigurations(BuilderT builder, Configuration conf) { | ||
| boolean s3agEnabled = conf.getBoolean(AWS_S3_ACCESS_GRANTS_ENABLED, false); | ||
| if (!s3agEnabled){ | ||
| LOG_EXACTLY_ONCE.debug("s3ag plugin is not enabled."); |
There was a problem hiding this comment.
this won't work, so basically it will log s3ag plugin is not enabled. once, but then your LOG_EXACTLY_ONCE.debug( "Class {} is not found exception: {}." and wherever else you use it will never log. If you want to log those they will need their own instances of log exactly once.
But also these are debug logs, you don't need to use log exactly once. We do that more when we need to make user logs clearer when logging at warn or info. You can just use a regular logger here in my opinion.
I also don't think you need this s3ag plugin is not enabled. log specifically. it's getting logged for any who is not using S3AG, so not adding any value. instead maybe add a log in the try block Configuring S3 access grants plugin
| @@ -401,4 +409,32 @@ private static Region getS3RegionFromEndpoint(final String endpoint, | |||
| return Region.of(AWS_S3_DEFAULT_REGION); | |||
| } | |||
|
|
|||
| public static <BuilderT extends S3BaseClientBuilder<BuilderT, ClientT>, ClientT> void | |||
There was a problem hiding this comment.
This is for testing purpose, otherwise we may need to use reflection to test private method. Please share your thoughts on this. Thanks.
| @@ -0,0 +1,60 @@ | |||
| package org.apache.hadoop.fs.s3a.tools; | |||
|
|
|||
| import org.apache.hadoop.conf.Configuration; | |||
There was a problem hiding this comment.
add apache license to the top of this class (copy it over from any other class)
| protected static final Logger LOG = | ||
| LoggerFactory.getLogger(S3AccessGrantsUtil.class); | ||
|
|
||
| private static final LogExactlyOnce LOG_EXACTLY_ONCE = new LogExactlyOnce(LOG); |
There was a problem hiding this comment.
rename from LOG_EXACTLY_ONCE to what this log is actually for, eg: IAM_FALLBACK_WARN. look at WARN_OF_DEFAULT_REGION_CHAIN in DefaultS3ClientFactory as an example.
| S3AccessGrantsPlugin accessGrantsPlugin = | ||
| S3AccessGrantsPlugin.builder().enableFallback(s3agFallbackEnabled).build(); | ||
| builder.addPlugin(accessGrantsPlugin); | ||
| LOG_EXACTLY_ONCE.info("s3ag plugin is added to s3 client with fallback: {}", s3agFallbackEnabled); |
There was a problem hiding this comment.
Have two different Loggers that log exactly once for these two different statements.
There was a problem hiding this comment.
and +1 on @adnanhemani 's comment, let's make these logs uniform so they begin with "S3 access grant ... "
| @@ -0,0 +1,60 @@ | |||
| package org.apache.hadoop.fs.s3a.tools; | |||
|
|
|||
There was a problem hiding this comment.
wrong package for this class, move to the impl package.
| assertEquals(builder.plugins().size(), 0); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
add new line after class
| @@ -508,6 +508,29 @@ | |||
| <artifactId>bundle</artifactId> | |||
| <scope>compile</scope> | |||
| </dependency> | |||
| <dependency> | |||
There was a problem hiding this comment.
these should all be moved into the pom.xml in hadoop-project as that where we define dependencies. look at how we import the sdk dependency in this pom.xml for example.
This reverts commit 1e47cc9.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Update: I've successfully influenced the S3 Access Grants team and the AWS Java SDK team to include the S3 Access Grants Plugin within the AWS Java SDK bundle. For that, we will require an SDK version upgrade. Will work on that first to ensure we don't require the reflection logic here. In the meantime, any review on the latest version of this code would be appreciated - it will help me make the logic here solid while we work on the AWS SDK upgrade in parallel. |
This was referenced Feb 8, 2024
|
This has been evolved into #6544. This PR will no longer be used now. |
|
@adnanhemani thanks; without that change we'd have problems with the PR, as in "you get to support it all through reflection" the way we have to do with wildfly/openssl binding (NetworkBinding) and more. |
steveloughran
requested changes
Mar 5, 2024
There was a problem hiding this comment.
@adnanhemani aah. trouble with the decoupling is it misses a key reason we love the bundle jar, despite its weight.
The bundle JAR shades its dependencies which is critical for us so it is possible
- For the AWS team to update their dependencies without forcing us to update and cause transitive pain all the way down the stack
- Avoids forcing us to freeze the aws jar version because of version conflict.
- avoids subtle version conflict between dependencies which only surface in production
These are not hypothetical concerns: we have encountered all of them in previous releases of the v1 SDK to the point where we simply stopped upgrading the AWS SDK as it was trying to dictate versions all the way up the stack. You look for the relevant JIRAs in hadoop 2.x releases if you are curious.
In your new library, you are now importing caffeine. What if we want to use it ourselves? We don't want the AG to force us to upgrade when you do, or stop us upgrading when we want to.
If you want this to get into the hadoop-aws module then I'm afraid you will need to get it into bundle.jar. Please discuss with the SDK team.
| <dependency> | ||
| <groupId>software.amazon.s3.accessgrants</groupId> | ||
| <artifactId>aws-s3-accessgrants-java-plugin</artifactId> | ||
| <version>2.0.0</version> |
There was a problem hiding this comment.
can you move this version flag into a property next to the other aws s3 options, at least until this module is merged into bundle.jar. this makes it overrideable
| <artifactId>*</artifactId> | ||
| </exclusion> | ||
| <exclusion> | ||
| <groupId>org.assertj</groupId> |
There was a problem hiding this comment.
this should really be scoped as testing in the module itself...
| <artifactId>*</artifactId> | ||
| </exclusion> | ||
| <exclusion> | ||
| <groupId>com.github.ben-manes.caffeine</groupId> |
There was a problem hiding this comment.
are you confident this isn't used?
| @@ -508,6 +508,29 @@ | |||
| <artifactId>bundle</artifactId> | |||
| <scope>compile</scope> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>software.amazon.s3.accessgrants</groupId> | |||
There was a problem hiding this comment.
@ahmarsuhail put in the work to get s3 express in
| LOG.debug( | ||
| "Class {} is not found exception: {}.", | ||
| S3AG_UTIL_CLASSNAME, | ||
| e.getStackTrace() |
There was a problem hiding this comment.
- pass in e and let the logger handle the rest.
- use multiple classes in the catch statement to avoid duplication/maintenance costs
| ```xml | ||
| <configuration> | ||
| ... | ||
| <property> |
There was a problem hiding this comment.
nit: use two chars for indentation
| applyVerifyS3AGPlugin(S3AsyncClient.builder(), true, false); | ||
| } | ||
|
|
||
| private Configuration createConfig(boolean isDefault, boolean s3agEnabled) { |
| DefaultS3ClientFactory.applyS3AccessGrantsConfigurations(builder, createConfig(isDefault, enabled)); | ||
| if (enabled){ | ||
| assertEquals(1, builder.plugins().size()); | ||
| assertEquals("software.amazon.awssdk.s3accessgrants.plugin.S3AccessGrantsPlugin", |
There was a problem hiding this comment.
use assertj asserts here
| applyVerifyS3AGPlugin(BuilderT builder, boolean isDefault, boolean enabled) { | ||
| DefaultS3ClientFactory.applyS3AccessGrantsConfigurations(builder, createConfig(isDefault, enabled)); | ||
| if (enabled){ | ||
| assertEquals(1, builder.plugins().size()); |
There was a problem hiding this comment.
assert j assert on .hasSize() for list
| ); | ||
| } | ||
| else { | ||
| assertEquals(builder.plugins().size(), 0); |
steveloughran
changed the title
|
Hi @steveloughran, as noted in this comment (and the one above it), I’ve made the changes you requested (including getting the S3 Access Grants plugin added to the bundle JAR) in a new PR: #6507 (comment) #6544 has those changes. Unless I’m misunderstanding what you are commenting about (from what I got, you are advocating for the plugin to be part of the bundle JAR - which is now the case), please close this PR. I’m glad to continue our discussion on the new PR (#6544) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Add support for AWS S3 Access Grants(https://aws.amazon.com/s3/features/access-grants/)
How was this patch tested?
Run all integration tests with scale, assume role and KMS.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?