Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
2a9d104
Bump smithy IDL version
0marperez Nov 11, 2024
205839c
Add requestChecksumCalculation config option
0marperez Nov 11, 2024
7233b71
Added responseChecksumValidation
0marperez Nov 11, 2024
e1dc616
Add todos for business metrics
0marperez Nov 12, 2024
e436482
Unit tests pass
0marperez Nov 25, 2024
abdba02
Merge branch 'main' of https://github.com/awslabs/smithy-kotlin into …
0marperez Nov 26, 2024
3e4c891
E2E tests pass
0marperez Nov 26, 2024
9760ee1
Self review
0marperez Nov 27, 2024
f8b39b0
Self review 2
0marperez Nov 27, 2024
f676b7b
Smithy codegen version bump
0marperez Nov 27, 2024
6e9b206
Make composite checksum check S3 specific
0marperez Dec 3, 2024
fb3a52a
Turn off all failing protocol tests
0marperez Dec 3, 2024
40bb298
PR feedback and fix breaking changes
0marperez Dec 3, 2024
828adaa
Merge branch 'main' into flexible-checksums
0marperez Dec 3, 2024
e2068e7
Trigger CI
0marperez Dec 4, 2024
08b4a37
Drop support for http body dot bytes response checksums
0marperez Dec 4, 2024
91355d1
Fix HttpChecksumRequiredTrait
0marperez Dec 4, 2024
1fcd4b2
Fix kotlin writer runtime exception
0marperez Dec 5, 2024
4edabfb
Deprecate HttpOperationContext.ChecksumAlgorithm
0marperez Dec 9, 2024
db65d74
PR feedback
0marperez Dec 11, 2024
b2e6f61
Re-add support for http body dot bytes response checksusms
0marperez Dec 13, 2024
c63cf83
Presigned URL checksums
0marperez Dec 13, 2024
b68a9f5
Merge branch 'main' of https://github.com/awslabs/smithy-kotlin into …
0marperez Dec 13, 2024
0dd7886
PR feedback checkpoint?
0marperez Dec 13, 2024
d74c949
Refactor checksum interceptors
0marperez Dec 18, 2024
1157f26
Fix composite checksums
0marperez Dec 19, 2024
d9f0659
Make it compile
0marperez Dec 19, 2024
dc3ce8b
Merge branch 'main' of https://github.com/awslabs/smithy-kotlin into …
0marperez Dec 19, 2024
3ef1c20
Use toList supported for JVM versions less than 16
0marperez Dec 19, 2024
7116221
PR feedback
0marperez Dec 23, 2024
344f118
Change JVM version
0marperez Dec 24, 2024
c689ff5
Clean up
0marperez Dec 30, 2024
9beca23
misc: revert toList/JVM compatibility changes
0marperez Jan 8, 2025
aa714d9
fix: pr feedback v1
0marperez Jan 9, 2025
69b7765
fix: pr feedback v2 (get rid of caching non bytes http bodies)
0marperez Jan 13, 2025
399e87d
misc: merge from main
0marperez Jan 13, 2025
f4c7e17
fix: pr feedback v3
0marperez Jan 15, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,8 @@ object RuntimeTypes {
object Config : RuntimeTypePackage(KotlinDependency.SMITHY_CLIENT, "config") {
val RequestCompressionConfig = symbol("RequestCompressionConfig")
val CompressionClientConfig = symbol("CompressionClientConfig")
val HttpChecksumClientConfig = symbol("HttpChecksumClientConfig")
val HttpChecksumConfigOption = symbol("HttpChecksumConfigOption")
}

object Endpoints : RuntimeTypePackage(KotlinDependency.SMITHY_CLIENT, "endpoints") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@ software.amazon.smithy.kotlin.codegen.rendering.endpoints.discovery.EndpointDisc
software.amazon.smithy.kotlin.codegen.rendering.endpoints.SdkEndpointBuiltinIntegration
software.amazon.smithy.kotlin.codegen.rendering.compression.RequestCompressionIntegration
software.amazon.smithy.kotlin.codegen.rendering.auth.SigV4AsymmetricAuthSchemeIntegration
software.amazon.smithy.kotlin.codegen.rendering.smoketests.SmokeTestsIntegration
software.amazon.smithy.kotlin.codegen.rendering.smoketests.SmokeTestsIntegration
2 changes: 1 addition & 1 deletion gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ crt-kotlin-version = "0.8.10"
micrometer-version = "1.13.6"

# codegen
smithy-version = "1.51.0"
smithy-version = "1.53.0"
smithy-gradle-version = "0.9.0"

# testing
Expand Down
7 changes: 2 additions & 5 deletions runtime/protocol/http-client/api/http-client.api
Original file line number Diff line number Diff line change
Expand Up @@ -332,18 +332,15 @@ public final class aws/smithy/kotlin/runtime/http/interceptors/DiscoveredEndpoin
}

