HADOOP-19004. S3A: Support Authentication through HttpSigner API #6324
Conversation
This file contains hidden or 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
|
This patch is from @HarshitGupta11; I've added my wrapping changes to it so its going to have yetus and steve happy |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ahmarsuhail
requested changes
Dec 7, 2023
There was a problem hiding this comment.
This looks good mostly.
But we are now configuring signers twice -
Once in AwsClientConfig.createClientConfigBuilder() and here. If someone has fs.s3a.custom.signers and fs.s3a.http.signer.enabled, I don't know what happens.
The way I was thinking about doing this was removing all the current code we have for configuring Signers and only having the new auth scheme stuff. Also use the existing config names instead of adding new ones. The old Signer interface has been deprecated I think, and that interface was a breaking change with V2 anyway, so should be able to update to new AuthSchemes only.
Is this enough for providing S3Express auth scheme support too? From what I saw before the Auth scheme needs to implement S3ExpressAuthScheme. So we'll need to check if the custom Signer is for S3Express. Maybe adding a test for S3Express will be useful too. Also will need documentation, this is all a bit confusing now and easy to use the the wrong Signer type with S3Express and miss out on the fast createSession auth.
I am happy to merge this once the createSession() comment is addressed, and I can do all of the above in a follow up PR.
| .build(); | ||
| } | ||
|
|
||
| public CreateSessionResponse createSessionInternal(CreateSessionRequest createSessionRequest){ |
There was a problem hiding this comment.
This is unused, consider removing. if it is for internal use only, move to S3AInternals? There's also no corresponding method for it RequestFactory.
There was a problem hiding this comment.
aah, that's why the imports are still obsolete...will cut
Move to the new auth flow based signers for aws. * Implement a new Signer Initialization Chain * Add a new instantiation method * Add a new test * Fix Reflection Code for SignerInitialization (+steve changes) * move instantiation code into SignerFactory * CustomHttpSigner goes into production code for broader testing * some tuning of property values and names of constants/methods. Change-Id: I8f34af4bd958d631bcb03d7c073ed1da01cb8205
Change-Id: Ibd1e708e5ff6eb3f7e055153e4d199c945650d2c
* Remove unused method in s3afs * put static imports in DefaultS3ClientFactory in right order Change-Id: Ib5125d5e1a373835bac5f90efed42d0947cd1ba5
steveloughran
force-pushed
the
s3/HADOOP-19004-httpsigner
branch
from
ab04508 to
c6542d8
Compare
|
|
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
commented
Jan 1, 2024
| import software.amazon.awssdk.services.s3.S3Client; | ||
| import software.amazon.awssdk.services.s3.model.CompleteMultipartUploadRequest; | ||
| import software.amazon.awssdk.services.s3.model.CompleteMultipartUploadResponse; | ||
| import software.amazon.awssdk.services.s3.model.CreateSessionRequest; |
There was a problem hiding this comment.
why these imports
There was a problem hiding this comment.
oh, left in when I cut its use; will fix and retest
Change-Id: I5d590b110d8667371eff449154c602e6fd420728
|
🎊 +1 overall
This message was automatically generated. |
Change-Id: Ifcb9274dcb0124f5dc2f9a5acf6cb0c7b5280bfe
|
🎊 +1 overall
This message was automatically generated. |
|
OK, checkstyle fixed, +1, merging (remember, this is Harshit's patch, I've just been doing the wrap-up) |
asfgit
pushed a commit
that referenced
this pull request
Jan 22, 2024
Move to the new auth flow based signers for aws. * Implement a new Signer Initialization Chain * Add a new instantiation method * Add a new test * Fix Reflection Code for SignerInitialization Contributed by Harshit Gupta
slfan1989
pushed a commit
that referenced
this pull request
Jan 25, 2024
Move to the new auth flow based signers for aws. * Implement a new Signer Initialization Chain * Add a new instantiation method * Add a new test * Fix Reflection Code for SignerInitialization Contributed by Harshit Gupta
jiajunmao
pushed a commit
to jiajunmao/hadoop-MLEC
that referenced
this pull request
Feb 6, 2024
…che#6324) Move to the new auth flow based signers for aws. * Implement a new Signer Initialization Chain * Add a new instantiation method * Add a new test * Fix Reflection Code for SignerInitialization Contributed by Harshit Gupta
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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-19004. S3A: Support Authentication through HttpSigner API
Move to the new auth flow based signers for aws. * Implement a new Signer Initialization Chain
(+steve changes)
How was this patch tested?
new test
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?