-
Notifications
You must be signed in to change notification settings - Fork 863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SRA] Rules-based authentication resolvers #3540
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to get feedback on this approach I took: The SRA docs mention we should code generate the mapping from auth scheme parameters to endpoint resolver parameters, however that was a lot of duplication for S3. By calling the generated resolver instead, we'll use all endpoint parameters anyways (including new ones we might add in the future):
aws-sdk-net/sdk/src/Services/S3/Generated/Internal/AmazonS3EndpointResolver.cs
Lines 70 to 82 in 36af043
One benefit is that it allowed me to keep the implementation to retrieve auth schemes from an endpoint (in
sdk/src/Core/Amazon.Runtime/Pipeline/Handlers/BaseAuthResolverHandler.cs
) simpler.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would rerunning the endpoint resolver twice work when the SDK supports account id based endpoint rules? Because we would need the identity to get the account id for the endpoint rules I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do need to run endpoint rules twice and it will use the exact same parameters we could stash the results in
executionContext.RequestContext.ContextAttributes
and then have the endpoint resolver check for the existence and if so reuse those results.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the internal design doc (that recommended running the EP2.0 resolver twice), there was a comment on not allowing S3 to use the
AWS::Auth::AccountId
parameter in rules that have authentication schemes.I'll follow up to see if that was ever built or if it's something we should do in our generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the other comment, I thought about populating the existing
EndpointAttributes
property:aws-sdk-net/sdk/src/Core/Amazon.Runtime/Internal/IRequest.cs
Line 388 in ad13fd4
But it looks like when the endpoints resolver calls
GetEndpoint
it calculates them again anyway (this constructor is called by the generated code):aws-sdk-net/sdk/src/Core/Amazon.Runtime/Endpoints/Endpoint.cs
Lines 45 to 49 in ad13fd4
Or did you mean I should cache the entire endpoint? (I don't think that's the case since we'd need to change whenever we add support for account-ID based endpoints).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding was running the endpoint rules twice would only work if we the service didn't use account id as part of the rule. In that case would we need to change this when the SDK supports account id based rules? So yes I was suggesting caching the results within the scope of the current request in stead of running the endpoint rules twice. But maybe I'm misunderstanding something and the second time could cause a different results in some scenarios.