public final class aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor : aws/smithy/kotlin/runtime/http/interceptors/AbstractChecksumInterceptor {
public fun <init> ()V
public fun <init> (Lkotlin/jvm/functions/Function1;)V
public synthetic fun <init> (Lkotlin/jvm/functions/Function1;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (ZLaws/smithy/kotlin/runtime/client/config/HttpChecksumConfigOption;Ljava/lang/String;)V
public fun applyChecksum (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Ljava/lang/String;)Laws/smithy/kotlin/runtime/http/request/HttpRequest;
public fun calculateChecksum (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
public fun modifyBeforeSigning (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
public fun readAfterSerialization (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;)V
}

public final class aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor : aws/smithy/kotlin/runtime/client/Interceptor {
public static final field Companion Laws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor$Companion;
public fun <init> (Lkotlin/jvm/functions/Function1;)V
public fun <init> (ZLaws/smithy/kotlin/runtime/client/config/HttpChecksumConfigOption;)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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,102 +7,124 @@ package aws.smithy.kotlin.runtime.http.interceptors

import aws.smithy.kotlin.runtime.ClientException
import aws.smithy.kotlin.runtime.InternalApi
import aws.smithy.kotlin.runtime.businessmetrics.BusinessMetric
import aws.smithy.kotlin.runtime.businessmetrics.SmithyBusinessMetric
import aws.smithy.kotlin.runtime.businessmetrics.emitBusinessMetric
import aws.smithy.kotlin.runtime.client.ProtocolRequestInterceptorContext
import aws.smithy.kotlin.runtime.client.config.HttpChecksumConfigOption
import aws.smithy.kotlin.runtime.hashing.*
import aws.smithy.kotlin.runtime.http.*
import aws.smithy.kotlin.runtime.http.operation.HttpOperationContext
import aws.smithy.kotlin.runtime.http.request.HttpRequest
import aws.smithy.kotlin.runtime.http.request.header
import aws.smithy.kotlin.runtime.http.request.toBuilder
import aws.smithy.kotlin.runtime.io.*
import aws.smithy.kotlin.runtime.telemetry.logging.Logger
import aws.smithy.kotlin.runtime.telemetry.logging.logger
import aws.smithy.kotlin.runtime.text.encoding.encodeBase64String
import aws.smithy.kotlin.runtime.util.LazyAsyncValue
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.job
import kotlin.coroutines.coroutineContext

/**
* Mutate a request to enable flexible checksums.
* Calculates a request's checksum.
*
* If the checksum will be sent as a header, calculate the checksum.
* If a user supplies a checksum via an HTTP header no calculation will be done. The exception is MD5, if a user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correctness/clarification: "Calculates a request's checksum" and "If a user supplies a checksum via an HTTP header no calculation will be done." conflict with each other

* supplies an MD5 checksum header it will be ignored.
*
* Otherwise, if it will be sent as a trailing header, calculate the checksum as asynchronously as the body is streamed.
* In this case, a [LazyAsyncValue] will be added to the execution context which allows the trailing checksum to be sent
* after the entire body has been streamed.
* If the request configuration and model requires checksum calculation:
* - Check if the user configured a checksum algorithm for the request and attempt to use that.
* - If no checksum is configured for the request then use the default checksum algorithm to calculate a checksum.
*
* @param checksumAlgorithmNameInitializer an optional function which parses the input [I] to return the checksum algorithm name.
* if not set, then the [HttpOperationContext.ChecksumAlgorithm] execution context attribute will be used.
* If the request will be streamed:
* - The checksum calculation is done asynchronously using a hashing & completing body.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "asynchronously" → "during transmission"

* - The checksum will be sent in a trailing header, once the request is consumed.
*
* If the request will not be streamed:
* - The checksum calculation is done synchronously
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "synchronously" → "before transmission"

* - The checksum will be sent in a header
*
* Business metrics MUST be emitted for the checksum algorithm used.
*
* @param requestChecksumRequired Model sourced flag indicating if checksum calculation is mandatory.
* @param requestChecksumCalculation Configuration option that determines when checksum calculation should be done.
* @param userSelectedChecksumAlgorithm The checksum algorithm that the user selected for the request, may be null.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming: requestChecksumAlgorithm

*/
@InternalApi
public class FlexibleChecksumsRequestInterceptor<I>(
private val checksumAlgorithmNameInitializer: ((I) -> String?)? = null,
public class FlexibleChecksumsRequestInterceptor(
requestChecksumRequired: Boolean,
requestChecksumCalculation: HttpChecksumConfigOption?,
userSelectedChecksumAlgorithm: String?,
) : AbstractChecksumInterceptor() {
private var checksumAlgorithmName: String? = null

@Deprecated("readAfterSerialization is no longer used")
override fun readAfterSerialization(context: ProtocolRequestInterceptorContext<Any, HttpRequest>) { }
private val forcedToCalculateChecksum = requestChecksumRequired || requestChecksumCalculation == HttpChecksumConfigOption.WHEN_SUPPORTED
private val checksumHeader = StringBuilder("x-amz-checksum-")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Leaving checksumHeader as a StringBuilder means we have to call toString() repeatedly in the class. We'll know the value during class initialization so couldn't we fully build the string?

private val checksumHeader = buildString {
    append("x-amz-checksum-")
    append(userSelectedChecksumAlgorithm?.lowercase() ?: "crc32")
}

private val defaultChecksumAlgorithm = lazy { Crc32() }
private val defaultChecksumAlgorithmHeaderPostfix = "crc32"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Postfix -> Suffix


private val checksumAlgorithm = userSelectedChecksumAlgorithm?.let {
val hashFunction = userSelectedChecksumAlgorithm.toHashFunction()
if (hashFunction == null || !hashFunction.isSupported) {
throw ClientException("Checksum algorithm '$userSelectedChecksumAlgorithm' is not supported for flexible checksums")
}
checksumHeader.append(userSelectedChecksumAlgorithm.lowercase())
hashFunction
} ?: if (forcedToCalculateChecksum) {
checksumHeader.append(defaultChecksumAlgorithmHeaderPostfix)
defaultChecksumAlgorithm.value
} else {
null
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Seems unnecessary to declare forcedToCalculateChecksum, defaultChecksumAlgorithm, and defaultChecksumAlgorithmHeaderPostfix as fields only to use them in one if branch. Could this just be:

?: if (requestChecksumRequired || requestChecksumCalculation == HttpChecksumConfigOption.WHEN_SUPPORTED) {
    checksumHeader.append("crc32")
    Crc32()
} else


override suspend fun modifyBeforeSigning(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): HttpRequest {
val logger = coroutineContext.logger<FlexibleChecksumsRequestInterceptor<I>>()
val logger = coroutineContext.logger<FlexibleChecksumsRequestInterceptor>()

@Suppress("UNCHECKED_CAST")
val input = context.request as I
checksumAlgorithmName = checksumAlgorithmNameInitializer?.invoke(input) ?: context.executionContext.getOrNull(HttpOperationContext.ChecksumAlgorithm)
userProviderChecksumHeader(context.protocolRequest, logger)?.let {
logger.debug { "User supplied a checksum via header, skipping checksum calculation" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: These log messages which talk about the user feel awkward. Log messages are for users so it seems strange to mention the user in the third person. I'd suggest rewording these to be more about data/actions and less about the actors involved (e.g., "Found a provided checksum in the request, skipping checksum calculation").


checksumAlgorithmName ?: run {
logger.debug { "no checksum algorithm specified, skipping flexible checksums processing" }
return context.protocolRequest
val request = context.protocolRequest.toBuilder()
request.headers.removeAllChecksumHeadersExcept(it)
return request.build()
}

val req = context.protocolRequest.toBuilder()

check(context.protocolRequest.body !is HttpBody.Empty) {
"Can't calculate the checksum of an empty body"
if (checksumAlgorithm == null) {
logger.debug { "User didn't select a checksum algorithm and checksum calculation isn't required, skipping checksum calculation" }
return context.protocolRequest
}

val headerName = "x-amz-checksum-$checksumAlgorithmName".lowercase()
logger.debug { "Resolved checksum header name: $headerName" }

// remove all checksum headers except for $headerName
// this handles the case where a user inputs a precalculated checksum, but it doesn't match the input checksum algorithm
req.headers.removeAllChecksumHeadersExcept(headerName)

val checksumAlgorithm = checksumAlgorithmName?.toHashFunction() ?: throw ClientException("Could not parse checksum algorithm $checksumAlgorithmName")

if (!checksumAlgorithm.isSupported) {
throw ClientException("Checksum algorithm $checksumAlgorithmName is not supported for flexible checksums")
}
logger.debug { "Calculating checksum using '$checksumAlgorithm'" }

if (req.body.isEligibleForAwsChunkedStreaming) {
req.header("x-amz-trailer", headerName)
val request = context.protocolRequest.toBuilder()

if (request.body.isEligibleForAwsChunkedStreaming) {
val deferredChecksum = CompletableDeferred<String>(context.executionContext.coroutineContext.job)

if (req.headers[headerName] != null) {
logger.debug { "User supplied a checksum, skipping asynchronous calculation" }

val checksum = req.headers[headerName]!!
req.headers.remove(headerName) // remove the checksum header because it will be sent as a trailing header

deferredChecksum.complete(checksum)
} else {
logger.debug { "Calculating checksum asynchronously" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We've lost the log message that we're calculating a checksum asynchronously during transmission. I'd suggest moving your "Calculating checksum using '$checksumAlgorithm'" message into the if/else clauses, tweaking each to mention whether the checksum is being calculated before or during transmission.

req.body = req.body
.toHashingBody(checksumAlgorithm, req.body.contentLength)
.toCompletingBody(deferredChecksum)
}

req.trailingHeaders.append(headerName, deferredChecksum)
return req.build()
request.body = request.body
.toHashingBody(
checksumAlgorithm,
request.body.contentLength,
)
.toCompletingBody(
deferredChecksum,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Unnecessary wrapping


request.headers.append("x-amz-trailer", checksumHeader.toString())
request.trailingHeaders.append(checksumHeader.toString(), deferredChecksum)
} else {
return super.modifyBeforeSigning(context)
checksumAlgorithm.update(
request.body.readAll() ?: byteArrayOf(),
)
request.headers[checksumHeader.toString()] = checksumAlgorithm.digest().encodeBase64String()
}

context.executionContext.emitBusinessMetric(checksumAlgorithm.toBusinessMetric())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: I see here we're emitting the metric for the specific algorithm (e.g., FLEXIBLE_CHECKSUMS_REQ_CRC32C). Where are we emitting the metric for request/response enablement mode (e.g., FLEXIBLE_CHECKSUMS_REQ_WHEN_REQUIRED)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're emitting those metrics in the SDK side. Here and here.

request.headers.removeAllChecksumHeadersExcept(checksumHeader.toString())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: This is the second time this method calls removeAllChecksumHeadersExcept. Why does this need to be done twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be used given two different cases. The first is for when the checksum is provided via a header, we call it, then return. The second time is for when we computed the checksum ourselves, we call it, then return.


return request.build()
}

override suspend fun calculateChecksum(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): String? {
val req = context.protocolRequest.toBuilder()
val checksumAlgorithm = checksumAlgorithmName?.toHashFunction() ?: return null

if (checksumAlgorithm == null) return null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Null check should come before we call context.protocolRequest.toBuilder()


return when {
req.body.contentLength == null && !req.body.isOneShot -> {
Expand All @@ -121,12 +143,10 @@ public class FlexibleChecksumsRequestInterceptor<I>(
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
checksum: String,
): HttpRequest {
val headerName = "x-amz-checksum-$checksumAlgorithmName".lowercase()

val req = context.protocolRequest.toBuilder()

if (!req.headers.contains(headerName)) {
req.header(headerName, checksum)
if (!req.headers.contains(checksumHeader.toString())) {
req.header(checksumHeader.toString(), checksum)
}

return req.build()
Expand Down Expand Up @@ -210,4 +230,36 @@ public class FlexibleChecksumsRequestInterceptor<I>(
}
return hashFunction.digest()
}

/**
* Checks if a user provided a checksum for a request via an HTTP header.
* The header must start with "x-amz-checksum-" followed by the checksum algorithm's name.
* MD5 is not considered a valid checksum algorithm.
*/
private fun userProviderChecksumHeader(request: HttpRequest, logger: Logger): String? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: can be restructured as an extension function HttpRequest.checksumHeader(logger: Logger): String?

and naming: userProvidedChecksumHeader

request.headers.entries().forEach { header ->
val headerName = header.key.lowercase()
if (headerName.startsWith("x-amz-checksum-")) {
if (headerName.endsWith("md5")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is any header ending with md5 to be ignored (e.g., x-amz-checksum-definitely-not-md5)? Or precisely x-amz-checksum-md5 and nothing else?

logger.debug {
"User provided md5 request checksum via headers, md5 is not a valid algorithm, ignoring header"
}
} else {
return headerName
}
}
}
return null
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: "valid" → "supported" (in both KDoc and log message)


/**
* Maps supported hash functions to business metrics.
*/
private fun HashFunction.toBusinessMetric(): BusinessMetric = when (this) {
is Crc32 -> SmithyBusinessMetric.FLEXIBLE_CHECKSUMS_REQ_CRC32
is Crc32c -> SmithyBusinessMetric.FLEXIBLE_CHECKSUMS_REQ_CRC32C
is Sha1 -> SmithyBusinessMetric.FLEXIBLE_CHECKSUMS_REQ_SHA1
is Sha256 -> SmithyBusinessMetric.FLEXIBLE_CHECKSUMS_REQ_SHA256
else -> throw IllegalStateException("Checksum was calculated using an unsupported hash function: $this")
}
}
Loading
Loading