feat(rt): support static stability for IMDS credentials #719

lauzadis · 2022-10-05T21:41:20Z

Adds support for static stability for IMDS credentials.
Uses the expireCredentialsAfter time in CachedCredentialsProvider for credentials which have an expiration time set.

Issue #

#707
#718

Description of changes

#707:

This change is required to comply with AWS SDK standards. Specifically, this will support the use of expired IMDS credentials. This is useful because in the case of service degradation in IMDS which prevents credentials refresh, AWS will allow us to keep using the expired credentials.

When we use expired credentials, the CachedCredentialsProvider will continuously try to refresh them. To solve this, a new field called nextRefresh was added to the Credentials data class. When present, this will override any expiration times, and the providers will only try to refresh the credentials if the nextRefresh time has passed.

#718:

When receiving credentials with an expiration time, set the cached credentials' expiration time to the minimum of expireCredentialsAfter and the received expiration time. This will ensure we are refreshing credentials at the rate the user specifies.

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

…vice failures

lauzadis · 2022-10-05T21:43:35Z

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

        }

+        // if we have previously served IMDS credentials and it's not time for a refresh, just return the previous credentials
+        if (previousCredentials != null &&


Is there a nicer way to do this null check? Previously I had:

previousCredentials?.run { this.nextRefresh?.run { if (clock.now() < this) { return previousCredentials as Credentials } } }

and was torn between the two

I believe you could use the safe call operator and takeIf to clean this up slightly:

previousCredentials?.nextRefresh?.takeIf { Clock.now() < it }?.run { return previousCredentials as Credentials }

Why is the as Credentials cast necessary?

without as Credentials, I get this error: Smart cast to 'Credentials' is impossible, because 'previousCredentials' is a mutable property that could have been changed by this time

Another option that works is adding the non-null assertion !!

previousCredentials?.nextRefresh?.takeIf { clock.now() < it }?.run { return previousCredentials!! }

I will change it to that.

lauzadis · 2022-10-05T21:44:59Z

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

+                ex is IOException ||
+                    ex is EC2MetadataError && ex.statusCode == HttpStatusCode.InternalServerError.value -> {
+                    previousCredentials = previousCredentials?.copy(nextRefresh = clock.now() + DEFAULT_CREDENTIALS_REFRESH_SECONDS.seconds)
+                    return previousCredentials ?: throw CredentialsProviderException("failed to load instance profile", ex)


is there a nice way to remove this duplicated throw ...? (consolidating the right-half of the elvis operator and the else block)

Not given that the condition cannot easily be condensed into a when branch. You can either create a helper function for it or just leave the duplicated throw.

github-actions · 2022-10-05T21:46:21Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

ianbotsf · 2022-10-06T15:05:00Z

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

+                ex is IOException ||
+                    ex is EC2MetadataError && ex.statusCode == HttpStatusCode.InternalServerError.value -> {
+                    previousCredentials = previousCredentials?.copy(nextRefresh = clock.now() + DEFAULT_CREDENTIALS_REFRESH_SECONDS.seconds)
+                    return previousCredentials ?: throw CredentialsProviderException("failed to load instance profile", ex)


Not given that the condition cannot easily be condensed into a when branch. You can either create a helper function for it or just leave the duplicated throw.

ianbotsf · 2022-10-06T15:09:00Z

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

    private val profileOverride: String? = null,
    private val client: Lazy<InstanceMetadataProvider> = lazy { ImdsClient() },
    private val platformProvider: PlatformEnvironProvider = Platform,
+    private var previousCredentials: Credentials? = null,


Correctness: None of the handling around this field is thread-safe, leading to potentially inconsistent results when invoking on multiple threads simultaneously. I suggest you control access to this field with a mutex lock.

ianbotsf · 2022-10-06T16:50:36Z

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

+        // if we have previously served IMDS credentials and it's not time for a refresh, just return the previous credentials
+        if (previousCredentials != null &&
+            previousCredentials!!.nextRefresh != null &&
+            clock.now() < previousCredentials!!.nextRefresh!!
+        ) {
+            return previousCredentials as Credentials
+        }


Question: Is this the correct layer for this behavior? This means we'll effectively cache the credentials at this layer and not try fetching new ones until nextRefresh. That sounds correct for a caching layer but not what I'd necessarily expect from the direct IMDS provider.

Yeah this seems to duplicate the cache behavior does it not?

I think removing this would violate the guidelines for waiting 5-10 minutes between requests when the user has expired credentials and is using a direct IMDS provider, right?

This was added for the case when a user specifies a custom CredentialsProvider (i.e one consisting only of ImdsCredentialsProvider). If we remove this, then we will reach out to IMDS to get credentials for every HTTP request, even if the returned credentials are expired.

If we get expired credentials, we are supposed to wait 5-10 minutes before reaching out to IMDS again. This internal caching logic only runs when we have expired credentials (nextRefresh != null). If everything is operating normally, nextRefresh will remain null and there will be no caching.

Hmm I'm a little conflicted here. All of our providers work this way and would refresh on every HTTP request without a caching provider wrapping it. We even note this in some of the provider documentation.

On one hand I would sort of like to keep that clean separation, on the other I can understand your point. 🤔
@ianbotsf thoughts?

I think the logic of using cached credentials from within the ImdsCredentialsProvider is correct if we're in stability mode (because that's an implementation detail of how to use IMDS). If we're not in stability mode, I'd expect the provider to fetch fresh credentials every time getCredentials is called (just like every other provider does) and relegate regular caching to the CachedCredentialsProvider.

How complicated would that be to implement?

There is no way to tell static stability is enabled with 100% certainty, so we can't really create any logic around that:

IMDS doesn’t provide a formal flag, so the SDKs must always assume Static Stability is enabled.

One solution is to remove the nextRefresh logic from CachedCredentialsProvider entirely and leave the expired-credential caching solely in ImdsCredentialsProvider. In this scenario, when the default CachedCredentialsProvider is used and we are in static stability mode, the IMDS credentials will always appear as expired. To fetch credentials, we walk the chain of CredentialsProviders, and when we reach ImdsCredentialsProvider, we will branch into this caching logic and return the credentials.

One downside is that we will have to check the entire chain, which could be costly. But the added overhead could be fixed with this TODO where we would cache the credential provider.

Will we know we're in stability mode the first time we call IMDS and get either expired credentials or some type of service error?

I think I would be ok with that, it isolates all of this into the IMDS provider and if that is the only downside then we can fix the other TODO as you mentioned.

Which way are you leaning?

@ianbotsf We could but it would not be 100% certain. For example, there may have been a service error for some other reason (i.e there's an availability issue, but static stability is not enabled yet)

@aajtodd I am leaning towards the idea I proposed. One more possible downside is that we are adding a whole new field to Credentials (nextRefresh) just to be used in ImdsCredentialsProvider. Would it make sense to keep this as a member in ImdsCredentialsProvider instead of in the Credentials data class?

aajtodd

Looks great overall. Couple questions and callouts (in addition to Ian's callout on thread safety)

aajtodd · 2022-10-06T17:30:51Z

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

 import aws.smithy.kotlin.runtime.util.Platform
 import aws.smithy.kotlin.runtime.util.PlatformEnvironProvider
 import aws.smithy.kotlin.runtime.util.asyncLazy
+import java.io.IOException


fix: JVM only (not sure why this even compiles)

aajtodd · 2022-10-06T17:33:31Z

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

+        // if we have previously served IMDS credentials and it's not time for a refresh, just return the previous credentials
+        if (previousCredentials != null &&
+            previousCredentials!!.nextRefresh != null &&
+            clock.now() < previousCredentials!!.nextRefresh!!
+        ) {
+            return previousCredentials as Credentials
+        }


Yeah this seems to duplicate the cache behavior does it not?

aajtodd · 2022-10-06T17:37:18Z

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

+
+                if (creds.expiration!! < clock.now()) {
+                    // let the user know we will be using expired credentials
+                    logger.info { STATIC_STABILITY_LOG_MESSAGE.format(DEFAULT_CREDENTIALS_REFRESH_SECONDS / 60) }


fix: It looks like String.format is JVM only.

Also probably log at warn level?

aajtodd · 2022-10-06T17:40:34Z

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

+                    PROVIDER_NAME,
+                )
+
+                if (creds.expiration!! < clock.now()) {


Question: Is this condition sufficient or should it include that previousCredentials wasn't null as well? I suppose it would be odd that IMDS would be returning expired creds so maybe this is ok?

It is possible to receive expired credentials from IMDS when it's in static stability mode:

Test 1: SDK can use IMDS credential provider if first IMDS call returns expired credentials.

So you are proposing the conditional to be:

if (creds.expiration!! < clock.now() && previousCredentials != null)

I'm wondering what you think we should do if the incoming credentials are expired but previousCredentials is null? This can happen if we start an SDK client during an outage -- we will have no previousCredentials but the incoming credentials will be expired

This can happen if we start an SDK client during an outage

Hmm good point. That seems sufficient then

aajtodd · 2022-10-06T17:48:22Z

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

-                PROVIDER_NAME,
-            )
+            is JsonCredentialsResponse.SessionCredentials -> {
+                var creds = Credentials(


Suggestion: I think we can restructure this to remove the mutable creds variable and the copy, something like:

val nextRefresh = if (resp.expiration != null && resp.expiration < clock.now()) { logger.warn(...) clock.now() + DEFAULT_CREDENTIALS_REFRESH_SECONDS.seconds }else { null } return Credentials(..., nextRefresh = nextRefresh) .also { previousCredentials = it }

aajtodd · 2022-10-06T17:53:32Z

...ws-config/common/test/aws/sdk/kotlin/runtime/auth/credentials/ImdsCredentialsProviderTest.kt

+import io.mockk.spyk
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.runTest
+import java.net.SocketTimeoutException


fix: JVM only

...ws-config/common/test/aws/sdk/kotlin/runtime/auth/credentials/ImdsCredentialsProviderTest.kt

aajtodd · 2022-10-06T17:58:41Z

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

        val providerCreds = source.getCredentials()
        if (providerCreds.expiration != null) {
-            ExpiringValue(providerCreds, providerCreds.expiration!!)
+            val expirationTime = providerCreds.nextRefresh ?: minOf(providerCreds.expiration!!, (clock.now() + expireCredentialsAfter))


missed this in review: Missing test for this

aajtodd · 2022-10-06T18:00:21Z

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

+            when {
+                ex is IOException ||
+                    ex is EC2MetadataError && ex.statusCode == HttpStatusCode.InternalServerError.value -> {
+                    previousCredentials = previousCredentials?.copy(nextRefresh = clock.now() + DEFAULT_CREDENTIALS_REFRESH_SECONDS.seconds)


Another miss in my initial review.

Question: Should this be the DEFAULT_CREDENTIALS_REFRESH_SECONDS? I see our guidance is random between 5-10 minutes which would attempt to refresh a little more aggressively.

Yeah, the guidance is a random time in the 5-10 minute interval, "unless the SDK has a more appropriate default already defined"

I figured this DEFAULT_CREDENTIALS_REFRESH_SECONDS would be good to use since it's already well-defined but I can make it align closer to the guidance if you think it should be. To be fair the guidance gives some conflicting messages -- is it a "MUST" or "SHOULD"?

If a refresh returns expired credentials, an SDK MUST wait between 5-10 minutes before attempting another refresh.

The delay until the next refresh SHOULD be a uniformly random time in the interval of 5-10 minutes, unless the SDK has a more appropriate default already defined.

I think either is fine I was mostly just asking if we think we should be more aggressive here or not

github-actions · 2022-10-06T18:07:56Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-06T21:10:15Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

…atic-stability

github-actions · 2022-10-06T21:33:20Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-06T22:26:03Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-07T18:40:01Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-07T21:11:45Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-07T21:56:16Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-07T21:56:52Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-10T14:32:03Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-10T14:51:12Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-10T15:06:56Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

github-actions · 2022-10-10T16:25:17Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

aajtodd · 2022-10-10T18:41:06Z

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

 private const val CODE_ASSUME_ROLE_UNAUTHORIZED_ACCESS: String = "AssumeRoleUnauthorizedAccess"
 private const val PROVIDER_NAME = "IMDSv2"

+public expect class SdkIOException : Exception // FIXME move this to the proper place when we do the larger KMP Exception refactor


fix: This can be internal or private

When I switch this to internal or private, I get the following error in the JVM definition of SDKIOException:

Actual typealias 'SdkIOException' has no corresponding expected declaration The following declaration is incompatible because visibility is different: internal final expect class SdkIOException : Exception /* = Exception */ defined in aws.sdk.kotlin.runtime.auth.credentials in file ImdsCredentialsProvider.kt

I played around with it for a while and this expect seems to be required to be public

Expect and actual visibility has to match

They were both matching in this case -- I tried all combinations and it does not compile when both are set to internal. This seems to be a common issue: https://youtrack.jetbrains.com/issue/KT-37316

It can be fixed by adding @Suppress("ACTUAL_WITHOUT_EXPECT") decorator to the JVM implementation. I will go that route unless anybody is against it?

Ahh I see, because the type aliased actual differs in visibility. The suppression suggested seems fine then

aajtodd · 2022-10-10T18:41:48Z

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

    private val profileOverride: String? = null,
    private val client: Lazy<InstanceMetadataProvider> = lazy { ImdsClient() },
    private val platformProvider: PlatformEnvironProvider = Platform,
+    private var previousCredentials: Credentials? = null,


Question: Does this really need to be a constructor parameter?

It was nice for testing purposes, but not necessary. I will refactor it and update the test cases

aajtodd · 2022-10-10T19:12:50Z

...ws-config/common/test/aws/sdk/kotlin/runtime/auth/credentials/ImdsCredentialsProviderTest.kt

+        )
+
+        // call getCredentials 3 times
+        for (i in 1..3) {


nit: repeat(3) { ... }

aajtodd · 2022-10-10T19:38:48Z

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

        } catch (ex: Exception) {
-            throw CredentialsProviderException("failed to load instance profile", ex)
+            when {
+                ex is SdkIOException ||


Lines 85-92 are nearly same as 99-107, we could probably consolidate

e.g.

val profileName = try { ... } catch(ex :Exception) { return useCachedCredentials(ex) ?: throw CredentialsProviderException("failed to load instance profile", ex) } val payload = try { ... } catch(ex :Exception) { return useCachedCredentials(ex) ?: throw CredentialsProviderException("failed to load credentials", ex) } private fun useCachedCredentials(ex: Exception): Credentials? = when(ex) { is SdkIOException || is EC2MetadataError && ex.statusCode == HttpStatusCode.InternalServerError.value -> { mu.withLock { previousCredentials?.run { nextRefresh = clock.now() + ... } previousCredentials } } else -> null }

aajtodd · 2022-10-10T19:47:09Z

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

+    private val clock: Clock = Clock.System,
 ) : CredentialsProvider, Closeable {
    private val logger = Logger.getLogger<ImdsCredentialsProvider>()
+    private val mu = Mutex()


nit: You might clarify what this mutex protects:

e.g.

// protects previousCredentials + nextRefresh private val mu = Mutex()

… change visibility of SdkIOException to internal

github-actions · 2022-10-10T23:13:43Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

ianbotsf

Looks pretty good to me!

ianbotsf · 2022-10-11T21:12:46Z

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

+                previousCredentials?.run { nextRefresh = clock.now() + DEFAULT_CREDENTIALS_REFRESH_SECONDS.seconds }
+                return previousCredentials


Nit: I believe this can be simplified to:

previousCredentials?.apply { nextRefresh = clock.now() + DEFAULT_CREDENTIALS_REFRESH_SECONDS.seconds }

sonarqubecloud · 2022-10-11T21:23:03Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
11 Security Hotspots
5 Code Smells

No Coverage information
38.2% Duplication

github-actions · 2022-10-11T21:26:55Z

A new generated diff is ready to view: __generated-main...__generated-feat-static-stability

lauzadis added 5 commits October 5, 2022 14:52

use nextRefresh to set the credentials' expiration time

f7387b3

Allow use of expired credentials, add exception handling for IMDS ser…

98e9516

…vice failures

Add static stability test cases

f35aaf6

ktlint formatting

0a20654

add changelog

86f4035

lauzadis requested a review from a team as a code owner October 5, 2022 21:41

lauzadis commented Oct 5, 2022

View reviewed changes

lauzadis mentioned this pull request Oct 5, 2022

feat(rt): add nextRefresh field to Credentials for static stability support smithy-lang/smithy-kotlin#712

Closed

lauzadis marked this pull request as draft October 5, 2022 21:54

ianbotsf reviewed Oct 6, 2022

View reviewed changes

Use sanitized nextRefresh field

35283d7

aajtodd suggested changes Oct 6, 2022

View reviewed changes

aajtodd reviewed Oct 6, 2022

View reviewed changes

Set expireCredentialsAfter longer than the credentials expirationTime

3365e41

aajtodd reviewed Oct 6, 2022

View reviewed changes

lauzadis added 2 commits October 6, 2022 14:27

Add test for nextRefresh time taking priority over expiration time

d776b78

Add mutex for previousCredentials

f25571d

Merge branch 'main' of github.com:awslabs/aws-sdk-kotlin into feat-st…

cc9b26c

…atic-stability

lauzadis added 3 commits October 6, 2022 17:13

Remove usage of nextRefresh in Credentials

2a50708

Replace Credentials' nextRefresh with a class member

ec342cb

remove trailing comma

8cdff30

Add SdkIOException for multiplatform support of IOException

3b0be19

lauzadis marked this pull request as ready for review October 7, 2022 18:34

Remove microseconds from expiration time

a32db13

lauzadis added 2 commits October 7, 2022 16:50

Pass testClock into ImdsCredentialsProvider

7c82e9f

tickle CI

43a7af5

Address ktlint

6cced41

Remove microseconds from IMDS response

e4e35a1

Revert: remove microseconds from IMDS response

78eca44

Truncate expiration time to microseconds

a885300

lauzadis requested review from aajtodd and ianbotsf October 10, 2022 16:47

aajtodd reviewed Oct 10, 2022

View reviewed changes

lauzadis added 3 commits October 10, 2022 18:06

Change SdkIOException visibility to internal

17d9585

Remove previousCredentials from constructor and refactor its usage,…

004a2ee

… change visibility of SdkIOException to internal

ktlint

d668d4d

aajtodd approved these changes Oct 11, 2022

View reviewed changes

ianbotsf approved these changes Oct 11, 2022

View reviewed changes

Use .apply{} instead of .run{} to update nextRefresh

9c4b276

lauzadis merged commit 84468f6 into main Oct 11, 2022

lauzadis deleted the feat-static-stability branch October 11, 2022 21:51

This was referenced Oct 13, 2022

Obey the expireCredentialsAfter in CachedCredentialsProvider #718

Closed

Implement static stability support for credentials providers #707

Closed

		previousCredentials?.run { nextRefresh = clock.now() + DEFAULT_CREDENTIALS_REFRESH_SECONDS.seconds }
		return previousCredentials

feat(rt): support static stability for IMDS credentials #719

feat(rt): support static stability for IMDS credentials #719

Uh oh!

Conversation

lauzadis commented Oct 5, 2022

Issue #

Description of changes

#707:

#718:

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!

lauzadis Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lauzadis Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 5, 2022

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!

aajtodd 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!

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!

lauzadis Oct 6, 2022 •

edited

Loading

lauzadis Oct 5, 2022 •

edited

Loading