feat: support default checksums #1191

0marperez · 2024-11-27T10:53:55Z

Issue #

Description of changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Signed-off-by: 0marperez <[email protected]>

…flexible-checksums

github-actions · 2024-11-27T10:57:43Z

Affected Artifacts

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
smithy-client-jvm.jar	65,382	62,681	2,701	4.31%
http-client-jvm.jar	319,197	314,811	4,386	1.39%
runtime-core-jvm.jar	811,421	811,044	377	0.05%

0marperez · 2024-11-27T16:01:36Z

Note to reviewers: I'll address the breaking API changes and failing protocol issues soon. In the meantime, I’d appreciate a review of the checksum-related changes.

lauzadis · 2024-11-27T19:19:09Z

...ommon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

+ * 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


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

lauzadis · 2024-11-27T19:21:12Z

...ommon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

+ *
+ * @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.


naming: requestChecksumAlgorithm

lauzadis · 2024-11-27T19:21:55Z

...ommon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

+    private val forcedToCalculateChecksum = requestChecksumRequired || requestChecksumCalculation == HttpChecksumConfigOption.WHEN_SUPPORTED
+    private val checksumHeader = StringBuilder("x-amz-checksum-")
+    private val defaultChecksumAlgorithm = lazy { Crc32() }
+    private val defaultChecksumAlgorithmHeaderPostfix = "crc32"


Postfix -> Suffix

lauzadis · 2024-11-27T19:24:30Z

...ommon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

