HADOOP-18997. S3A: make createSession optional when working with S3Express buckets #6316
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
New option fs.s3a.s3express.create.session; default is true. Disabled in some of the role tests. Change-Id: I8d3af8265525346a42cf3f5612a8d0130de4f1c6
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
commented
Dec 2, 2023
There was a problem hiding this comment.
would like some advice from @ahmarsuhail about building the right IAM policy to issue createSession access to the active bucket, so we can dynamically generate the right permissions for delegation tokens. The PR currently just turns off createSession in those tests.
I'm also thinking we need to consider having a special test.s3express.enabled switch to turn on any specific tests we should add here.
- verify turning off the session works (by assuming a role without it and reading a file)
- try to connect to an unknown bucket and verify that it fails meaningfully
- skip those distcp tests in treewalk
- maybe: dynamically disable SSE encryption tests
- future tests?
the core "is this an s3express bucket?" probe exists and we can use this to dynamically skip test suites/cases today.
| @@ -4498,6 +4500,10 @@ public List<RoleModel.Statement> listAWSPolicyRules( | |||
| // way to predict read keys, and not worried about granting | |||
| // too much encryption access. | |||
| statements.add(STATEMENT_ALLOW_KMS_RW); | |||
| if (s3ExpressStore) { | |||
| // tODO | |||
There was a problem hiding this comment.
yeah, what do we add here?
| // write access. | ||
| conf.set(CANNED_ACL, LOG_DELIVERY_WRITE); | ||
|
|
||
| if (conf.getBoolean(KEY_ACL_TESTS_ENABLED, false)) { |
There was a problem hiding this comment.
or do this automatically on an s3 express bucket; saves fiddling with so many settings when testing different storage types
* warn that policy rule can't be generated * move ApiCallTimeoutException logic until after sdk exceptions get translated to IOE. This lines up for any future AWS throwing up underlying cause here. Change-Id: Ife4071ed97d810df405297b62513b481576cf733
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
changed the title
steveloughran
changed the title
* Tests to automatically skip acl, storage class, S3 Select or encryption tests when target fs is S3Express. * same for the out of order multipart uploader test cases, v1 listing. * bucket tool s3 test treats invalid location error as a successful invocation of the create bucket attempt * More disabling of CreateSession call with role tests. A lot of role tests are still failing; looks like role policies aren't being set up right. Going to create a new JIRA for that as it's going to take time on its own and is independent from the actual ability to disable sessions. Change-Id: Ib3b3acd9f8a062526bdf958e3403992c2c9a0371
|
tested s3express us-west. all the role tests are failing, but not in createsession; some other issue which I won't address here. ITestS3ACommitterFactory seems unrelated; the setting of the invalid config isn't getting passed down. will create a jira there too. |
|
@HarshitGupta11 @ahmarsuhail can I get this looked at? thanks. |
|
🎊 +1 overall
This message was automatically generated. |
ahmarsuhail
approved these changes
Dec 6, 2023
There was a problem hiding this comment.
+1. Some nits, but otherwise LGTM.
do you know what tests pass with disableCreateSession(conf) that weren't passing before? for me it doesn't seem to make a difference for a lot of them.
I'm asking internally why the LIST calls are failing for us. IAM permissions with S3Express have changed a bit, documented here. but without createSession, it should (i think) be the same. Once I have clarity there I will create the PR for https://issues.apache.org/jira/browse/HADOOP-19003
| @@ -181,6 +189,10 @@ protected Configuration createValidRoleConf() throws JsonProcessingException { | |||
| conf.set(ASSUMED_ROLE_ARN, roleARN); | |||
| conf.set(ASSUMED_ROLE_SESSION_NAME, "valid"); | |||
| conf.set(ASSUMED_ROLE_SESSION_DURATION, "45m"); | |||
| // disable create session so there's no need to | |||
| // add a role policy for it. | |||
| disableCreateSession(conf); | |||
There was a problem hiding this comment.
what was happening without this? I am seeing the same failure on trunk and on this branch. for eg testPartialDelete fails on list for /test/testPartialDelete/file-1/
There was a problem hiding this comment.
without this it was failing without createsession permissions. now its failing for s3: related IAM issues. progress
| } | ||
| // disable create session so there's no need to | ||
| // add a role policy for it. | ||
| disableCreateSession(conf); |
There was a problem hiding this comment.
same here, I didn't see a difference in failure for testDelegatedFileSystem with and without this.
There was a problem hiding this comment.
you should have got further
| } | ||
|
|
||
| /** | ||
| * Skip a test if the filesystem lacks a required capability. |
There was a problem hiding this comment.
nit: update javadoc, i think this will skip if it has a capability
| */ | ||
| public static void assumePathCapabilityFalse(FileSystem fs, String capability) { | ||
| try { | ||
| assume("Filesystem lacks capability " + capability, |
There was a problem hiding this comment.
think this should be "filesystem has capability"
| @@ -118,7 +121,10 @@ public void testRecreateTestBucketS3Express() throws Throwable { | |||
| fsURI)); | |||
| if (ex instanceof AWSBadRequestException) { | |||
| // owned error | |||
| assertExceptionContains(OWNED, ex); | |||
| if (!ex.getMessage().contains(OWNED) | |||
| && !ex.getMessage().contains(INVALID_LOCATION)) { | |||
There was a problem hiding this comment.
when does the INVALID_LOCATION get thrown?
There was a problem hiding this comment.
there's some hardcoded expectations about region and if you test somewhere else it blows up.
Change-Id: I85d1685358f6e220b78528e7bc3e2018bce60b5a
|
🎊 +1 overall
This message was automatically generated. |
Change-Id: I7a78752628375927a781d9d9f8da8e1ac9bb07a3
|
🎊 +1 overall
This message was automatically generated. |
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
…ble S3Express CreateSession (apache#6316) Adds a new option fs.s3a.s3express.create.session; default is true. When false, this disables the CreateSession call to create/refresh temporary session credentials when working with an Amazon S3 Express store. This avoids having to give the caller the new IAM role permission, at the expense of every remote call on the S3 Express store having to include the latency of a checkup of IAM permissions. * fs.s3a.s3express.create.session is set to false in tests which generate new role permissions and call AssumeRole * move ApiCallTimeoutException logic until after sdk exceptions get translated to IOE. This lines up for any future AWS throwing up underlying cause here. * Tests will automatically skip ACL, storage class, S3 Select or encryption tests when target fs is S3Express. * same for the out of order multipart uploader test cases, v1 listing. * bucket tool s3 test treats invalid location error as a successful invocation of the create bucket attempt Contributed by Steve Loughran
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.
HADOOP-18997
New option fs.s3a.s3express.create.session; default is true.
I'm still seeing more role tests failing, where permissions aren't quite right.
That is: I get past the create session failures but still getting read/list errors.
Not sure what is wrong there ... advice from others needed.
But a lot of the session and role tests are now working
How was this patch tested?
adding the option to role tests and verifying they now work without needing create session role statements added.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?