From cd061bc95cee41f2b5f9117b2cfa040d34eb0869 Mon Sep 17 00:00:00 2001 From: Aaron J Todd Date: Fri, 8 Oct 2021 13:31:10 -0400 Subject: [PATCH 1/7] refactor(rt): define CRT engine config and consume common settings --- .../kotlin/runtime/config/imds/ImdsClient.kt | 11 ++-- .../runtime/http/engine/crt/CrtHttpEngine.kt | 44 ++++++++++---- .../http/engine/crt/CrtHttpEngineConfig.kt | 57 +++++++++++++++++++ .../http/engine/crt/AsyncStressTest.kt | 5 +- 4 files changed, 99 insertions(+), 18 deletions(-) create mode 100644 aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngineConfig.kt diff --git a/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsClient.kt b/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsClient.kt index 38cf7eb9374..c066876945d 100644 --- a/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsClient.kt +++ b/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsClient.kt @@ -17,7 +17,6 @@ import aws.sdk.kotlin.runtime.http.middleware.UserAgent import aws.smithy.kotlin.runtime.client.ExecutionContext import aws.smithy.kotlin.runtime.http.* import aws.smithy.kotlin.runtime.http.engine.HttpClientEngine -import aws.smithy.kotlin.runtime.http.engine.HttpClientEngineConfig import aws.smithy.kotlin.runtime.http.operation.* import aws.smithy.kotlin.runtime.http.response.HttpResponse import aws.smithy.kotlin.runtime.io.Closeable @@ -57,8 +56,15 @@ public class ImdsClient private constructor(builder: Builder) : Closeable { private val tokenTtl: Duration = builder.tokenTTL private val clock: Clock = builder.clock private val platformProvider: PlatformProvider = builder.platformProvider + private val httpClient: SdkHttpClient init { + val engine = builder.engine ?: CrtHttpEngine { + connectTimeout = Duration.seconds(1) + } + + httpClient = sdkHttpClient(engine) + // validate the override at construction time if (endpointConfiguration is EndpointConfiguration.Custom) { val url = endpointConfiguration.endpoint.toUrl() @@ -70,9 +76,6 @@ public class ImdsClient private constructor(builder: Builder) : Closeable { } } - // TODO connect/socket timeouts - private val httpClient = sdkHttpClient(builder.engine ?: CrtHttpEngine(HttpClientEngineConfig())) - // cached middleware instances private val middleware: List = listOf( ServiceEndpointResolver.create { diff --git a/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt index 40d28280427..f1b112a273a 100644 --- a/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt +++ b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt @@ -7,10 +7,10 @@ package aws.sdk.kotlin.runtime.http.engine.crt import aws.sdk.kotlin.crt.http.* import aws.sdk.kotlin.crt.io.* +import aws.sdk.kotlin.runtime.ClientException import aws.sdk.kotlin.runtime.crt.SdkDefaultIO import aws.smithy.kotlin.runtime.http.engine.HttpClientEngine import aws.smithy.kotlin.runtime.http.engine.HttpClientEngineBase -import aws.smithy.kotlin.runtime.http.engine.HttpClientEngineConfig import aws.smithy.kotlin.runtime.http.engine.callContext import aws.smithy.kotlin.runtime.http.request.HttpRequest import aws.smithy.kotlin.runtime.http.response.HttpCall @@ -18,33 +18,55 @@ import aws.smithy.kotlin.runtime.time.Instant import kotlinx.coroutines.job import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock +import kotlinx.coroutines.withTimeoutOrNull +import kotlin.time.ExperimentalTime internal const val DEFAULT_WINDOW_SIZE_BYTES: Int = 16 * 1024 /** * [HttpClientEngine] based on the AWS Common Runtime HTTP client */ -public class CrtHttpEngine(public val config: HttpClientEngineConfig) : HttpClientEngineBase("crt") { - // FIXME - use the default TLS context when profile cred provider branch is merged - private val tlsCtx = TlsContext(TlsContextOptions.defaultClient()) +public class CrtHttpEngine(public val config: CrtHttpEngineConfig) : HttpClientEngineBase("crt") { + public constructor() : this(CrtHttpEngineConfig.Default) + public companion object { + public operator fun invoke(block: CrtHttpEngineConfig.Builder.() -> Unit): CrtHttpEngine = CrtHttpEngine(CrtHttpEngineConfig.Builder().apply(block).build()) + } + + private val customTlsContext: TlsContext? = if (config.alpn.isNotEmpty() && config.tlsContext == null) { + val options = TlsContextOptionsBuilder().apply { + verifyPeer = true + alpn = config.alpn.joinToString(separator = ";") + }.build() + TlsContext(options) + } else { + null + } + + @OptIn(ExperimentalTime::class) private val options = HttpClientConnectionManagerOptionsBuilder().apply { - clientBootstrap = SdkDefaultIO.ClientBootstrap - tlsContext = tlsCtx + clientBootstrap = config.clientBootstrap ?: SdkDefaultIO.ClientBootstrap + tlsContext = customTlsContext ?: config.tlsContext ?: SdkDefaultIO.TlsContext manualWindowManagement = true - socketOptions = SocketOptions() - initialWindowSize = DEFAULT_WINDOW_SIZE_BYTES - // TODO - max connections/timeouts/etc + socketOptions = SocketOptions( + connectTimeoutMs = config.connectTimeout.inWholeMilliseconds.toInt() + ) + initialWindowSize = config.initialWindowSizeBytes + maxConnections = config.maxConnections.toInt() + maxConnectionIdleMs = config.connectionIdleTimeout.inWholeMilliseconds } // connection managers are per host private val connManagers = mutableMapOf() private val mutex = Mutex() + @OptIn(ExperimentalTime::class) override suspend fun roundTrip(request: HttpRequest): HttpCall { val callContext = callContext() val manager = getManagerForUri(request.uri) - val conn = manager.acquireConnection() + val conn = withTimeoutOrNull(config.connectionAcquireTimeout) { + manager.acquireConnection() + } ?: throw ClientException("timed out waiting for an HTTP connection to be acquired from the pool") try { val reqTime = Instant.now() @@ -78,7 +100,7 @@ public class CrtHttpEngine(public val config: HttpClientEngineConfig) : HttpClie // close all resources // SAFETY: shutdown is only invoked once AND only after all requests have completed and no more are coming connManagers.forEach { entry -> entry.value.close() } - tlsCtx.close() + customTlsContext?.close() } private suspend fun getManagerForUri(uri: Uri): HttpClientConnectionManager = mutex.withLock { diff --git a/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngineConfig.kt b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngineConfig.kt new file mode 100644 index 00000000000..37084325e58 --- /dev/null +++ b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngineConfig.kt @@ -0,0 +1,57 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0. + */ + +package aws.sdk.kotlin.runtime.http.engine.crt + +import aws.sdk.kotlin.crt.io.ClientBootstrap +import aws.sdk.kotlin.crt.io.TlsContext +import aws.smithy.kotlin.runtime.http.engine.HttpClientEngineConfig +import aws.smithy.kotlin.runtime.util.InternalApi + +@InternalApi +public class CrtHttpEngineConfig private constructor(builder: Builder) : HttpClientEngineConfig(builder) { + public companion object { + /** + * The default engine config. Most clients should use this. + */ + public val Default: CrtHttpEngineConfig = CrtHttpEngineConfig(Builder()) + } + + /** + * The amount of data that can be buffered before reading from the socket will cease. Reading will + * resume as data is consumed. + */ + public val initialWindowSizeBytes: Int = builder.initialWindowSizeBytes + + /** + * The [ClientBootstrap] to use for the engine. By default it is a shared instance. + */ + public var clientBootstrap: ClientBootstrap? = builder.clientBootstrap + + /** + * The TLS context to use. By default it is a shared instance. + */ + public var tlsContext: TlsContext? = builder.tlsContext + + public class Builder : HttpClientEngineConfig.Builder() { + /** + * Set the amount of data that can be buffered before reading from the socket will cease. Reading will + * resume as data is consumed. + */ + public var initialWindowSizeBytes: Int = DEFAULT_WINDOW_SIZE_BYTES + + /** + * Set the [ClientBootstrap] to use for the engine. By default it is a shared instance. + */ + public var clientBootstrap: ClientBootstrap? = null + + /** + * Set the TLS context to use. By default it is a shared instance. + */ + public var tlsContext: TlsContext? = null + + internal fun build(): CrtHttpEngineConfig = CrtHttpEngineConfig(this) + } +} diff --git a/aws-runtime/http-client-engine-crt/jvm/test/aws/sdk/kotlin/runtime/http/engine/crt/AsyncStressTest.kt b/aws-runtime/http-client-engine-crt/jvm/test/aws/sdk/kotlin/runtime/http/engine/crt/AsyncStressTest.kt index 71953aaa5d3..dbc54d375ec 100644 --- a/aws-runtime/http-client-engine-crt/jvm/test/aws/sdk/kotlin/runtime/http/engine/crt/AsyncStressTest.kt +++ b/aws-runtime/http-client-engine-crt/jvm/test/aws/sdk/kotlin/runtime/http/engine/crt/AsyncStressTest.kt @@ -8,7 +8,6 @@ package aws.sdk.kotlin.runtime.http.engine.crt import aws.sdk.kotlin.runtime.testing.runSuspendTest import aws.smithy.kotlin.runtime.http.HttpMethod import aws.smithy.kotlin.runtime.http.Protocol -import aws.smithy.kotlin.runtime.http.engine.HttpClientEngineConfig import aws.smithy.kotlin.runtime.http.readAll import aws.smithy.kotlin.runtime.http.request.HttpRequestBuilder import aws.smithy.kotlin.runtime.http.request.url @@ -43,7 +42,7 @@ class AsyncStressTest : TestWithLocalServer() { @Test fun testConcurrentRequests() = runSuspendTest { // https://github.com/awslabs/aws-sdk-kotlin/issues/170 - val client = sdkHttpClient(CrtHttpEngine(HttpClientEngineConfig())) + val client = sdkHttpClient(CrtHttpEngine()) val request = HttpRequestBuilder().apply { url { scheme = Protocol.HTTP @@ -78,7 +77,7 @@ class AsyncStressTest : TestWithLocalServer() { // appropriately and allows requests to proceed (a stream that isn't consumed will be in a stuck state // if the window is full and never incremented again, this can lead to all connections being consumed // and the engine to no longer make further requests) - val client = sdkHttpClient(CrtHttpEngine(HttpClientEngineConfig())) + val client = sdkHttpClient(CrtHttpEngine()) val request = HttpRequestBuilder().apply { url { scheme = Protocol.HTTP From 0e129d0fb74f98ffc24db71ee1e023fa95d70d83 Mon Sep 17 00:00:00 2001 From: Aaron J Todd Date: Fri, 8 Oct 2021 14:55:18 -0400 Subject: [PATCH 2/7] use concrete alpn type --- .../src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt index f1b112a273a..20fb3c464d9 100644 --- a/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt +++ b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt @@ -36,7 +36,7 @@ public class CrtHttpEngine(public val config: CrtHttpEngineConfig) : HttpClientE private val customTlsContext: TlsContext? = if (config.alpn.isNotEmpty() && config.tlsContext == null) { val options = TlsContextOptionsBuilder().apply { verifyPeer = true - alpn = config.alpn.joinToString(separator = ";") + alpn = config.alpn.joinToString(separator = ";") { it.protocolId } }.build() TlsContext(options) } else { From 85d20a91d8730144aaf05d302ef3e2fd6a762171 Mon Sep 17 00:00:00 2001 From: Aaron J Todd Date: Fri, 8 Oct 2021 15:12:59 -0400 Subject: [PATCH 3/7] add warning about read and write timeouts --- .../sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt index 20fb3c464d9..7a36987e838 100644 --- a/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt +++ b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt @@ -14,6 +14,7 @@ import aws.smithy.kotlin.runtime.http.engine.HttpClientEngineBase import aws.smithy.kotlin.runtime.http.engine.callContext import aws.smithy.kotlin.runtime.http.request.HttpRequest import aws.smithy.kotlin.runtime.http.response.HttpCall +import aws.smithy.kotlin.runtime.logging.Logger import aws.smithy.kotlin.runtime.time.Instant import kotlinx.coroutines.job import kotlinx.coroutines.sync.Mutex @@ -26,12 +27,14 @@ internal const val DEFAULT_WINDOW_SIZE_BYTES: Int = 16 * 1024 /** * [HttpClientEngine] based on the AWS Common Runtime HTTP client */ +@OptIn(ExperimentalTime::class) public class CrtHttpEngine(public val config: CrtHttpEngineConfig) : HttpClientEngineBase("crt") { public constructor() : this(CrtHttpEngineConfig.Default) public companion object { public operator fun invoke(block: CrtHttpEngineConfig.Builder.() -> Unit): CrtHttpEngine = CrtHttpEngine(CrtHttpEngineConfig.Builder().apply(block).build()) } + private val logger = Logger.getLogger() private val customTlsContext: TlsContext? = if (config.alpn.isNotEmpty() && config.tlsContext == null) { val options = TlsContextOptionsBuilder().apply { @@ -43,6 +46,11 @@ public class CrtHttpEngine(public val config: CrtHttpEngineConfig) : HttpClientE null } + init { + logger.warn { "CrtHttpEngine does not support HttpClientEngineConfig.socketReadTimeout(${config.socketReadTimeout}; ignoring" } + logger.warn { "CrtHttpEngine does not support HttpClientEngineConfig.socketWriteTimeout(${config.socketWriteTimeout}; ignoring" } + } + @OptIn(ExperimentalTime::class) private val options = HttpClientConnectionManagerOptionsBuilder().apply { clientBootstrap = config.clientBootstrap ?: SdkDefaultIO.ClientBootstrap From 74ac876c8524c9f12fc29b8c1debbd4d272ebdc8 Mon Sep 17 00:00:00 2001 From: Aaron J Todd Date: Mon, 11 Oct 2021 11:15:51 -0400 Subject: [PATCH 4/7] implement imds timeout test --- .../kotlin/runtime/config/imds/ImdsClient.kt | 1 + .../runtime/config/imds/ImdsClientTest.kt | 22 +++++++++++++++---- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsClient.kt b/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsClient.kt index c066876945d..db97da289c2 100644 --- a/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsClient.kt +++ b/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/imds/ImdsClient.kt @@ -61,6 +61,7 @@ public class ImdsClient private constructor(builder: Builder) : Closeable { init { val engine = builder.engine ?: CrtHttpEngine { connectTimeout = Duration.seconds(1) + socketReadTimeout = Duration.seconds(1) } httpClient = sdkHttpClient(engine) diff --git a/aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/imds/ImdsClientTest.kt b/aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/imds/ImdsClientTest.kt index 85b3349a5b0..76220b44e16 100644 --- a/aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/imds/ImdsClientTest.kt +++ b/aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/imds/ImdsClientTest.kt @@ -16,8 +16,10 @@ import aws.smithy.kotlin.runtime.http.request.url import aws.smithy.kotlin.runtime.http.response.HttpResponse import aws.smithy.kotlin.runtime.httptest.TestConnection import aws.smithy.kotlin.runtime.httptest.buildTestConnection +import aws.smithy.kotlin.runtime.time.Instant import aws.smithy.kotlin.runtime.time.ManualClock import io.kotest.matchers.string.shouldContain +import kotlinx.coroutines.withTimeout import kotlinx.serialization.json.* import kotlin.test.* import kotlin.time.Duration @@ -203,11 +205,23 @@ class ImdsClientTest { fail("not implemented yet") } - @Ignore @Test - fun testHttpConnectTimeouts() { - // Need a 1 sec connect timeout + other timeouts in imds spec - fail("not implemented yet") + fun testHttpConnectTimeouts(): Unit = runSuspendTest { + // end-to-end real client times out after 1-second + val client = ImdsClient { + // will never resolve + endpointConfiguration = EndpointConfiguration.Custom("http://240.0.0.0".toEndpoint()) + } + + val start = Instant.now() + assertFails { + withTimeout(3000) { + client.get("/latest/metadata") + } + }.message.shouldContain("timed out") + val elapsed = Instant.now().epochSeconds - start.epochSeconds + assertTrue(elapsed >= 1) + assertTrue(elapsed < 2) } data class ImdsConfigTest( From 66f9c832cf65c63eccaf1fb38c660e31df62a047 Mon Sep 17 00:00:00 2001 From: Aaron J Todd Date: Mon, 11 Oct 2021 11:17:45 -0400 Subject: [PATCH 5/7] fix template --- .github/ISSUE_TEMPLATE/feature_request.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md index e13a8b1794c..67ead712bbe 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.md +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -30,7 +30,7 @@ labels: enhancement, needs-triage ## Additional Context - + From fcd948a17eb66798fc4807d71fbf49de387fb3f8 Mon Sep 17 00:00:00 2001 From: Aaron J Todd Date: Mon, 11 Oct 2021 14:27:59 -0400 Subject: [PATCH 6/7] fix log message --- .../aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt index 7a36987e838..4dcace93e42 100644 --- a/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt +++ b/aws-runtime/http-client-engine-crt/common/src/aws/sdk/kotlin/runtime/http/engine/crt/CrtHttpEngine.kt @@ -47,8 +47,8 @@ public class CrtHttpEngine(public val config: CrtHttpEngineConfig) : HttpClientE } init { - logger.warn { "CrtHttpEngine does not support HttpClientEngineConfig.socketReadTimeout(${config.socketReadTimeout}; ignoring" } - logger.warn { "CrtHttpEngine does not support HttpClientEngineConfig.socketWriteTimeout(${config.socketWriteTimeout}; ignoring" } + logger.warn { "CrtHttpEngine does not support HttpClientEngineConfig.socketReadTimeout(${config.socketReadTimeout}); ignoring" } + logger.warn { "CrtHttpEngine does not support HttpClientEngineConfig.socketWriteTimeout(${config.socketWriteTimeout}); ignoring" } } @OptIn(ExperimentalTime::class) From f4d650c83e66b11bb79672ffd57889dbcb3522f3 Mon Sep 17 00:00:00 2001 From: Aaron J Todd Date: Mon, 11 Oct 2021 14:49:13 -0400 Subject: [PATCH 7/7] see if timeout is truncated --- .../aws/sdk/kotlin/runtime/config/imds/ImdsClientTest.kt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/imds/ImdsClientTest.kt b/aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/imds/ImdsClientTest.kt index 76220b44e16..52c3fa202e3 100644 --- a/aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/imds/ImdsClientTest.kt +++ b/aws-runtime/aws-config/common/test/aws/sdk/kotlin/runtime/config/imds/ImdsClientTest.kt @@ -18,6 +18,7 @@ import aws.smithy.kotlin.runtime.httptest.TestConnection import aws.smithy.kotlin.runtime.httptest.buildTestConnection import aws.smithy.kotlin.runtime.time.Instant import aws.smithy.kotlin.runtime.time.ManualClock +import aws.smithy.kotlin.runtime.time.epochMilliseconds import io.kotest.matchers.string.shouldContain import kotlinx.coroutines.withTimeout import kotlinx.serialization.json.* @@ -219,9 +220,9 @@ class ImdsClientTest { client.get("/latest/metadata") } }.message.shouldContain("timed out") - val elapsed = Instant.now().epochSeconds - start.epochSeconds - assertTrue(elapsed >= 1) - assertTrue(elapsed < 2) + val elapsed = Instant.now().epochMilliseconds - start.epochMilliseconds + assertTrue(elapsed >= 1000) + assertTrue(elapsed < 2000) } data class ImdsConfigTest(