feat: IMDSv2 client #330

aajtodd · 2021-10-05T18:06:25Z

Issue #

closes #224

Description of changes

Adds a new IMDSv2 client (Ec2Metadata)
- Still left to do is handling retries and http timeouts. These will come in future PRs when the knobs are available
Relocated profile parsing and types to a new aws-config module and changed the package from aws.sdk.kotlin.runtime.config -> aws.sdk.kotlin.runtime.config.profile
Relocated several common test classes/types to smithy-kotlin such that they can be re-used in either runtime.

Scope

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

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

ianbotsf

Looks good, minor concerns only.

ianbotsf · 2021-10-06T16:38:39Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/EC2Metadata.kt

+    private val maxRetries: UInt = builder.maxRetries
+    private val endpointOverride: Endpoint? = builder.endpoint
+    private val endpointModeOverride: EndpointMode? = builder.endpointMode
+    private val tokenTTL: Duration = builder.tokenTTL


Style: tokenTTL → tokenTtl

ianbotsf · 2021-10-06T16:41:41Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/EC2Metadata.kt

+        if (endpointOverride != null && endpointModeOverride != null) {
+            logger.warn {
+                "EndpointMode was set in combination with an explicit endpoint. " +
+                    "The mode override will be ignored: endpointMode=$endpointModeOverride, endpoint=$endpointOverride"
+            }
+        }


Question: If only one of these should be set, is a union type a better fit?

sealed class EndpointConfiguration { object Default : EndpointConfiguration() data class Override(endpoint: Endpoint) : EndpointConfiguration() data class ModeOverride(mode: EndpointMode): EndpointConfiguration() }

I see these parameters are also reused in ImdsEndpointResolver which seems like further argument for type encapsulation.

yup that works for me, updated

ianbotsf · 2021-10-06T16:47:33Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/EC2Metadata.kt

+     * Example:
+     *
+     * ```kotlin
+     * val client = EC2Metadata()
+     * val amiId = client.get("/latest/meta-data/ami-id")
+     * ```


Comment: I like all the examples you're including in recent PRs. At some point, we should decide whether using the @sample KDoc tag is worthwhile. On the surface it seems like an excellent way to keep examples fresh/compiling as the code ages.

TIL.

Do you have an example of using the tag? Happy to update.

I don't necessarily think we should do it as part of this PR as it may be difficult to bootstrap initially but:

Here's an example of KDoc usage on String.replaceFirstChar and here's the sample to which it points.

