-
Notifications
You must be signed in to change notification settings - Fork 55
fix: credentials provider ownership #498
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
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
ca4483c
fix(rt): make AwsClientConfig implement Closeable
aajtodd 9705ed1
fix: close provider chain when owned
aajtodd 26a8e91
fix tests
aajtodd 60e14f4
fix grammar
aajtodd bfdb8be
feat(rt): allow explicit profile override from default chain
aajtodd 9ee2552
refactor!: remove AwsClientConfig
aajtodd e4d2d8a
refactor!: make PresignConfig required so it doesn't have to implemen…
aajtodd 578ee6a
Merge branch 'thread-provider-config' into fix-provider-ownership
aajtodd e0b7c1c
Merge branch 'feat-cred-providers' into fix-provider-ownership
aajtodd 64cc449
changed wrong config property
aajtodd 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 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
27 changes: 27 additions & 0 deletions
27
...ommon/src/aws/sdk/kotlin/runtime/auth/credentials/internal/BorrowedCredentialsProvider.kt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0. | ||
| */ | ||
|
|
||
| package aws.sdk.kotlin.runtime.auth.credentials.internal | ||
|
|
||
| import aws.sdk.kotlin.runtime.InternalSdkApi | ||
| import aws.sdk.kotlin.runtime.auth.credentials.CredentialsProvider | ||
| import aws.smithy.kotlin.runtime.io.Closeable | ||
|
|
||
| private class BorrowedCredentialsProvider( | ||
| private val borrowed: CredentialsProvider | ||
| ) : CredentialsProvider by borrowed, Closeable { | ||
| override fun close() { } | ||
| } | ||
|
|
||
| /** | ||
| * Wraps another [CredentialsProvider] with a no-op close implementation. This inserts a level of indirection for | ||
| * use cases when a provider is explicitly given to the SDK, and its ownership should remain with the caller. | ||
| * This allows the SDK to treat resources as owned and not have to track ownership state. | ||
| */ | ||
| @InternalSdkApi | ||
| public fun CredentialsProvider.borrow(): CredentialsProvider = when (this) { | ||
| is BorrowedCredentialsProvider -> this | ||
| else -> BorrowedCredentialsProvider(this) | ||
| } |
68 changes: 0 additions & 68 deletions
68
aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/AwsClientConfigLoader.kt
This file was deleted.
Oops, something went wrong.
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
28 changes: 0 additions & 28 deletions
28
...runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/AwsClientConfigLoaderTest.kt
This file was deleted.
Oops, something went wrong.
33 changes: 0 additions & 33 deletions
33
aws-runtime/aws-types/common/src/aws/sdk/kotlin/runtime/client/AwsClientConfig.kt
This file was deleted.
Oops, something went wrong.
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 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 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 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,6 +95,16 @@ open class AwsHttpProtocolClientGenerator( | |
| } | ||
| } | ||
|
|
||
| override fun renderClose(writer: KotlinWriter) { | ||
| writer.addImport(RuntimeTypes.IO.Closeable) | ||
| writer.write("") | ||
| .openBlock("override fun close() {") | ||
| .write("client.close()") | ||
| .write("(config.credentialsProvider as? #T)?.close()", RuntimeTypes.IO.Closeable) | ||
| .closeBlock("}") | ||
| .write("") | ||
| } | ||
|
Comment on lines
+98
to
+106
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment: Would be nice to have a new test for this. |
||
|
|
||
| private fun renderInternals() { | ||
| val endpointsData = javaClass.classLoader.getResource("aws/sdk/kotlin/codegen/endpoints.json")?.readText() ?: throw CodegenException("could not load endpoints.json resource") | ||
| val endpointData = Node.parse(endpointsData).expectObjectNode() | ||
|
|
||
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
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.
question
I'm missing something regarding
ClosableandCredentialsProvidertypes. The latter doesn't specify a close method, but theWrappedCredentialsProviderdoes combine the two, and looks here that we check to see if the cp implements closable and then call it if so. Given this would it not be simpler to have CP simply implementClosable? Put another way, what value are we gaining by having onlyBorrowedCredentialsProviderimplementClosable?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.
It isn't the only one that implements
Closeable. All of the following do:DefaultChainCredentialsProviderEcsCredentialsProviderImdsCredentialsProviderProfileCredentialsProviderEnvironment, StsWebIdentity, StsAssumeRole, and SSO providers do not implement
Closeable.The issue is that the type is just
CredentialsProviderboth here and inBorrowedCredentialsProviderso both have to check if the underlying implementation actually needs to be closed or not. I have gone back and forth on this and considered makingCredentialsProviderimplementCloseablebut ultimately decided against it to be more clear about what types do and don't need to be closed when used directly by a customer. I could be persuaded the other way though if you want to make a case for it.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.
Personally I'm not sure what's best either way, but it may be more satisfying of "We ensure accessing AWS services is performant, secure, and reliable for developers" if
CredentialProviderimplementsClosable. I reckon this may be so because if closability varies, intuitively it seems likely that someone may forget that a particular CP needs to be closed. Or perhaps a function that takes a non closing CP is passed a requires-closing CP during a refactor but the nuance is dropped in the refactor. If all CPs must be closed (with some of them being NOP), it seems to be less likely that this kind of bug would occur. This comes at the expense of greater overall complexity of the CP type, however the value may outweigh the cost. @ianbotsf any thoughts on this?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'm in favor of explicitly modelling closability. If it can/must be closed, it should be
Closeable. Otherwise, I don't think we should unnecessarily attach closing semantics to theCredentialProviderinterface because some of the implementations are closeable.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.
Ya I can see both sides here and I hear the concern. I'm going to leave it as is for now and lets see how it shakes out.