feat(rt)!: kmp default credentials provider chain #491

aajtodd · 2022-01-05T20:12:20Z

Issue #

N/A

Description of changes

Re-implement the default chain in Kotlin
Add unit tests adapted from the Rust test suite covering e2e behavior of the default chain
- The tests are all a directory in a particular convention:
  - fs/* is the resulting test filesystem rooted at fs
  - test-case.json is the expected result of the test (specific to default chain tests)
  - env.json is the test environment variables to set
  - http-traffic.json is the request/response pair(s) that the engine expects/responds with (there are limitations currently in what can be asserted about the requests but the tests are setup that asserting the request isn't all that important. You'll either get the correct credentials or you wont)
Fixed SSO response parsing discovered as part of creating the test covering sso in the default chain (response is epoch milliseconds not epoch seconds)
Refactored constructors to all use consistent names for the same argument types
Relocated the region/credential chain implementations to be in aws-config to get the right exception type(s)

Notes

There are still some todos/fixmes. I will follow up with additional smaller PRs to round out the credential provider overhaul. In particular we need to hash out resource management and probably thread through profile override and HttpClientEngine from shared config.

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-05T20:16:27Z

A new generated diff is ready to view: __generated-main...__generated-feat-default-provider

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

A new generated diff is ready to view: __generated-main...__generated-feat-default-provider

kggilmer

Those test cases are gold. Nice work.

kggilmer · 2022-01-06T21:49:24Z

...me/aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/CredentialsProviderChain.kt

+    }
+
+    override fun close() {
+        providers.forEach { (it as? Closeable)?.close() }


concern

in the case that a CredentialsProvider.close() throws an exception, we may end up with unclosed CPs. Suggest wrapping the close in a try to address.

Agreed. Will fix

What should the behavior be? Should we rethrow the first? Add all as suppressed exceptions?

Something like this?

override fun close() { var closeEx: Throwable? = null providers.forEach { try { (it as? Closeable)?.close() }catch(ex: Exception) { if (closeEx == null) { closeEx = ex }else { closeEx!!.addSuppressed(ex) } } if (closeEx != null) throw closeEx!! } }

Since the throw happens inside the loop, won't that still lead to unclosed providers?

I think we want something like:

val exceptions = providers.mapNotNull { try { (it as? Closeable)?.close() null } catch (t: Throwable) { t } } if (exceptions.isNotEmpty()) { val t = exceptions.first() exceptions.drop(1).forEach(t::addSuppressed) throw t }

err whoops the throw should be outside the loop. Oddly enough I had something similar to what you are suggesting locally but opted for not allocating a list. Since you suggested it though I'm assuming you'd be in favor of it anyway. Unless anyone has an issue with it I'll just take Ian's suggestion.

kggilmer · 2022-01-06T21:58:17Z

...nfig/jvm/test/aws/sdk/kotlin/runtime/auth/credentials/DefaultChainCredentialsProviderTest.kt

+import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+
+// TODO - refactor to make this work in common


question

regarding making this non-jvm, I don't see a way of doing this without platform specific file system abstractions...is there another way?

The idea would be like a kmp version of loadResource but it requires build script support. I've seen an example in the wild but yeah it's not straightforward.

ianbotsf · 2022-01-06T22:21:31Z

aws-runtime/aws-config/build.gradle.kts


-                // credential providers
-                implementation("aws.sdk.kotlin.crt:aws-crt-kotlin:$crtKotlinVersion")
-                implementation(project(":aws-runtime:crt-util"))


Question: Can any code be removed from crt-util now?

Maybe haven't checked. A lot of crt-util supports signing

I don't see anything that should be removed at a glance

ianbotsf · 2022-01-06T22:24:14Z

...aws-config/common/src/aws/sdk/kotlin/runtime/auth/credentials/CrtBasedCredentialsProvider.kt

-internal interface CrtCredentialsProvider : CredentialsProvider {
-    val crtProvider: CredentialsProviderCrt
-
-    override suspend fun getCredentials(): Credentials = crtProvider.getCredentials().toSdk()
-}


Comment: Seems like a lot of related code can be removed from aws-crt-kotlin as well.

We would need to discuss the plan for this. Just because SDK doesn't use them doesn't mean crt-kotlin can't support them (it is after all supposed to be bindings for crt for the language not SDK specific necessarily)

ianbotsf · 2022-01-06T22:35:16Z

...config/common/src/aws/sdk/kotlin/runtime/auth/credentials/DefaultChainCredentialsProvider.kt

+    override fun close() {
+        chain.close()
+        if (manageEngine) {
+            httpClientEngine.close()
        }
    }


Comment: Maybe this is part of the open changes to make regarding ownership but I'd expect a CachedCredentialsProvider to be Closeable and to close its underlying provider when closed.

Good point. Cached provider takes ownership so that should just be fixed here probably

ianbotsf · 2022-01-06T22:38:34Z

...config/common/src/aws/sdk/kotlin/runtime/auth/credentials/DefaultChainCredentialsProvider.kt

+
+    private val manageEngine = httpClientEngine == null
+    private val httpClientEngine = httpClientEngine ?: CrtHttpEngine()
+
+    public constructor() : this(Platform)
+
+    private val chain = CredentialsProviderChain(


Nit: The secondary constructor seems like it belongs right after the primary constructor definition, not in between private vals.

…e throwing

github-actions · 2022-01-07T14:17:00Z

A new generated diff is ready to view: __generated-main...__generated-feat-default-provider

sonarqubecloud · 2022-01-10T14:33:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

No Coverage information
0.0% Duplication

github-actions · 2022-01-10T14:36:54Z

A new generated diff is ready to view: __generated-main...__generated-feat-default-provider

…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 12 commits January 5, 2022 14:59

refactor(rt)!: implement kmp default chain; import test suite

416ad72

fix web identity tests

8288a75

rm crt providers

2d3e1a0

cleanup docs

733851a

style: make field name consistent across providers

0837b24

fix sso expiration

703ba11

improve debug logging for region resolution

9d9567e

add sso test from working capture

7dcd197

fix http engine lifetime in default cred chain

0498bbe

refactor: relocate provider chains to aws-config

8b5a867

fix credential provider chain exception

9ada835

cache region after resolving it

a625ef2

aajtodd requested a review from a team as a code owner January 5, 2022 20:12

aajtodd mentioned this pull request Jan 5, 2022

feat(rt): allow nullable LazyAsyncValue; add test utils for constructing http traffic from JSON smithy-lang/smithy-kotlin#561

Merged

aajtodd requested review from ianbotsf and kggilmer January 5, 2022 20:16

import sts retry on error test case

a675de6

kggilmer approved these changes Jan 6, 2022

View reviewed changes

ianbotsf approved these changes Jan 6, 2022

View reviewed changes

close cached provider source; ensure chain is completely closed befor…

c4bd3a2

…e throwing

fix chain close

1559c56

aajtodd merged commit 1e98d70 into feat-cred-providers Jan 10, 2022

aajtodd deleted the feat-default-provider branch January 10, 2022 15:56

feat(rt)!: kmp default credentials provider chain #491

feat(rt)!: kmp default credentials provider chain #491

Uh oh!

Conversation

aajtodd commented Jan 5, 2022

Issue #

Description of changes

Notes

Uh oh!

github-actions bot commented Jan 5, 2022

Uh oh!

github-actions bot commented Jan 6, 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!

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!

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 7, 2022

Uh oh!

sonarqubecloud bot commented Jan 10, 2022

Uh oh!

github-actions bot commented Jan 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants