-
Notifications
You must be signed in to change notification settings - Fork 55
feat: implement recursion detection middleware #602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
d8e797b
feat: set recursion detection header
lucix-aws 247364a
fix traceid env
lucix-aws c639bd3
move percentEncode into middleware, since it's SEP-specific
lucix-aws cc56086
cleanup codegen piece
lucix-aws 03fde7a
remove unused import
lucix-aws 647aadd
consume new percentEncodeTo
lucix-aws df73fa1
changelog
lucix-aws bafb05c
update %encode rules per published SEP
lucix-aws 822c1c3
remove dangerous chars
lucix-aws File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| { | ||
| "id": "2d52d36d-564b-4e31-a7fa-14d8839d4a96", | ||
| "type": "feature", | ||
| "description": "Implement recursion detection middleware." | ||
| } |
56 changes: 56 additions & 0 deletions
56
aws-runtime/aws-http/common/src/aws/sdk/kotlin/runtime/http/middleware/RecursionDetection.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package aws.sdk.kotlin.runtime.http.middleware | ||
|
|
||
| import aws.sdk.kotlin.runtime.InternalSdkApi | ||
| import aws.smithy.kotlin.runtime.http.operation.ModifyRequestMiddleware | ||
| import aws.smithy.kotlin.runtime.http.operation.SdkHttpRequest | ||
| import aws.smithy.kotlin.runtime.util.EnvironmentProvider | ||
| import aws.smithy.kotlin.runtime.util.Platform | ||
| import aws.smithy.kotlin.runtime.util.text.percentEncodeTo | ||
|
|
||
| internal const val ENV_FUNCTION_NAME = "AWS_LAMBDA_FUNCTION_NAME" | ||
| internal const val ENV_TRACE_ID = "_X_AMZN_TRACE_ID" | ||
| internal const val HEADER_TRACE_ID = "X-Amzn-Trace-Id" | ||
|
|
||
| /** | ||
| * HTTP middleware to add the recursion detection header where required. | ||
| */ | ||
| @InternalSdkApi | ||
| public class RecursionDetection( | ||
| private val env: EnvironmentProvider = Platform | ||
| ) : ModifyRequestMiddleware { | ||
| override suspend fun modifyRequest(req: SdkHttpRequest): SdkHttpRequest { | ||
| if (req.subject.headers.contains(HEADER_TRACE_ID)) return req | ||
|
|
||
| val traceId = env.getenv(ENV_TRACE_ID) | ||
| if (env.getenv(ENV_FUNCTION_NAME) == null || traceId == null) return req | ||
|
|
||
| req.subject.headers[HEADER_TRACE_ID] = traceId.percentEncode() | ||
| return req | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Percent-encode ISO control characters for the purposes of this specific header. | ||
| * | ||
| * The existing `Char::isISOControl` check cannot be used here, because that matches against characters in | ||
| * `[0x00, 0x1f] U [0x7f, 0x9f]`. The SEP for recursion detection dictates we should only encode across | ||
| * `[0x00, 0x1f]`. | ||
| */ | ||
| private fun String.percentEncode(): String { | ||
| val sb = StringBuilder(this.length) | ||
| val data = this.encodeToByteArray() | ||
| for (cbyte in data) { | ||
| val chr = cbyte.toInt().toChar() | ||
| if (chr.code in 0x00..0x1f) { | ||
| cbyte.percentEncodeTo(sb) | ||
| } else { | ||
| sb.append(chr) | ||
| } | ||
| } | ||
| return sb.toString() | ||
| } |
143 changes: 143 additions & 0 deletions
143
...ime/aws-http/common/test/aws/sdk/kotlin/runtime/http/middleware/RecursionDetectionTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package aws.sdk.kotlin.runtime.http.middleware | ||
|
|
||
| import aws.sdk.kotlin.runtime.testing.TestPlatformProvider | ||
| import aws.smithy.kotlin.runtime.client.ExecutionContext | ||
| import aws.smithy.kotlin.runtime.http.Headers | ||
| import aws.smithy.kotlin.runtime.http.HttpBody | ||
| import aws.smithy.kotlin.runtime.http.HttpStatusCode | ||
| import aws.smithy.kotlin.runtime.http.engine.HttpClientEngineBase | ||
| import aws.smithy.kotlin.runtime.http.operation.* | ||
| import aws.smithy.kotlin.runtime.http.request.HttpRequest | ||
| import aws.smithy.kotlin.runtime.http.request.HttpRequestBuilder | ||
| import aws.smithy.kotlin.runtime.http.response.HttpCall | ||
| import aws.smithy.kotlin.runtime.http.response.HttpResponse | ||
| import aws.smithy.kotlin.runtime.http.sdkHttpClient | ||
| import aws.smithy.kotlin.runtime.time.Instant | ||
| import aws.smithy.kotlin.runtime.util.get | ||
| import kotlinx.coroutines.ExperimentalCoroutinesApi | ||
| import kotlinx.coroutines.test.runTest | ||
| import kotlin.test.Test | ||
| import kotlin.test.assertEquals | ||
| import kotlin.test.assertFalse | ||
|
|
||
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| class RecursionDetectionTest { | ||
| private class TraceHeaderSerializer( | ||
| private val traceHeader: String | ||
| ) : HttpSerialize<Unit> { | ||
| override suspend fun serialize(context: ExecutionContext, input: Unit): HttpRequestBuilder { | ||
| val builder = HttpRequestBuilder() | ||
| builder.headers[HEADER_TRACE_ID] = traceHeader | ||
| return builder | ||
| } | ||
| } | ||
|
|
||
| private val mockEngine = object : HttpClientEngineBase("test") { | ||
| override suspend fun roundTrip(request: HttpRequest): HttpCall { | ||
| val resp = HttpResponse(HttpStatusCode.fromValue(200), Headers.Empty, HttpBody.Empty) | ||
| val now = Instant.now() | ||
| return HttpCall(request, resp, now, now) | ||
| } | ||
| } | ||
|
|
||
| private val client = sdkHttpClient(mockEngine) | ||
|
|
||
| private suspend fun test( | ||
| env: Map<String, String>, | ||
| existingTraceHeader: String?, | ||
| expectedTraceHeader: String? | ||
| ) { | ||
| val op = SdkHttpOperation.build<Unit, HttpResponse> { | ||
| serializer = if (existingTraceHeader != null) TraceHeaderSerializer(existingTraceHeader) else UnitSerializer | ||
| deserializer = IdentityDeserializer | ||
| context { | ||
| service = "Test Service" | ||
| operationName = "testOperation" | ||
| } | ||
| } | ||
|
|
||
| val provider = TestPlatformProvider(env) | ||
| op.install(RecursionDetection(provider)) | ||
| op.roundTrip(client, Unit) | ||
|
|
||
| val request = op.context[HttpOperationContext.HttpCallList].last().request | ||
| if (expectedTraceHeader != null) { | ||
| assertEquals(expectedTraceHeader, request.headers[HEADER_TRACE_ID]) | ||
| } else { | ||
| assertFalse(request.headers.contains(HEADER_TRACE_ID)) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `it noops if env unset`() = runTest { | ||
| test( | ||
| emptyMap(), | ||
| null, | ||
| null | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `it sets header when both envs are present`() = runTest { | ||
| test( | ||
| mapOf( | ||
| ENV_FUNCTION_NAME to "some-function", | ||
| ENV_TRACE_ID to "Root=1-5759e988-bd862e3fe1be46a994272793;Parent=53995c3f42cd8ad8;Sampled=1;lineage=a87bd80c:0,68fd508a:5,c512fbe3:2" | ||
| ), | ||
| null, | ||
| "Root=1-5759e988-bd862e3fe1be46a994272793;Parent=53995c3f42cd8ad8;Sampled=1;lineage=a87bd80c:0,68fd508a:5,c512fbe3:2" | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `it noops if trace env set but no lambda env`() = runTest { | ||
| test( | ||
| mapOf( | ||
| ENV_TRACE_ID to "Root=1-5759e988-bd862e3fe1be46a994272793;Parent=53995c3f42cd8ad8;Sampled=1;lineage=a87bd80c:0,68fd508a:5,c512fbe3:2" | ||
| ), | ||
| null, | ||
| null | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `it respects existing trace header`() = runTest { | ||
| test( | ||
| mapOf( | ||
| ENV_FUNCTION_NAME to "some-function", | ||
| ENV_TRACE_ID to "EnvValue" | ||
| ), | ||
| "OriginalValue", | ||
| "OriginalValue" | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `it url encodes new trace header`() = runTest { | ||
| test( | ||
| mapOf( | ||
| ENV_FUNCTION_NAME to "some-function", | ||
| ENV_TRACE_ID to "first\nsecond" | ||
| ), | ||
| null, | ||
| "first%0Asecond" | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `ignores other chars that are usually percent encoded`() = runTest { | ||
| test( | ||
| mapOf( | ||
| ENV_FUNCTION_NAME to "some-function", | ||
| ENV_TRACE_ID to "test123-=;:+&[]{}\"'" | ||
| ), | ||
| null, | ||
| "test123-=;:+&[]{}\"'" | ||
| ) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
31 changes: 31 additions & 0 deletions
31
...c/main/kotlin/aws/sdk/kotlin/codegen/protocols/middleware/RecursionDetectionMiddleware.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package aws.sdk.kotlin.codegen.protocols.middleware | ||
|
|
||
| import aws.sdk.kotlin.codegen.AwsKotlinDependency | ||
| import software.amazon.smithy.kotlin.codegen.core.KotlinWriter | ||
| import software.amazon.smithy.kotlin.codegen.model.buildSymbol | ||
| import software.amazon.smithy.kotlin.codegen.model.namespace | ||
| import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolGenerator | ||
| import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolMiddleware | ||
| import software.amazon.smithy.model.shapes.OperationShape | ||
|
|
||
| /** | ||
| * HTTP middleware to add the recursion detection header where required. | ||
| */ | ||
| class RecursionDetectionMiddleware : ProtocolMiddleware { | ||
| override val name: String = "RecursionDetection" | ||
| override val order: Byte = 30 | ||
|
|
||
| private val middlewareSymbol = buildSymbol { | ||
| name = "RecursionDetection" | ||
| namespace(AwsKotlinDependency.AWS_HTTP, subpackage = "middleware") | ||
| } | ||
|
|
||
| override fun render(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) { | ||
| writer.write("op.install(#T())", middlewareSymbol) | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: This middleware doesn't override
isEnabledFor. Just confirming...we want this to run for every operation in every AWS service?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats a good question, it could be limited to only services that have an integration that causes recursion if we know that set. Otherwise it would have to be every service I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per post-standup discussion, we'll leave it on every request since