fix: credentials provider ownership #498

aajtodd · 2022-01-11T16:46:06Z

Issue #

N/A

Description of changes

Adds a borrow() extension that wraps an existing provider with a no-op close implementation
Close the client config credentialsProvider in the service client's close implementation (if it was borrowed it will do nothing, otherwise it will close it)
~~Implement Closeable for shared AwsClientConfig~~
(breaking): remove AwsClientConfig and associated constructor
(breaking): make credentialsProvider required for PresignConfig (otherwise it needs to be Closeable because it was instantiating a default chain provider)
(refactor): inline region resolution into Client.fromEnvironment() and update it's signature to use Client.Config.Builder instead of AwsClientLoadOptions
(refactor): thread profileName through DefaultCredentialProviderChain constructor. Make constructor with httpClientEngine override public.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

github-actions · 2022-01-11T16:50:23Z

A new generated diff is ready to view: __generated-main...__generated-fix-provider-ownership

github-actions · 2022-01-11T17:06:43Z

A new generated diff is ready to view: __generated-main...__generated-fix-provider-ownership

ianbotsf

I like the general direction of this, it's nice and clean!

ianbotsf · 2022-01-11T17:30:59Z

...g/common/src/aws/sdk/kotlin/runtime/auth/credentials/internal/BorrowedCredentialsProvider.kt

+ * 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 it's ownership should remain with the caller.
+ * This allows the SDK to treat resources as owned and not have to track ownership state.


Nit: it's → its

ianbotsf · 2022-01-11T17:34:43Z

aws-runtime/aws-types/common/src/aws/sdk/kotlin/runtime/client/AwsClientConfig.kt

+    // provide default no-op close
+    override fun close() { }
+


Comment: This seems dangerous. Given the implications of forgetting to close something, it seems like there should be no default close and implementors should have to make explicit decisions about closing.

We could do that but every generated Config implements AwsClientConfig and so we would have to generate a close() method there.

We could do that and just do config.close() instead in the generated client close implementation?

It would be nice to express a way to say that a property represents a closeable resource I guess and have the config generator automatically render it in the close implementation.

It would be nice to express a way to say that a property represents a closeable resource I guess and have the config generator automatically render it in the close implementation.

+1. Seems like going this way we'd be able to verify that a given codegen output cleans up resources as expected in CI

ianbotsf · 2022-01-11T17:36:31Z

...egen/src/main/kotlin/aws/sdk/kotlin/codegen/protocols/core/AwsHttpProtocolClientGenerator.kt

+    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: Would be nice to have a new test for this.

kggilmer · 2022-01-11T19:40:55Z

aws-runtime/aws-types/common/src/aws/sdk/kotlin/runtime/client/AwsClientConfig.kt

+    // provide default no-op close
+    override fun close() { }
+


It would be nice to express a way to say that a property represents a closeable resource I guess and have the config generator automatically render it in the close implementation.

+1. Seems like going this way we'd be able to verify that a given codegen output cleans up resources as expected in CI

…t Closeable

github-actions · 2022-01-19T15:33:37Z

A new generated diff is ready to view: __generated-main...__generated-fix-provider-ownership

github-actions · 2022-01-19T15:39:11Z

A new generated diff is ready to view: __generated-main...__generated-fix-provider-ownership

sonarqubecloud · 2022-01-19T16:21:59Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.8% Duplication

github-actions · 2022-01-19T16:24:44Z

A new generated diff is ready to view: __generated-main...__generated-fix-provider-ownership

kggilmer

Question regarding Closable and CredentialsProvider but nothing to warrant blocking the PR

kggilmer · 2022-01-21T22:01:06Z

...egen/src/main/kotlin/aws/sdk/kotlin/codegen/protocols/core/AwsHttpProtocolClientGenerator.kt

+        writer.write("")
+            .openBlock("override fun close() {")
+            .write("client.close()")
+            .write("(config.credentialsProvider as? #T)?.close()", RuntimeTypes.IO.Closeable)


