Skip to content
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

feat: support default checksums #1191

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
Comment on lines +29 to +31
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
}
Comment on lines +60 to +75
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" }
Comment on lines +80 to +81
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,
)

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())
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?


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
Comment on lines 125 to +127
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")) {
Comment on lines +242 to +243
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
}
Comment on lines +234 to +253
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