HADOOP-19201 S3A. Support external-id in assume role #6876
Merged
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
Signed-off-by: Smith Cruise <[email protected]>
|
💔 -1 overall
This message was automatically generated. |
|
I didn't know about this -yes, it could be potentially useful
Now, first step to testing: look at the test policy. You have to run all the s3 tests against a S3 store of your choice, and as you are doing things with roles, that must include the assumed role suites. Once you've done that, tell us where and then it's time to work on the test design -which we can help with. https://hadoop.apache.org/docs/stable/hadoop-aws/tools/hadoop-aws/testing.html |
steveloughran
changed the title
Got it. I will do it when i am free. |
|
@Smith-Cruise this isn't going to get into 3.4.1 but if you can pick this up again we can target 3.4.2 |
|
tested this. it works If you can add a mention of this option in hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/assumed_roles.md then this is ready to merge. We don't want to add features which are only visible to people who looked at the source code... |
OK, I will add it to doc |
Signed-off-by: Smith Cruise <[email protected]>
I've updated the doc, PTAL |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
approved these changes
Aug 30, 2024
| @@ -153,6 +153,14 @@ Here are the full set of configuration options. | |||
| </description> | |||
| </property> | |||
|
|
|||
| <property> | |||
| <name>fs.s3a.assumed.role.external.id</name> | |||
| <value /> | |||
There was a problem hiding this comment.
can you an a valid example
There was a problem hiding this comment.
What do you mean? Put an example value here?
There was a problem hiding this comment.
I think putting an example here is meaningless, because external id is just a string set by the user, it can by any value. No unified format.
There was a problem hiding this comment.
so make that clear, e.g
<value>arbitrary value</value
hadoop-tools/hadoop-aws/src/site/markdown/tools/hadoop-aws/assumed_roles.md
Show resolved
Hide resolved
| @@ -1457,6 +1457,14 @@ | |||
| </description> | |||
| </property> | |||
|
|
|||
| <property> | |||
There was a problem hiding this comment.
lets actually cut this. Adding default values here increases the size of the configuration when marshalling, which happens a lot these days.
Signed-off-by: Smith Cruise <[email protected]>
|
💔 -1 overall
This message was automatically generated. |
Signed-off-by: Smith Cruise <[email protected]>
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran Can call anyone help to have a review? Thks! |
steveloughran
requested changes
Sep 9, 2024
There was a problem hiding this comment.
commented. code is all good.
However, we need test code -that is the only way we can verify that setting it doesn't break things. And for you to be sure nobody breaks it in future, inour code or the AWS SDK.
I'm going to propose you extend ITestAssumeRole slightly by
- In
RoleTestUtils.newAssumedRoleConfig()addASSUMED_ROLE_EXTERNAL_IDto the list of options to remove inremoveBaseAndBucketOverrides - In
ITestAssumeRole.testAssumeRoleCreateFS(), after the call tonewAssumedRoleConfig(), setASSUMED_ROLE_EXTERNAL_IDto some value, such asITestAssumeRole.testAssumeRoleCreateFS()itself.
Set up your hadoop-aws test configuration to include the role settings (see testing.md) and run that test. I'm running the tests to again, #7021 shows I'd accidentally stopped doing that. Once you have it working, tell us which s3 endpoint you ran against and I will retest the PR myself.
| @@ -153,6 +153,14 @@ Here are the full set of configuration options. | |||
| </description> | |||
| </property> | |||
|
|
|||
| <property> | |||
| <name>fs.s3a.assumed.role.external.id</name> | |||
| <value /> | |||
There was a problem hiding this comment.
so make that clear, e.g
<value>arbitrary value</value
Signed-off-by: Smith Cruise <[email protected]>
|
I'm curious about one thing, If I only tell you which endpoint I tested, how can you verify it? Do I need to provide ak/sk for you? |
|
💔 -1 overall
This message was automatically generated. |
steveloughran
pushed a commit
that referenced
this pull request
Sep 10, 2024
The option fs.s3a.assumed.role.external.id sets the external id for calls of AssumeRole to the STS service Contributed by Smith Cruise
|
merged to trunk and 3.4 branches; added the option to my account. lets see if any surprises show up. Though I know that not enough people are running the hadoop-aws test with the assume role options correctly set. |
Actually, we've already run it in StarRocks project, everything looks OK |
Hexiaoqiao
pushed a commit
to Hexiaoqiao/hadoop
that referenced
this pull request
Sep 12, 2024
The option fs.s3a.assumed.role.external.id sets the external id for calls of AssumeRole to the STS service Contributed by Smith Cruise
asfgit
pushed a commit
that referenced
this pull request
Sep 13, 2024
The option fs.s3a.assumed.role.external.id sets the external id for calls of AssumeRole to the STS service Contributed by Smith Cruise
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
Support external id in AssumedRoleCredentialProvider.java
https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_create_for-user_externalid.html
How was this patch tested?
tested in my env
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?