question

I'm missing something regarding Closable and CredentialsProvider types. The latter doesn't specify a close method, but the WrappedCredentialsProvider does 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 implement Closable? Put another way, what value are we gaining by having only BorrowedCredentialsProvider implement Closable?

what value are we gaining by having only BorrowedCredentialsProvider implement Closable

It isn't the only one that implements Closeable. All of the following do:

DefaultChainCredentialsProvider

EcsCredentialsProvider

ImdsCredentialsProvider

ProfileCredentialsProvider

Environment, StsWebIdentity, StsAssumeRole, and SSO providers do not implement Closeable.

The issue is that the type is just CredentialsProvider both here and in BorrowedCredentialsProvider so 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 making CredentialsProvider implement Closeable but 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.

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 CredentialProvider implements Closable. 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?

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 the CredentialProvider interface because some of the implementations are closeable.

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.

…ible (#469) Refactor credential providers to remove CRT dependency and make them KMP compatible. Added SSO provider to default chain. Lots of misc cleanup and improvements. * feat(rt): standalone sso credentials provider (#462) * refactor(rt)!: generated sts and sts web identity credential providers (#470) * refactor(rt)!: implement kmp ecs provider (#475) * feat(rt)!: implement kmp profile credentials provider (#478) * feat(rt)!: kmp default credentials provider chain (#491) * fix: work around machine-specific Gradle bug with aws-config variants (#496) * fix: credentials provider ownership (#498) Co-authored-by: Ian Botsford <83236726+ianbotsf@users.noreply.github.com>

aajtodd added 2 commits January 10, 2022 14:58

fix(rt): make AwsClientConfig implement Closeable

ca4483c

fix: close provider chain when owned

9705ed1

aajtodd requested a review from a team as a code owner January 11, 2022 16:46

aajtodd mentioned this pull request Jan 11, 2022

feat(codegen): allow custom rendering of config properties smithy-lang/smithy-kotlin#568

Merged

aajtodd requested review from ianbotsf and kggilmer January 11, 2022 16:46

fix tests

26a8e91

ianbotsf requested changes Jan 11, 2022

View reviewed changes

kggilmer approved these changes Jan 11, 2022

View reviewed changes

aajtodd added 5 commits January 11, 2022 16:53

fix grammar

60e14f4

feat(rt): allow explicit profile override from default chain

bfdb8be

refactor!: remove AwsClientConfig

9ee2552

refactor!: make PresignConfig required so it doesn't have to implemen…

e4d2d8a

…t Closeable

Merge branch 'thread-provider-config' into fix-provider-ownership

578ee6a

aajtodd requested a review from ianbotsf January 19, 2022 15:33

aajtodd requested a review from kggilmer January 19, 2022 15:33

Merge branch 'feat-cred-providers' into fix-provider-ownership

e0b7c1c

changed wrong config property

64cc449

ianbotsf approved these changes Jan 19, 2022

View reviewed changes

kggilmer approved these changes Jan 21, 2022

View reviewed changes

aajtodd merged commit 6da1456 into feat-cred-providers Jan 27, 2022

aajtodd deleted the fix-provider-ownership branch January 27, 2022 00:59

ianbotsf mentioned this pull request Jun 1, 2022

How-to guides show outdated client config #620

Closed

fix: credentials provider ownership #498

fix: credentials provider ownership #498

Uh oh!

Conversation

aajtodd commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue #

Description of changes

Uh oh!

github-actions bot commented Jan 11, 2022

Uh oh!

github-actions bot commented Jan 11, 2022

Uh oh!

ianbotsf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 19, 2022

Uh oh!

github-actions bot commented Jan 19, 2022

Uh oh!

sonarqubecloud bot commented Jan 19, 2022

Uh oh!

github-actions bot commented Jan 19, 2022

Uh oh!

kggilmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aajtodd commented Jan 11, 2022 •

edited

Loading