+     * 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? {


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

and naming: userProvidedChecksumHeader

lauzadis · 2024-11-27T19:30:00Z

...mmon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor.kt

 * Users can check which checksum was validated by referencing the `ResponseChecksumValidated` execution context variable.
 *
- * @param shouldValidateResponseChecksumInitializer A function which uses the input [I] to return whether response checksum validation should occur
+ * @param responseValidationRequired Flag indicating if the checksum validation is mandatory.


docs: "Model sourced flag"

lauzadis · 2024-11-27T19:40:29Z

...mmon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor.kt

+            .removePrefix("x-amz-checksum-")
+            .toHashFunction() ?: throw ClientException("Could not parse checksum algorithm from header $checksumHeader")
+
+        if (context.protocolResponse.body is HttpBody.Bytes) {


Why was this branch added? The spec says we should delay checksum calculation until the body is consumed:

Where possible, SDKs MUST defer this calculation and validation until the payload is actually consumed by the user i.e. a payload must not be read twice.

toHashingBody doesn't support Bytes bodies. I decided to calculate the checksum in memory instead of adding support for Bytes to toHashingBody. It should be safe if the request is not being streamed but I think you're right that we're not following the spec

toHashingBody doesn't support Bytes bodies

Does it need to? The previous implementation never seemed to need it

lauzadis · 2024-11-27T19:41:28Z

.../test/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptorTest.kt

@@ -126,23 +133,6 @@ class FlexibleChecksumsRequestInterceptorTest {
        assertEquals(0, call.request.headers.getNumChecksumHeaders())
    }

-    @Test
-    fun itSetsChecksumHeaderViaExecutionContext() = runTest {


Why was this test removed?

lauzadis · 2024-11-27T19:41:52Z

...test/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptorTest.kt

@@ -163,29 +168,4 @@ class FlexibleChecksumsResponseInterceptorTest {

        op.roundTrip(client, TestInput("input"))
    }
-
-    @Test
-    fun testSkipsValidationWhenDisabled() = runTest {


Why was this test removed?

lauzadis · 2024-11-27T19:43:48Z

...smithy-client/common/src/aws/smithy/kotlin/runtime/client/config/HttpChecksumClientConfig.kt

+
+public enum class HttpChecksumConfigOption {
+    /**
+     * SDK will create/validate checksum if the service marks it as required or if this is set.


docs: create -> calculate

lauzadis · 2024-11-27T19:54:55Z

.../test/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptorTest.kt

correctness: missing tests for when requestChecksumRequired = false and requestChecksumCalculation = HttpChecksumConfigOption.WHEN_REQUIRED. Same for response validation

ianbotsf · 2024-11-27T19:02:29Z

...ommon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

- * @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.


Nit: "asynchronously" → "during transmission"

ianbotsf · 2024-11-27T19:03:02Z

...ommon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

+ * - 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


Nit: "synchronously" → "before transmission"

ianbotsf · 2024-11-27T19:31:01Z

...ommon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

+    private val defaultChecksumAlgorithm = lazy { Crc32() }
+    private val defaultChecksumAlgorithmHeaderPostfix = "crc32"
+
+    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
+    }


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

ianbotsf · 2024-11-27T19:35:19Z

...ommon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

+        userProviderChecksumHeader(context.protocolRequest, logger)?.let {
+            logger.debug { "User supplied a checksum via header, skipping checksum calculation" }


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").

ianbotsf · 2024-11-27T19:47:52Z

...ommon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt

-
-                deferredChecksum.complete(checksum)
-            } else {
-                logger.debug { "Calculating checksum asynchronously" }


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.

ianbotsf · 2024-11-27T20:14:57Z

...mmon/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor.kt

+private fun String.isCompositeChecksum(): Boolean {
+    // Ends with "-#" where "#" is a number between 1-1000
+    val regex = Regex("-([1-9][0-9]{0,2}|1000)$")
+    return regex.containsMatchIn(this)
+}


Nit: When this logic is made S3 specific, we shouldn't assert on the part number fitting inside a certain range. Just "-(\d)+$" should suffice.

ianbotsf · 2024-11-27T20:16:29Z

.../test/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptorTest.kt

Nit: Please add tests for HttpChecksumConfigOption.WHEN_REQUIRED. Applies to response interceptors as well.

ianbotsf · 2024-11-27T21:08:36Z

.../test/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptorTest.kt

-    @Test
-    fun itSetsChecksumHeaderViaExecutionContext() = runTest {
-        checksums.forEach { (checksumAlgorithmName, expectedChecksumValue) ->
-            val req = HttpRequestBuilder().apply {
-                body = HttpBody.fromBytes("<Foo>bar</Foo>".encodeToByteArray())
-            }
-
-            val op = newTestOperation<Unit, Unit>(req, Unit)
-            op.context[HttpOperationContext.ChecksumAlgorithm] = checksumAlgorithmName
-            op.interceptors.add(FlexibleChecksumsRequestInterceptor<Unit>())
-
-            op.roundTrip(client, Unit)
-            val call = op.context.attributes[HttpOperationContext.HttpCallList].first()
-            assertEquals(expectedChecksumValue, call.request.headers["x-amz-checksum-$checksumAlgorithmName"])
-        }
-    }


Question: Do we still use HttpOperationContext.ChecksumAlgorithm or can it be deprecated?

ianbotsf · 2024-11-27T21:10:10Z

...test/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptorTest.kt

-    @Test
-    fun testSkipsValidationWhenDisabled() = runTest {
-        val req = HttpRequestBuilder()
-        val op = newTestOperation<TestInput>(req)
-
-        op.interceptors.add(
-            FlexibleChecksumsResponseInterceptor<TestInput> {
-                false
-            },
-        )
-
-        val responseChecksumHeaderName = "x-amz-checksum-crc32"
-
-        val responseHeaders = Headers {
-            append(responseChecksumHeaderName, "incorrect-checksum-would-throw-if-validated")
-        }
-
-        val client = getMockClient(response, responseHeaders)
-
-        val output = op.roundTrip(client, TestInput("input"))
-        output.body.readAll()
-
-        assertNull(op.context.getOrNull(ChecksumHeaderValidated))
-    }


Question: Why did we delete this test?

ianbotsf · 2024-11-27T21:13:01Z

...smithy-client/common/src/aws/smithy/kotlin/runtime/client/config/HttpChecksumClientConfig.kt

+    /**
+     * SDK will create/validate checksum if the service marks it as required or if this is set.
+     */
+    WHEN_SUPPORTED,


Nit: "or if this is set" → "or if the service offers optional checksums"

0marperez added 10 commits November 10, 2024 22:28

Bump smithy IDL version

2a9d104

Signed-off-by: 0marperez <[email protected]>

Add requestChecksumCalculation config option

205839c

Added responseChecksumValidation

7233b71

Add todos for business metrics

e1dc616

Unit tests pass

e436482

Merge branch 'main' of https://github.com/awslabs/smithy-kotlin into …

abdba02

…flexible-checksums

E2E tests pass

3e4c891

Self review

9760ee1

Self review 2

f8b39b0

Smithy codegen version bump

f676b7b

0marperez added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Nov 27, 2024

This comment has been minimized.

Sign in to view

lauzadis reviewed Nov 27, 2024

View reviewed changes

ianbotsf reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support default checksums #1191

feat: support default checksums #1191

0marperez commented Nov 27, 2024

This comment has been minimized.

github-actions bot commented Nov 27, 2024

0marperez commented Nov 27, 2024

lauzadis Nov 27, 2024

lauzadis Nov 27, 2024

lauzadis Nov 27, 2024

lauzadis Nov 27, 2024

lauzadis Nov 27, 2024

lauzadis Nov 27, 2024

0marperez Nov 27, 2024

lauzadis Nov 27, 2024 •

edited

Loading

lauzadis Nov 27, 2024

lauzadis Nov 27, 2024

lauzadis Nov 27, 2024

lauzadis Nov 27, 2024

ianbotsf Nov 27, 2024

ianbotsf Nov 27, 2024

ianbotsf Nov 27, 2024

ianbotsf Nov 27, 2024

ianbotsf Nov 27, 2024

ianbotsf Nov 27, 2024

ianbotsf Nov 27, 2024

ianbotsf Nov 27, 2024

ianbotsf Nov 27, 2024

ianbotsf Nov 27, 2024

		userProviderChecksumHeader(context.protocolRequest, logger)?.let {
		logger.debug { "User supplied a checksum via header, skipping checksum calculation" }

feat: support default checksums #1191

Are you sure you want to change the base?

feat: support default checksums #1191

Conversation

0marperez commented Nov 27, 2024

Issue #

Description of changes

This comment has been minimized.

github-actions bot commented Nov 27, 2024

Affected Artifacts

0marperez commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauzadis Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauzadis Nov 27, 2024 •

edited

Loading