From d665873b3ebee66d49c014d92dcffb212efa3673 Mon Sep 17 00:00:00 2001 From: 0marperez Date: Wed, 11 Dec 2024 16:46:30 -0500 Subject: [PATCH] PR feedback --- aws-runtime/aws-config/api/aws-config.api | 5 +- .../ResolveFlexibleChecksumsConfig.kt | 42 ++++++ .../ResolveRequestChecksumCalculation.kt | 29 ----- .../ResolveResponseChecksumValidation.kt | 29 ----- aws-runtime/aws-http/api/aws-http.api | 23 ++++ .../S3CompositeChecksumsInterceptor.kt | 47 +++++++ .../S3CompositeChecksumsInterceptorTest.kt | 35 +++++ .../aws/sdk/kotlin/codegen/AwsRuntimeTypes.kt | 1 + .../FlexibleChecksumsResponse.kt | 8 -- .../s3/S3CompositeChecksumsIntegration.kt | 44 +++++++ .../s3/express/S3ExpressIntegration.kt | 16 ++- ...tlin.codegen.integration.KotlinIntegration | 1 + .../S3ExpressDisableChecksumInterceptor.kt | 19 ++- .../s3/express/ChecksumRemovalTest.kt | 34 ++--- services/s3/e2eTest/src/S3ChecksumTest.kt | 120 ++++++++++++------ services/s3/e2eTest/src/S3IntegrationTest.kt | 2 +- services/s3/e2eTest/src/S3TestUtils.kt | 35 +++-- tests/codegen/build.gradle.kts | 1 - .../codegen/checksums/ChecksumConfigTest.kt | 35 +---- .../checksums/utils/ChecksumTestUtils.kt | 11 +- 20 files changed, 356 insertions(+), 181 deletions(-) create mode 100644 aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveFlexibleChecksumsConfig.kt delete mode 100644 aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveRequestChecksumCalculation.kt delete mode 100644 aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveResponseChecksumValidation.kt create mode 100644 aws-runtime/aws-http/common/src/aws/sdk/kotlin/runtime/http/interceptors/S3CompositeChecksumsInterceptor.kt create mode 100644 aws-runtime/aws-http/common/test/aws/sdk/kotlin/runtime/http/interceptors/S3CompositeChecksumsInterceptorTest.kt create mode 100644 codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/flexiblechecksums/s3/S3CompositeChecksumsIntegration.kt diff --git a/aws-runtime/aws-config/api/aws-config.api b/aws-runtime/aws-config/api/aws-config.api index 6a9be2adba5..1d5c135a3ae 100644 --- a/aws-runtime/aws-config/api/aws-config.api +++ b/aws-runtime/aws-config/api/aws-config.api @@ -262,12 +262,9 @@ public final class aws/sdk/kotlin/runtime/config/AwsSdkSettingKt { public static final fun resolveEndpointUrl (Laws/sdk/kotlin/runtime/config/AwsSdkSetting;Laws/smithy/kotlin/runtime/util/PlatformProvider;Ljava/lang/String;Ljava/lang/String;)Laws/smithy/kotlin/runtime/net/url/Url; } -public final class aws/sdk/kotlin/runtime/config/checksums/ResolveRequestChecksumCalculationKt { +public final class aws/sdk/kotlin/runtime/config/checksums/ResolveFlexibleChecksumsConfigKt { public static final fun resolveRequestChecksumCalculation (Laws/smithy/kotlin/runtime/util/PlatformProvider;Laws/smithy/kotlin/runtime/util/LazyAsyncValue;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; public static synthetic fun resolveRequestChecksumCalculation$default (Laws/smithy/kotlin/runtime/util/PlatformProvider;Laws/smithy/kotlin/runtime/util/LazyAsyncValue;Lkotlin/coroutines/Continuation;ILjava/lang/Object;)Ljava/lang/Object; -} - -public final class aws/sdk/kotlin/runtime/config/checksums/ResolveResponseChecksumValidationKt { public static final fun resolveResponseChecksumValidation (Laws/smithy/kotlin/runtime/util/PlatformProvider;Laws/smithy/kotlin/runtime/util/LazyAsyncValue;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; public static synthetic fun resolveResponseChecksumValidation$default (Laws/smithy/kotlin/runtime/util/PlatformProvider;Laws/smithy/kotlin/runtime/util/LazyAsyncValue;Lkotlin/coroutines/Continuation;ILjava/lang/Object;)Ljava/lang/Object; } diff --git a/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveFlexibleChecksumsConfig.kt b/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveFlexibleChecksumsConfig.kt new file mode 100644 index 00000000000..bf7049ef2f3 --- /dev/null +++ b/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveFlexibleChecksumsConfig.kt @@ -0,0 +1,42 @@ +package aws.sdk.kotlin.runtime.config.checksums + +import aws.sdk.kotlin.runtime.ConfigurationException +import aws.sdk.kotlin.runtime.InternalSdkApi +import aws.sdk.kotlin.runtime.config.AwsSdkSetting +import aws.sdk.kotlin.runtime.config.profile.AwsProfile +import aws.sdk.kotlin.runtime.config.profile.requestChecksumCalculation +import aws.sdk.kotlin.runtime.config.profile.responseChecksumValidation +import aws.smithy.kotlin.runtime.client.config.HttpChecksumConfigOption +import aws.smithy.kotlin.runtime.config.resolve +import aws.smithy.kotlin.runtime.util.LazyAsyncValue +import aws.smithy.kotlin.runtime.util.PlatformProvider + +/** + * Attempts to resolve requestChecksumCalculation from the specified sources. + * @return requestChecksumCalculation setting if found, the default value if not. + */ +@InternalSdkApi +public suspend fun resolveRequestChecksumCalculation(platform: PlatformProvider = PlatformProvider.System, profile: LazyAsyncValue): HttpChecksumConfigOption { + val unparsedString = AwsSdkSetting.AwsRequestChecksumCalculation.resolve(platform) ?: profile.get().requestChecksumCalculation + return parseHttpChecksumConfigOption(unparsedString, "requestChecksumCalculation") +} + +/** + * Attempts to resolve responseChecksumValidation from the specified sources. + * @return responseChecksumValidation setting if found, the default value if not. + */ +@InternalSdkApi +public suspend fun resolveResponseChecksumValidation(platform: PlatformProvider = PlatformProvider.System, profile: LazyAsyncValue): HttpChecksumConfigOption { + val unparsedString = AwsSdkSetting.AwsResponseChecksumValidation.resolve(platform) ?: profile.get().responseChecksumValidation + return parseHttpChecksumConfigOption(unparsedString, "responseChecksumValidation") +} + +private fun parseHttpChecksumConfigOption(unparsedString: String?, configOption: String): HttpChecksumConfigOption = + when (unparsedString?.uppercase()) { + null -> HttpChecksumConfigOption.WHEN_SUPPORTED + "WHEN_SUPPORTED" -> HttpChecksumConfigOption.WHEN_SUPPORTED + "WHEN_REQUIRED" -> HttpChecksumConfigOption.WHEN_REQUIRED + else -> throw ConfigurationException( + "'$unparsedString' is not a valid value for $configOption. Valid values are: ${HttpChecksumConfigOption.entries}", + ) + } diff --git a/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveRequestChecksumCalculation.kt b/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveRequestChecksumCalculation.kt deleted file mode 100644 index dd521548b9f..00000000000 --- a/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveRequestChecksumCalculation.kt +++ /dev/null @@ -1,29 +0,0 @@ -package aws.sdk.kotlin.runtime.config.checksums - -import aws.sdk.kotlin.runtime.ConfigurationException -import aws.sdk.kotlin.runtime.InternalSdkApi -import aws.sdk.kotlin.runtime.config.AwsSdkSetting -import aws.sdk.kotlin.runtime.config.profile.AwsProfile -import aws.sdk.kotlin.runtime.config.profile.requestChecksumCalculation -import aws.smithy.kotlin.runtime.client.config.HttpChecksumConfigOption -import aws.smithy.kotlin.runtime.config.resolve -import aws.smithy.kotlin.runtime.util.LazyAsyncValue -import aws.smithy.kotlin.runtime.util.PlatformProvider - -/** - * Attempts to resolve requestChecksumCalculation from the specified sources. - * @return requestChecksumCalculation setting if found, the default value if not. - */ -@InternalSdkApi -public suspend fun resolveRequestChecksumCalculation(platform: PlatformProvider = PlatformProvider.System, profile: LazyAsyncValue): HttpChecksumConfigOption { - val unparsedString = AwsSdkSetting.AwsRequestChecksumCalculation.resolve(platform) ?: profile.get().requestChecksumCalculation - return unparsedString?.let { - when (unparsedString.uppercase()) { - "WHEN_SUPPORTED" -> HttpChecksumConfigOption.WHEN_SUPPORTED - "WHEN_REQUIRED" -> HttpChecksumConfigOption.WHEN_REQUIRED - else -> throw ConfigurationException( - "'$it' is not a valid value for request checksum calculation. Valid values are: ${HttpChecksumConfigOption.entries}", - ) - } - } ?: HttpChecksumConfigOption.WHEN_SUPPORTED -} diff --git a/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveResponseChecksumValidation.kt b/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveResponseChecksumValidation.kt deleted file mode 100644 index 6c7162c2195..00000000000 --- a/aws-runtime/aws-config/common/src/aws/sdk/kotlin/runtime/config/checksums/ResolveResponseChecksumValidation.kt +++ /dev/null @@ -1,29 +0,0 @@ -package aws.sdk.kotlin.runtime.config.checksums - -import aws.sdk.kotlin.runtime.ConfigurationException -import aws.sdk.kotlin.runtime.InternalSdkApi -import aws.sdk.kotlin.runtime.config.AwsSdkSetting -import aws.sdk.kotlin.runtime.config.profile.AwsProfile -import aws.sdk.kotlin.runtime.config.profile.responseChecksumValidation -import aws.smithy.kotlin.runtime.client.config.HttpChecksumConfigOption -import aws.smithy.kotlin.runtime.config.resolve -import aws.smithy.kotlin.runtime.util.LazyAsyncValue -import aws.smithy.kotlin.runtime.util.PlatformProvider - -/** - * Attempts to resolve responseChecksumValidation from the specified sources. - * @return responseChecksumValidation setting if found, the default value if not. - */ -@InternalSdkApi -public suspend fun resolveResponseChecksumValidation(platform: PlatformProvider = PlatformProvider.System, profile: LazyAsyncValue): HttpChecksumConfigOption { - val unparsedString = AwsSdkSetting.AwsResponseChecksumValidation.resolve(platform) ?: profile.get().responseChecksumValidation - return unparsedString?.let { - when (unparsedString.uppercase()) { - "WHEN_SUPPORTED" -> HttpChecksumConfigOption.WHEN_SUPPORTED - "WHEN_REQUIRED" -> HttpChecksumConfigOption.WHEN_REQUIRED - else -> throw ConfigurationException( - "'$it' is not a valid value for request checksum calculation. Valid values are: ${HttpChecksumConfigOption.entries}", - ) - } - } ?: HttpChecksumConfigOption.WHEN_SUPPORTED -} diff --git a/aws-runtime/aws-http/api/aws-http.api b/aws-runtime/aws-http/api/aws-http.api index 425d6c7ac51..0b475eb05fa 100644 --- a/aws-runtime/aws-http/api/aws-http.api +++ b/aws-runtime/aws-http/api/aws-http.api @@ -196,6 +196,29 @@ public final class aws/sdk/kotlin/runtime/http/interceptors/BusinessMetricsInter public fun readBeforeTransmit (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;)V } +public final class aws/sdk/kotlin/runtime/http/interceptors/S3CompositeChecksumsInterceptor : aws/smithy/kotlin/runtime/client/Interceptor { + public fun ()V + public fun modifyBeforeAttemptCompletion-gIAlu-s (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; + public fun modifyBeforeCompletion-gIAlu-s (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; + public fun modifyBeforeDeserialization (Laws/smithy/kotlin/runtime/client/ProtocolResponseInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; + public fun modifyBeforeRetryLoop (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; + public fun modifyBeforeSerialization (Laws/smithy/kotlin/runtime/client/RequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; + public fun modifyBeforeSigning (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; + public fun modifyBeforeTransmit (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; + public fun readAfterAttempt (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;)V + public fun readAfterDeserialization (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;)V + public fun readAfterExecution (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;)V + public fun readAfterSerialization (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;)V + public fun readAfterSigning (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;)V + public fun readAfterTransmit (Laws/smithy/kotlin/runtime/client/ProtocolResponseInterceptorContext;)V + public fun readBeforeAttempt (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;)V + public fun readBeforeDeserialization (Laws/smithy/kotlin/runtime/client/ProtocolResponseInterceptorContext;)V + public fun readBeforeExecution (Laws/smithy/kotlin/runtime/client/RequestInterceptorContext;)V + public fun readBeforeSerialization (Laws/smithy/kotlin/runtime/client/RequestInterceptorContext;)V + public fun readBeforeSigning (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;)V + public fun readBeforeTransmit (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;)V +} + public final class aws/sdk/kotlin/runtime/http/interceptors/UnsupportedSigningAlgorithmInterceptor : aws/smithy/kotlin/runtime/client/Interceptor { public fun ()V public fun modifyBeforeAttemptCompletion-gIAlu-s (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; diff --git a/aws-runtime/aws-http/common/src/aws/sdk/kotlin/runtime/http/interceptors/S3CompositeChecksumsInterceptor.kt b/aws-runtime/aws-http/common/src/aws/sdk/kotlin/runtime/http/interceptors/S3CompositeChecksumsInterceptor.kt new file mode 100644 index 00000000000..16adc9aeef5 --- /dev/null +++ b/aws-runtime/aws-http/common/src/aws/sdk/kotlin/runtime/http/interceptors/S3CompositeChecksumsInterceptor.kt @@ -0,0 +1,47 @@ +package aws.sdk.kotlin.runtime.http.interceptors + +import aws.smithy.kotlin.runtime.client.ProtocolResponseInterceptorContext +import aws.smithy.kotlin.runtime.http.HeadersBuilder +import aws.smithy.kotlin.runtime.http.interceptors.HttpInterceptor +import aws.smithy.kotlin.runtime.http.request.HttpRequest +import aws.smithy.kotlin.runtime.http.response.HttpResponse +import aws.smithy.kotlin.runtime.http.response.toBuilder +import aws.smithy.kotlin.runtime.telemetry.logging.Logger +import aws.smithy.kotlin.runtime.telemetry.logging.logger +import kotlin.coroutines.coroutineContext + +private const val CHECKSUM_HEADER_PREFIX = "x-amz-checksum-" + +/** + * Removes any checksum headers that contain composite checksums from an S3 response. + */ +public class S3CompositeChecksumsInterceptor : HttpInterceptor { + override suspend fun modifyBeforeDeserialization(context: ProtocolResponseInterceptorContext): HttpResponse { + val response = context.protocolResponse.toBuilder() + val logger = coroutineContext.logger() + + response.headers.removeCompositeChecksums(logger) + + return response.build() + } +} + +/** + * Removes any checksum headers that contain composite checksums. + */ +internal fun HeadersBuilder.removeCompositeChecksums(logger: Logger): Unit = + names().forEach { header -> + if (header.startsWith(CHECKSUM_HEADER_PREFIX, ignoreCase = true) && get(header)!!.isCompositeChecksum()) { + logger.warn { "S3 returned a composite checksum, composite checksums are not supported, removing checksum" } + remove(header) + } + } + +/** + * Verifies if a checksum is composite. + */ +private fun String.isCompositeChecksum(): Boolean { + // Ends with "-#" where "#" is a number + val regex = Regex("-(\\d)+$") + return regex.containsMatchIn(this) +} diff --git a/aws-runtime/aws-http/common/test/aws/sdk/kotlin/runtime/http/interceptors/S3CompositeChecksumsInterceptorTest.kt b/aws-runtime/aws-http/common/test/aws/sdk/kotlin/runtime/http/interceptors/S3CompositeChecksumsInterceptorTest.kt new file mode 100644 index 00000000000..042dcb5b99e --- /dev/null +++ b/aws-runtime/aws-http/common/test/aws/sdk/kotlin/runtime/http/interceptors/S3CompositeChecksumsInterceptorTest.kt @@ -0,0 +1,35 @@ +package aws.sdk.kotlin.runtime.http.interceptors + +import aws.smithy.kotlin.runtime.http.HeadersBuilder +import aws.smithy.kotlin.runtime.telemetry.logging.logger +import kotlinx.coroutines.test.runTest +import kotlin.coroutines.coroutineContext +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class S3CompositeChecksumsInterceptorTest { + @Test + fun compositeChecksumRemoval() = runTest { + val headers = HeadersBuilder() + + headers.append("x-amz-checksum-crc32", "foo-1") + headers.append("x-amz-checksum-sha256", "bar") + + assertTrue( + headers.contains("x-amz-checksum-crc32"), + ) + assertTrue( + headers.contains("x-amz-checksum-sha256"), + ) + + headers.removeCompositeChecksums(coroutineContext.logger()) + + assertFalse( + headers.contains("x-amz-checksum-crc32"), + ) + assertTrue( + headers.contains("x-amz-checksum-sha256"), + ) + } +} diff --git a/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/AwsRuntimeTypes.kt b/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/AwsRuntimeTypes.kt index b4debfc4bd8..70f3a4950ef 100644 --- a/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/AwsRuntimeTypes.kt +++ b/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/AwsRuntimeTypes.kt @@ -62,6 +62,7 @@ object AwsRuntimeTypes { val UnsupportedSigningAlgorithmInterceptor = symbol("UnsupportedSigningAlgorithmInterceptor") val BusinessMetricsInterceptor = symbol("BusinessMetricsInterceptor") val AwsBusinessMetric = symbol("AwsBusinessMetric") + val S3CompositeChecksumsInterceptor = symbol("S3CompositeChecksumsInterceptor") } object Retries { diff --git a/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/flexiblechecksums/FlexibleChecksumsResponse.kt b/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/flexiblechecksums/FlexibleChecksumsResponse.kt index afd8c29c20a..9767d2a9718 100644 --- a/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/flexiblechecksums/FlexibleChecksumsResponse.kt +++ b/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/flexiblechecksums/FlexibleChecksumsResponse.kt @@ -18,13 +18,6 @@ import software.amazon.smithy.model.Model import software.amazon.smithy.model.shapes.OperationShape import software.amazon.smithy.model.shapes.StructureShape -/** - * AWS services that can return composite checksums in their responses - */ -private val compositeChecksumServices = setOf( - "s3", -) - /** * Adds a middleware which validates checksums returned in responses if the user has opted-in. */ @@ -98,7 +91,6 @@ class FlexibleChecksumsResponse : KotlinIntegration { ) { writer.write("input.#L?.value == \"ENABLED\",", requestValidationModeMemberName) writer.write("config.responseChecksumValidation,") - writer.write("#L", compositeChecksumServices.contains(ctx.settings.sdkId)) } } } diff --git a/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/flexiblechecksums/s3/S3CompositeChecksumsIntegration.kt b/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/flexiblechecksums/s3/S3CompositeChecksumsIntegration.kt new file mode 100644 index 00000000000..cef54247f72 --- /dev/null +++ b/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/flexiblechecksums/s3/S3CompositeChecksumsIntegration.kt @@ -0,0 +1,44 @@ +package aws.sdk.kotlin.codegen.customization.flexiblechecksums.s3 + +import aws.sdk.kotlin.codegen.AwsRuntimeTypes.Http.Interceptors.S3CompositeChecksumsInterceptor +import aws.sdk.kotlin.codegen.customization.s3.isS3 +import software.amazon.smithy.aws.traits.HttpChecksumTrait +import software.amazon.smithy.kotlin.codegen.KotlinSettings +import software.amazon.smithy.kotlin.codegen.core.KotlinWriter +import software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration +import software.amazon.smithy.kotlin.codegen.model.expectShape +import software.amazon.smithy.kotlin.codegen.model.hasTrait +import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolGenerator +import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolMiddleware +import software.amazon.smithy.model.Model +import software.amazon.smithy.model.shapes.OperationShape +import software.amazon.smithy.model.shapes.ServiceShape + +/** + * Removes composite checksums returned by S3 so that flexible checksums won't validate them. + * Composite checksums are used for multipart uploads and end with "-#" where "#" is a number. + */ +class S3CompositeChecksumsIntegration : KotlinIntegration { + override val order: Byte + get() = -128 + + override fun enabledForService(model: Model, settings: KotlinSettings): Boolean = + model.expectShape(settings.service).isS3 + + override fun customizeMiddleware(ctx: ProtocolGenerator.GenerationContext, resolved: List) = + resolved + s3CompositeChecksumsMiddleware +} + +/** + * Adds [S3CompositeChecksumsInterceptor] to interceptors when operation has flexible checksums. + */ +private val s3CompositeChecksumsMiddleware = object : ProtocolMiddleware { + override val name: String = "S3CompositeChecksumsMiddleware" + + override fun isEnabledFor(ctx: ProtocolGenerator.GenerationContext, op: OperationShape): Boolean = + op.hasTrait() + + override fun render(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) { + writer.write("op.interceptors.add(#T())", S3CompositeChecksumsInterceptor) + } +} diff --git a/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/express/S3ExpressIntegration.kt b/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/express/S3ExpressIntegration.kt index 86e8c568506..067da2da40f 100644 --- a/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/express/S3ExpressIntegration.kt +++ b/codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/customization/s3/express/S3ExpressIntegration.kt @@ -16,6 +16,7 @@ import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolGenerato import software.amazon.smithy.kotlin.codegen.rendering.protocol.ProtocolMiddleware import software.amazon.smithy.kotlin.codegen.rendering.util.ConfigProperty import software.amazon.smithy.kotlin.codegen.rendering.util.ConfigPropertyType +import software.amazon.smithy.kotlin.codegen.utils.getOrNull import software.amazon.smithy.model.Model import software.amazon.smithy.model.shapes.* import software.amazon.smithy.model.traits.* @@ -138,12 +139,25 @@ class S3ExpressIntegration : KotlinIntegration { op.isS3UploadPart && op.hasTrait() override fun render(ctx: ProtocolGenerator.GenerationContext, op: OperationShape, writer: KotlinWriter) { + val httpChecksumTrait = op.getTrait()!! + + val requestAlgorithmMemberName = httpChecksumTrait.requestAlgorithmMember?.getOrNull()?.let { + val requestAlgorithmMemberShape = ctx.model.expectShape(op.input.get()) + .members() + .first { it.memberName == httpChecksumTrait.requestAlgorithmMember.get() } + ctx.symbolProvider.toMemberName(requestAlgorithmMemberShape) + } + val interceptorSymbol = buildSymbol { namespace = "aws.sdk.kotlin.services.s3.express" name = "S3ExpressDisableChecksumInterceptor" } writer.addImport(interceptorSymbol) - writer.write("op.interceptors.add(#T())", interceptorSymbol) + writer.write( + "op.interceptors.add(#T(input.#L?.value != null))", + interceptorSymbol, + requestAlgorithmMemberName, + ) } } diff --git a/codegen/aws-sdk-codegen/src/main/resources/META-INF/services/software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration b/codegen/aws-sdk-codegen/src/main/resources/META-INF/services/software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration index de1ab6cd4ac..20c0701ea92 100644 --- a/codegen/aws-sdk-codegen/src/main/resources/META-INF/services/software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration +++ b/codegen/aws-sdk-codegen/src/main/resources/META-INF/services/software.amazon.smithy.kotlin.codegen.integration.KotlinIntegration @@ -12,6 +12,7 @@ aws.sdk.kotlin.codegen.endpoints.AddAwsEndpointFunctions aws.sdk.kotlin.codegen.PresignerGenerator aws.sdk.kotlin.codegen.customization.flexiblechecksums.FlexibleChecksumsRequest aws.sdk.kotlin.codegen.customization.flexiblechecksums.FlexibleChecksumsResponse +aws.sdk.kotlin.codegen.customization.flexiblechecksums.s3.S3CompositeChecksumsIntegration aws.sdk.kotlin.codegen.customization.AddAwsSpanAttributes aws.sdk.kotlin.codegen.customization.s3.S3OperationErrorHandler aws.sdk.kotlin.codegen.customization.s3.S3SigningConfig diff --git a/services/s3/common/src/aws/sdk/kotlin/services/s3/express/S3ExpressDisableChecksumInterceptor.kt b/services/s3/common/src/aws/sdk/kotlin/services/s3/express/S3ExpressDisableChecksumInterceptor.kt index 170818208d4..1dd06c25406 100644 --- a/services/s3/common/src/aws/sdk/kotlin/services/s3/express/S3ExpressDisableChecksumInterceptor.kt +++ b/services/s3/common/src/aws/sdk/kotlin/services/s3/express/S3ExpressDisableChecksumInterceptor.kt @@ -11,7 +11,7 @@ import aws.smithy.kotlin.runtime.http.HeadersBuilder import aws.smithy.kotlin.runtime.http.interceptors.HttpInterceptor import aws.smithy.kotlin.runtime.http.request.HttpRequest import aws.smithy.kotlin.runtime.http.request.toBuilder -import aws.smithy.kotlin.runtime.telemetry.logging.logger +import aws.smithy.kotlin.runtime.telemetry.logging.warn import kotlin.coroutines.coroutineContext private const val CHECKSUM_HEADER_PREFIX = "x-amz-checksum-" @@ -21,14 +21,19 @@ private const val S3_EXPRESS_ENDPOINT_PROPERTY_VALUE = "S3Express" /** * Disables checksums for s3:UploadPart requests that use S3 express. */ -internal class S3ExpressDisableChecksumInterceptor : HttpInterceptor { +internal class S3ExpressDisableChecksumInterceptor( + private val userConfiguredChecksum: Boolean, +) : HttpInterceptor { override suspend fun modifyBeforeSigning(context: ProtocolRequestInterceptorContext): HttpRequest { if (context.executionContext.getOrNull(AttributeKey(S3_EXPRESS_ENDPOINT_PROPERTY_KEY)) != S3_EXPRESS_ENDPOINT_PROPERTY_VALUE) { return context.protocolRequest } - val logger = coroutineContext.logger() - logger.warn { "Checksums must not be sent with S3 express upload part operation, removing checksum(s)" } + if (userConfiguredChecksum) { + coroutineContext.warn { + "Checksums must not be sent with S3 Express UploadPart operation, removing checksum(s)" + } + } val request = context.protocolRequest.toBuilder() @@ -45,7 +50,7 @@ internal class S3ExpressDisableChecksumInterceptor : HttpInterceptor { */ internal fun HeadersBuilder.removeChecksumHeaders(): Unit = names().forEach { name -> - if (name.startsWith(CHECKSUM_HEADER_PREFIX)) { + if (name.startsWith(CHECKSUM_HEADER_PREFIX, ignoreCase = true)) { remove(name) } } @@ -55,7 +60,7 @@ internal fun HeadersBuilder.removeChecksumHeaders(): Unit = */ internal fun DeferredHeadersBuilder.removeChecksumTrailingHeaders(): Unit = names().forEach { name -> - if (name.startsWith(CHECKSUM_HEADER_PREFIX)) { + if (name.startsWith(CHECKSUM_HEADER_PREFIX, ignoreCase = true)) { remove(name) } } @@ -65,7 +70,7 @@ internal fun DeferredHeadersBuilder.removeChecksumTrailingHeaders(): Unit = */ internal fun HeadersBuilder.removeChecksumTrailingHeadersFromXAmzTrailer() { this.getAll("x-amz-trailer")?.forEach { trailingHeader -> - if (trailingHeader.startsWith(CHECKSUM_HEADER_PREFIX)) { + if (trailingHeader.startsWith(CHECKSUM_HEADER_PREFIX, ignoreCase = true)) { this.remove("x-amz-trailer", trailingHeader) } } diff --git a/services/s3/common/test/aws/sdk/kotlin/services/s3/express/ChecksumRemovalTest.kt b/services/s3/common/test/aws/sdk/kotlin/services/s3/express/ChecksumRemovalTest.kt index cbf5a9ad4d4..c00f78e821f 100644 --- a/services/s3/common/test/aws/sdk/kotlin/services/s3/express/ChecksumRemovalTest.kt +++ b/services/s3/common/test/aws/sdk/kotlin/services/s3/express/ChecksumRemovalTest.kt @@ -7,27 +7,31 @@ import kotlin.test.assertFalse import kotlin.test.assertTrue class ChecksumRemovalTest { + // Header capitalization shouldn't matter + private val crc32Header = "x-aMz-cHeCkSum-cRC32" + private val sha256Header = "X-Amz-ChEcKsum-sHa256" + @Test fun removeChecksumHeaders() { val headers = HeadersBuilder() - headers.append("x-amz-checksum-crc32", "foo") - headers.append("x-amz-checksum-sha256", "bar") + headers.append(crc32Header, "foo") + headers.append(sha256Header, "bar") assertTrue( - headers.contains("x-amz-checksum-crc32"), + headers.contains(crc32Header), ) assertTrue( - headers.contains("x-amz-checksum-sha256"), + headers.contains(sha256Header), ) headers.removeChecksumHeaders() assertFalse( - headers.contains("x-amz-checksum-crc32"), + headers.contains(crc32Header), ) assertFalse( - headers.contains("x-amz-checksum-sha256"), + headers.contains(sha256Header), ) } @@ -35,23 +39,23 @@ class ChecksumRemovalTest { fun removeChecksumTrailingHeaders() { val trailingHeaders = DeferredHeadersBuilder() - trailingHeaders.add("x-amz-checksum-crc32", "foo") - trailingHeaders.add("x-amz-checksum-sha256", "bar") + trailingHeaders.add(crc32Header, "foo") + trailingHeaders.add(sha256Header, "bar") assertTrue( - trailingHeaders.contains("x-amz-checksum-crc32"), + trailingHeaders.contains(crc32Header), ) assertTrue( - trailingHeaders.contains("x-amz-checksum-sha256"), + trailingHeaders.contains(sha256Header), ) trailingHeaders.removeChecksumTrailingHeaders() assertFalse( - trailingHeaders.contains("x-amz-checksum-crc32"), + trailingHeaders.contains(crc32Header), ) assertFalse( - trailingHeaders.contains("x-amz-checksum-sha256"), + trailingHeaders.contains(sha256Header), ) } @@ -59,13 +63,13 @@ class ChecksumRemovalTest { fun removeChecksumTrailingHeadersFromXAmzTrailer() { val headers = HeadersBuilder() - headers.append("x-amz-trailer", "x-amz-checksum-crc32") + headers.append("x-amz-trailer", crc32Header) headers.append("x-amz-trailer", "x-amz-trailing-header") val xAmzTrailer = headers.getAll("x-amz-trailer") assertTrue( - xAmzTrailer?.contains("x-amz-checksum-crc32") ?: false, + xAmzTrailer?.contains(crc32Header) ?: false, ) assertTrue( xAmzTrailer?.contains("x-amz-trailing-header") ?: false, @@ -74,7 +78,7 @@ class ChecksumRemovalTest { headers.removeChecksumTrailingHeadersFromXAmzTrailer() assertFalse( - xAmzTrailer?.contains("x-amz-checksum-crc32") ?: false, + xAmzTrailer?.contains(crc32Header) ?: false, ) assertTrue( xAmzTrailer?.contains("x-amz-trailing-header") ?: false, diff --git a/services/s3/e2eTest/src/S3ChecksumTest.kt b/services/s3/e2eTest/src/S3ChecksumTest.kt index db8c4026d5a..4b99ae22e8f 100644 --- a/services/s3/e2eTest/src/S3ChecksumTest.kt +++ b/services/s3/e2eTest/src/S3ChecksumTest.kt @@ -8,19 +8,21 @@ import aws.sdk.kotlin.services.s3.* import aws.sdk.kotlin.services.s3.model.CompletedMultipartUpload import aws.sdk.kotlin.services.s3.model.CompletedPart import aws.sdk.kotlin.services.s3.model.GetObjectRequest -import aws.smithy.kotlin.runtime.content.ByteStream -import aws.smithy.kotlin.runtime.content.fromInputStream +import aws.smithy.kotlin.runtime.content.* +import aws.smithy.kotlin.runtime.hashing.crc32 +import aws.smithy.kotlin.runtime.testing.RandomTempFile import kotlinx.coroutines.runBlocking import org.junit.jupiter.api.* import java.io.File import java.io.FileInputStream import java.util.* +import kotlin.test.assertEquals @TestInstance(TestInstance.Lifecycle.PER_CLASS) class S3ChecksumTest { private val client = S3Client { region = "us-west-2" } private val testBucket = "s3-test-bucket-ci-motorcade" - private val testObject = "test-object" + private fun testKey(): String = "test-object" + UUID.randomUUID() @BeforeAll private fun setUp(): Unit = runBlocking { @@ -38,87 +40,123 @@ class S3ChecksumTest { @Test fun testPutObject(): Unit = runBlocking { + val testBody = "Hello World" + val testKey = testKey() + client.putObject { bucket = testBucket - key = testObject - body = ByteStream.fromString("Hello World") + key = testKey + body = ByteStream.fromString(testBody) + } + + client.getObject( + GetObjectRequest { + bucket = testBucket + key = testKey + }, + ) { actual -> + assertEquals(testBody, actual.body?.decodeToString() ?: "") } } @Test fun testPutObjectWithEmptyBody(): Unit = runBlocking { + val testKey = testKey() + val testBody = "" + client.putObject { bucket = testBucket - key = testObject + UUID.randomUUID() + key = testKey + } + + client.getObject( + GetObjectRequest { + bucket = testBucket + key = testKey + }, + ) { actual -> + assertEquals(testBody, actual.body?.decodeToString() ?: "") } } @Test fun testPutObjectAwsChunkedEncoded(): Unit = runBlocking { - val testString = "Hello World" + val testKey = testKey() + val testBody = "Hello World" + val tempFile = File.createTempFile("test", ".txt").also { - it.writeText(testString) + it.writeText(testBody) it.deleteOnExit() } val inputStream = FileInputStream(tempFile) client.putObject { bucket = testBucket - key = testObject + UUID.randomUUID() - body = ByteStream.fromInputStream(inputStream, testString.length.toLong()) + key = testKey + body = ByteStream.fromInputStream(inputStream, testBody.length.toLong()) + } + + client.getObject( + GetObjectRequest { + bucket = testBucket + key = testKey + }, + ) { actual -> + assertEquals(testBody, actual.body?.decodeToString() ?: "") } } @Test fun testMultiPartUpload(): Unit = runBlocking { - // Parts need to be at least 5 MB - val partOne = "Hello".repeat(1_048_576) - val partTwo = "World".repeat(1_048_576) + val testKey = testKey() + + val partSize = 5 * 1024 * 1024 // 5 MB - min part size + val contentSize: Long = 8 * 1024 * 1024 // 2 parts + val file = RandomTempFile(sizeInBytes = contentSize) + + val expectedChecksum = file.readBytes().crc32() val testUploadId = client.createMultipartUpload { bucket = testBucket - key = testObject + key = testKey }.uploadId - val eTagPartOne = client.uploadPart { - bucket = testBucket - key = testObject - partNumber = 1 - uploadId = testUploadId - body = ByteStream.fromString(partOne) - }.eTag + val uploadedParts = file.chunk(partSize).mapIndexed { index, chunk -> + val adjustedIndex = index + 1 // index starts from 0 but partNumber needs to start from 1 - val eTagPartTwo = client.uploadPart { - bucket = testBucket - key = testObject - partNumber = 2 - uploadId = testUploadId - body = ByteStream.fromString(partTwo) - }.eTag + runBlocking { + client.uploadPart { + bucket = testBucket + key = testKey + partNumber = adjustedIndex + uploadId = testUploadId + body = file.asByteStream(chunk) + }.let { + CompletedPart { + partNumber = adjustedIndex + eTag = it.eTag + } + } + } + }.toList() client.completeMultipartUpload { bucket = testBucket - key = testObject + key = testKey uploadId = testUploadId multipartUpload = CompletedMultipartUpload { - parts = listOf( - CompletedPart { - partNumber = 1 - eTag = eTagPartOne - }, - CompletedPart { - partNumber = 2 - eTag = eTagPartTwo - }, - ) + parts = uploadedParts } } client.getObject( GetObjectRequest { bucket = testBucket - key = testObject + key = testKey }, - ) {} + ) { actual -> + val actualChecksum = actual.body!!.toByteArray().crc32() + assertEquals(actualChecksum, expectedChecksum) + } } } diff --git a/services/s3/e2eTest/src/S3IntegrationTest.kt b/services/s3/e2eTest/src/S3IntegrationTest.kt index 37ec353c9f6..aec1de82d16 100644 --- a/services/s3/e2eTest/src/S3IntegrationTest.kt +++ b/services/s3/e2eTest/src/S3IntegrationTest.kt @@ -390,7 +390,7 @@ class S3BucketOpsIntegrationTest { } // generate sequence of "chunks" where each range defines the inclusive start and end bytes -private fun File.chunk(partSize: Int): Sequence = +internal fun File.chunk(partSize: Int): Sequence = (0 until length() step partSize.toLong()).asSequence().map { it until minOf(it + partSize, length()) } diff --git a/services/s3/e2eTest/src/S3TestUtils.kt b/services/s3/e2eTest/src/S3TestUtils.kt index 4139b4d9e33..e089beb6614 100644 --- a/services/s3/e2eTest/src/S3TestUtils.kt +++ b/services/s3/e2eTest/src/S3TestUtils.kt @@ -10,6 +10,7 @@ import aws.sdk.kotlin.services.s3.model.* import aws.sdk.kotlin.services.s3.model.BucketLocationConstraint import aws.sdk.kotlin.services.s3.model.ExpirationStatus import aws.sdk.kotlin.services.s3.model.LifecycleRule +import aws.sdk.kotlin.services.s3.paginators.listBucketsPaginated import aws.sdk.kotlin.services.s3.paginators.listObjectsV2Paginated import aws.sdk.kotlin.services.s3.waiters.waitUntilBucketExists import aws.sdk.kotlin.services.s3.waiters.waitUntilBucketNotExists @@ -103,26 +104,42 @@ object S3TestUtils { suspend fun getBucketByName( client: S3Client, - bucket: String, + targetBucket: String, region: String? = null, accountId: String? = null, ): String = withTimeout(60.seconds) { - val buckets = client.listBuckets() - .buckets - ?.mapNotNull { it.name } + val bucketNames = mutableListOf() - var testBucket = buckets?.firstOrNull { bucketName -> - bucketName == bucket && - region?.let { + client.listBucketsPaginated() + .collect { response -> + response.buckets?.forEach { bucket -> + bucket.name?.let { bucketNames.add(it) } + } + } + + var testBucket = bucketNames.firstOrNull { bucketName -> + if (bucketName == targetBucket) { + val isInCorrectLocation = region?.let { client.getBucketLocation { - this.bucket = bucketName + bucket = bucketName expectedBucketOwner = accountId }.locationConstraint?.value == region } ?: true + + if (isInCorrectLocation) { + true + } else { + throw RuntimeException( + "The requested bucket ($targetBucket) already exists in another region than the one requested ($region)", + ) + } + } else { + false + } } if (testBucket == null) { - testBucket = bucket + testBucket = targetBucket println("Creating S3 bucket: $testBucket") client.createBucket { diff --git a/tests/codegen/build.gradle.kts b/tests/codegen/build.gradle.kts index dd0a9537e6b..696c3c3104a 100644 --- a/tests/codegen/build.gradle.kts +++ b/tests/codegen/build.gradle.kts @@ -16,7 +16,6 @@ subprojects { apply(plugin = libraries.plugins.kotlin.multiplatform.get().pluginId) val optinAnnotations = listOf( - "kotlin.RequiresOptIn", "aws.smithy.kotlin.runtime.InternalApi", "aws.sdk.kotlin.runtime.InternalSdkApi", ) diff --git a/tests/codegen/checksums/src/commonTest/kotlin/aws/sdk/kotlin/tests/codegen/checksums/ChecksumConfigTest.kt b/tests/codegen/checksums/src/commonTest/kotlin/aws/sdk/kotlin/tests/codegen/checksums/ChecksumConfigTest.kt index e26afa7908c..e7fe9933394 100644 --- a/tests/codegen/checksums/src/commonTest/kotlin/aws/sdk/kotlin/tests/codegen/checksums/ChecksumConfigTest.kt +++ b/tests/codegen/checksums/src/commonTest/kotlin/aws/sdk/kotlin/tests/codegen/checksums/ChecksumConfigTest.kt @@ -265,6 +265,10 @@ class UserProvidedChecksumHeader { assertTrue( headerReader.containsExpectedHeaders, ) + + assertFalse( + headerReader.containsForbiddenHeaders, + ) } } @@ -370,35 +374,4 @@ class ResponseChecksumValidation { } } } - - @Test - fun compositeChecksumsAreNotValidated(): Unit = runBlocking { - ClientConfigTestClient { - responseChecksumValidation = HttpChecksumConfigOption.WHEN_REQUIRED - httpClient = TestEngine( - roundTripImpl = { _, request -> - val resp = HttpResponse( - HttpStatusCode.OK, - Headers { - append( - "x-amz-checksum-crc32", - "I'm a composite checksum and will trigger `ChecksumMismatchException` if read!-1", - ) - }, - "World!".toHttpBody(), - ) - val now = Instant.now() - HttpCall(request, resp, now, now) - }, - ) - credentialsProvider = StaticCredentialsProvider( - Credentials("accessKeyID", "secretAccessKey"), - ) - region = "us-east-1" - }.use { client -> - client.checksumsRequiredOperation { - body = "Hello" - } - } - } } diff --git a/tests/codegen/checksums/src/commonTest/kotlin/aws/sdk/kotlin/tests/codegen/checksums/utils/ChecksumTestUtils.kt b/tests/codegen/checksums/src/commonTest/kotlin/aws/sdk/kotlin/tests/codegen/checksums/utils/ChecksumTestUtils.kt index a8f1f634a1f..c88263bfe70 100644 --- a/tests/codegen/checksums/src/commonTest/kotlin/aws/sdk/kotlin/tests/codegen/checksums/utils/ChecksumTestUtils.kt +++ b/tests/codegen/checksums/src/commonTest/kotlin/aws/sdk/kotlin/tests/codegen/checksums/utils/ChecksumTestUtils.kt @@ -20,8 +20,8 @@ internal class HeaderReader( private val expectedHeaders: Map, private val forbiddenHeaders: Map = emptyMap(), ) : HttpInterceptor { - var containsExpectedHeaders = true - var containsForbiddenHeaders = false + var containsExpectedHeaders = false + var containsForbiddenHeaders = true override fun readBeforeTransmit(context: ProtocolRequestInterceptorContext) { expectedHeaders.forEach { header -> @@ -31,17 +31,18 @@ internal class HeaderReader( } ?: true if (!containsHeader || !headerValueMatches) { - containsExpectedHeaders = false return } } forbiddenHeaders.forEach { header -> if (context.protocolRequest.headers.contains(header.key)) { - containsForbiddenHeaders = true return } } + + containsExpectedHeaders = true + containsForbiddenHeaders = false } } @@ -66,7 +67,7 @@ internal class HeaderSetter( internal class BusinessMetricsReader( private val expectedBusinessMetrics: Set, ) : HttpInterceptor { - var containsExpectedBusinessMetrics = true + var containsExpectedBusinessMetrics = false override fun readBeforeTransmit(context: ProtocolRequestInterceptorContext) { containsExpectedBusinessMetrics = context.executionContext[BusinessMetrics].containsAll(expectedBusinessMetrics)