(Note that this doesn't actually work in the IDE for stdlib right now because of KTIJ-8414.)

ianbotsf · 2021-10-06T17:07:01Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsEndpointResolver.kt

+internal class ImdsEndpointResolver(
+    private val platformProvider: PlatformProvider,
+    private val endpointModeOverride: EndpointMode? = null,
+    private val endpointOverride: Endpoint? = null,
+) : EndpointResolver {
+    // cached endpoint and profile
+    private var resolvedEndpoint: Endpoint? = null
+    private var cachedProfile: AwsProfile? = null
+
+    override suspend fun resolve(service: String, region: String): Endpoint = resolvedEndpoint ?: doResolveEndpoint()


Question: Why does this class implement EndpointResolver if it ignores the input arguments to resolve? What values could be passed in? Would any of them represent a fundamental misalignment that means the resolution should fail?

It implements EndpointResolver so that we can take advantage of ServiceEndpointResolver middleware (which will set the host, etc for us from a resolved endpoint).

We could forgo this and just duplicate the functionality if you find that preferable.

Hmm, actually I'm noticing that the StaticEnpointResolver ignores service and region and the EndpointResolverGenerator ignores service. My concern here is no worse than those examples so we can leave it as-is for now. But this does makes me question the interface of EndpointResolver.

Sure we can discuss that but IIRC the reason for the parameters is for the user to make a determination based on service and region. Not all resolvers need that information (e.g. the default resolver is generated per service so there is no need to use the service argument). A static resolver is well static so it's going to always return the same endpoint. A custom resolver may need that info is the primary driver.

ianbotsf · 2021-10-06T17:09:09Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsEndpointResolver.kt

+    private suspend fun doResolveEndpoint(): Endpoint {
+        val resolved = endpointOverride ?: resolveEndpointFromConfig()
+        return resolved.also { resolvedEndpoint = it }
+    }


Question: This field-level caching strategy is not thread-safe (applies also to cachedProfile and TokenMiddlware.cachedToken). Is concurrent access a concern? Would by lazy be a better fit here?

lazy doesn't work with suspend. Good call out though. Will fix.

ianbotsf · 2021-10-06T17:29:13Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/TokenMiddleware.kt

+
+    private suspend fun getToken(clock: Clock, req: SdkHttpRequest): Token {
+        val logger = req.context.getLogger("TokenMiddleware")
+        logger.trace { "refreshing IMDS token" }


Nit: This log message might be more useful if it included the values that led to us refreshing the token. For example: "Refreshing IMDS token because prior token expired at [timestamp] ([duration] ago)". (That might mean moving it elsewhere or refactoring...)

ianbotsf · 2021-10-06T18:17:07Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/profile/ContinuationMerger.kt

-package aws.sdk.kotlin.runtime.config
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0.
+ */
+
+package aws.sdk.kotlin.runtime.config.profile


Question: Curious...how did ktlint not catch this?

we don't use the same set of ktlint rules (the custom rules are only in smithy-kotlin)

ianbotsf · 2021-10-06T18:20:03Z

aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/profile/SpecTestSuites.kt

@@ -1,4 +1,4 @@
-package aws.sdk.kotlin.runtime.config
+package aws.sdk.kotlin.runtime.config.profile



Comment: Missing file header.

ianbotsf · 2021-10-06T18:33:29Z

aws-runtime/aws-core/common/src/aws/sdk/kotlin/runtime/AwsSdkSetting.kt

+            Boolean::class -> strValue.toBoolean()
+            else -> error("conversion to ${T::class} not implemented for AwsSdkSetting")
+        }
+        return typed as? T


Question: Why as? (nullable)? Isn't typed guaranteed to be castable to T at this point?

No, the resulting type is Any because all of the branches of when produce a different type. So we can also do as instead of as? if that is what you are asking. It won't matter in practice I think.

ianbotsf · 2021-10-06T18:34:17Z

aws-runtime/aws-core/common/src/aws/sdk/kotlin/runtime/AwsSdkSetting.kt

+public inline fun <reified T> AwsSdkSetting<T>.resolve(platform: PlatformEnvironProvider): T? {
+    val strValue = platform.getProperty(jvmProperty) ?: platform.getenv(environmentVariable)
+    if (strValue != null) {
+        val typed: Any = when (T::class) {
+            String::class -> strValue
+            Int::class -> strValue.toInt()
+            Long::class -> strValue.toLong()
+            Boolean::class -> strValue.toBoolean()
+            else -> error("conversion to ${T::class} not implemented for AwsSdkSetting")
+        }
+        return typed as? T
+    }
+    return defaultValue
+}


Question: Should checking for an incorrect type happen before getting strValue?

Not sure I follow...

Nevermind, I didn't fully read this function.

ianbotsf · 2021-10-07T18:20:01Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/CachedValue.kt

+ * @param bufferTime The amount of time before the actual expiration time when the value is considered expired. By default
+ * the buffer time is zero meaning the value expires at the expiration time. A non-zero buffer time means the value will
+ * expire BEFORE the actual expiration.


Question: Why do we need a buffer time? If that modifies the expiration to be an earlier expiration then why not just set expiresAt to something sooner?

We could do that too, this was an idea from Rust really that seemed like a useful abstraction to just encapsulate vs having to manually modify expirations wherever you might want a buffer. It also helps in debugging such that you can see the original expiration time vs some manually modified one.

ianbotsf · 2021-10-07T18:20:55Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/CachedValue.kt

+    /**
+     * Attempt to get the value or refresh it with [initializer] if it is expired
+     */
+    suspend fun getOrLoad(initializer: suspend () -> ExpiringValue<T>): T = mu.withLock {
+        if (!isExpiredUnlocked()) return@withLock value!!.value
+
+        val refreshed = initializer().also { value = it }
+        return refreshed.value
+    }


Question: Why provider a supplier at getOrLoad time vs in the constructor?

Hmm I suppose I can see it going either way

ianbotsf · 2021-10-07T18:25:59Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/CachedValue.kt

+@OptIn(ExperimentalTime::class)
+internal class CachedValue<T> (
+    private var value: ExpiringValue<T>? = null,
+    private val bufferTime: Duration = Duration.seconds(0),
+    private val clock: Clock = Clock.System
+) {


Comment: While I love that this is now encapsulated and I think it will have a lot of uses elsewhere, I wonder if the ergonomics can be improved. It would be delightful to be able to use a delegate such that callers could:

val latestFoo: Foo by expiringCache(Duration.seconds(60)) { Foo(…) }

Thoughts?

Delegates don't support suspend

(unless I'm missing something, because otherwise yes I would love for asyncLazy and this to work as a property delegate)

No you're probably right. I often expect suspend to be more baked into the language/stdlib than it actually is.

ianbotsf · 2021-10-07T18:31:50Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsEndpointResolver.kt

+    private val resolvedEndpoint = asyncLazy(::doResolveEndpoint)
+    private val activeProfile = asyncLazy { loadActiveAwsProfile(platformProvider) }


Question: Where is asyncLazy defined? I don't see it in this PR or in main.

It was added in the companion smithy-kotlin PR

kggilmer

Nice work, I especially like re-using the middleware facility for imds.

kggilmer · 2021-10-07T19:53:16Z

gradle.properties

 coroutinesVersion=1.5.1
 atomicFuVersion=0.16.1
-kotlinxSerializationVersion=0.20.0
+kotlinxSerializationVersion=1.3.0


question

big jump! was this necessary to support something in the PR or just general maintenance?

general maintenance. It looked odd that we were so far behind so I just bumped it

kggilmer · 2021-10-07T19:54:17Z

aws-runtime/testing/common/src/aws/sdk/kotlin/runtime/testing/TestPlatformProvider.kt

+    private val props: Map<String, String> = emptyMap(),
+    private val fs: Map<String, String> = emptyMap(),
+    private val os: OperatingSystem = OperatingSystem(OsFamily.Linux, "test")
+) : PlatformProvider, Filesystem by Filesystem.fromMap(fs.mapValues { it.value.encodeToByteArray() }) {


kggilmer · 2021-10-07T20:04:18Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/EC2Metadata.kt

+ * for more information.
+ */
+@OptIn(ExperimentalTime::class)
+public class EC2Metadata private constructor(builder: Builder) : Closeable {


style/suggestion

I would put a "Client" suffix on this since it's a client and not a metadata.

kneekey23 · 2021-10-20T04:02:28Z

aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsClient.kt

+     * Example:
+     *
+     * ```kotlin
+     * val client = EC2Metadata()


shouldnt this be val client = ImdsClient() ?

sigh yes...little late on the review though 😉

aajtodd added 14 commits September 28, 2021 16:51

feat(rt): bootstrap imds client

bc7e1ea

set unkonwn version

7d0d16a

refactor to use test utils from smithy-kotlin

2ec0efc

migrate test connection to smithy-kotlin for re-use

8dbe193

refactor: move profile configuration into aws-config

a816f87

move file missed in previous refactor

a0647f1

fix type conversion for AwsSdkSetting.resolve

61b3c01

remove mockk from common; use new interface for platform

59d033a

replace Platform singleton with PlatformProvider interface

a34e6e2

load endpoints from config and add test suite

7dc3ec5

Merge remote-tracking branch 'origin/main' into feat-imds-client

a022160

move profile parsing and types to subpackage

e4a27e1

ignore wip tests

fd0c78c

fix erroneous dependency relationship

1c7a96f

aajtodd mentioned this pull request Oct 5, 2021

refactor(rt): relocate and extend test utils smithy-lang/smithy-kotlin#489

Merged

aajtodd requested review from ianbotsf and kggilmer October 5, 2021 18:10

Merge remote-tracking branch 'origin/main' into feat-imds-client

96a26bf

ianbotsf approved these changes Oct 6, 2021

View reviewed changes

aajtodd added 2 commits October 7, 2021 11:56

thread safe cached value + pr feedback

bec6740

refactor endpoint configuration

d8e0cc9

ianbotsf reviewed Oct 7, 2021

View reviewed changes

kggilmer approved these changes Oct 7, 2021

View reviewed changes

refactor: rename to ImdsClient

4487bc2

aajtodd merged commit 4c86d31 into main Oct 7, 2021

aajtodd deleted the feat-imds-client branch October 7, 2021 20:40

kneekey23 reviewed Oct 20, 2021

View reviewed changes

		@@ -1,4 +1,4 @@
		package aws.sdk.kotlin.runtime.config
		package aws.sdk.kotlin.runtime.config.profile

		private val resolvedEndpoint = asyncLazy(::doResolveEndpoint)
		private val activeProfile = asyncLazy { loadActiveAwsProfile(platformProvider) }

feat: IMDSv2 client #330

feat: IMDSv2 client #330

Uh oh!

Conversation

aajtodd commented Oct 5, 2021

Issue #

Description of changes

Scope

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!

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!

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!

kggilmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment