kn/misc: enable various ignored tests #1257

lauzadis · 2025-03-20T14:50:08Z

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.

…tlin as an includeBuild

This reverts commit 3580da4.

ianbotsf · 2025-03-24T21:44:53Z

...h/aws-signing-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/RequestMutator.kt

+internal val AwsSigningConfig.credentialScope: String
+    get() = run {
+        val signingDate = signingDate.format(TimestampFormat.ISO_8601_CONDENSED_DATE)
+        return when (algorithm) {
+            AwsSigningAlgorithm.SIGV4 -> "$signingDate/$region/$service/aws4_request"
+            AwsSigningAlgorithm.SIGV4_ASYMMETRIC -> "$signingDate/$service/aws4_request"
+        }
+    }


Nit: The run function is good for assignments but just adds clutter to function definitions. Just use braces:

internal val AwsSigningConfig.credentialScope: String get() { val signingDate = signingDate.format(TimestampFormat.ISO_8601_CONDENSED_DATE) return when (algorithm) { AwsSigningAlgorithm.SIGV4 -> "$signingDate/$region/$service/aws4_request" AwsSigningAlgorithm.SIGV4_ASYMMETRIC -> "$signingDate/$service/aws4_request" } }

ianbotsf · 2025-03-24T23:12:59Z

...aws-signing-default/jvm/src/aws/smithy/kotlin/runtime/auth/awssigning/DefaultAwsSignerJVM.kt

+/** Creates a customized instance of [AwsSigner] */
+@Suppress("ktlint:standard:function-naming")
+public fun DefaultAwsSigner(block: DefaultAwsSignerBuilder.() -> Unit): AwsSigner =
+    DefaultAwsSignerBuilder().apply(block).build()
+
+/** A builder class for creating instances of [AwsSigner] using the default implementation */
+public class DefaultAwsSignerBuilder {
+    public var telemetryProvider: TelemetryProvider? = null
+
+    public fun build(): AwsSigner = DefaultAwsSignerImpl(
+        telemetryProvider = telemetryProvider,
+    )
+}


Opinion: It now feels weird that this factory function and builder type exist only in JVM when the named value exists in all source sets.

Discussed separately and agreed that this API disparity is fine for now and can be reexamined if we ever see problems or get feedback.

ianbotsf · 2025-03-24T23:13:41Z

...aws-signing-default/jvm/src/aws/smithy/kotlin/runtime/auth/awssigning/DefaultAwsSignerJVM.kt

+/** The default implementation of [AwsSigner] */
+public actual val DefaultAwsSigner: AwsSigner
+    get() = DefaultAwsSignerImpl()


Question: Why is a new signer instantiated on every call? Before it was initialized only once and then reused everywhere.

Will be refactored, same applies for CrtAwsSigner

ianbotsf · 2025-03-24T23:14:33Z

...gning-default/native/src/aws/smithy/kotlin/runtime/auth/awssigning/DefaultAwsSignerNative.kt

+/** The default implementation of [AwsSigner] */
+public actual val DefaultAwsSigner: AwsSigner
+    get() = CrtAwsSigner


Question: Do we have to use a getter for this or can we just set the value one time?

public actual val DefaultAwsSigner: AwsSigner = CrtAwsSigner

ianbotsf · 2025-03-24T23:23:56Z

runtime/crt-util/native/src/aws/smithy/kotlin/runtime/crt/ReadChannelBodyStreamNative.kt

-internal actual fun transferRequestBody(outgoing: SdkBuffer, dest: MutableBuffer): Int = TODO("Not yet implemented")
+internal actual fun transferRequestBody(outgoing: SdkBuffer, dest: MutableBuffer): Int {
+    val length = minOf(outgoing.size, dest.writeRemaining.toLong())
+    if (length <= 0) return 0
+    return dest.write(outgoing.readByteArray(length))
+}


Question: Why is this different than the JVM (formerly common) implementation? Why won't simply outgoing.read(dest.buffer) work?

dest.buffer exists only on JVM

ianbotsf · 2025-03-24T23:24:49Z

.../jvmAndNative/test/aws/smithy/kotlin/runtime/http/engine/crt/SdkStreamResponseHandlerTest.kt

-    @IgnoreNative // FIXME Re-enable after Kotlin/Native Implementation.
    @Test
    fun testStreamError() = runTest {
+        CRT.initRuntime()


Question: This shouldn't properly be here, right? Does this need a FIXME because some underlying component is not properly initializing the CRT?

I could not decide which underlying component needs to initialize CRT in this test. This is testing the SdkStreamResponseHandler which is a callback machine for reading a CRT HttpStream. We're not creating a CRT engine or anything else which might initialize CRT here.

The only reason this initRuntime is needed is for the final assertion:
ex.message.shouldContain("socket is closed.; crtErrorCode=$socketClosedEc")

CRT needs to be initialized to return human-readable error codes, otherwise it returns "unknown". This assertion is on an exception message which we typically avoid doing, so I could remove that assertion / remove CRT.init?

Yes removing the CRT.initRuntime() is probably the simplest thing to do for this test. But I still worry we have our CRT initialization calls in so many disparate places and that we may still be missing the calls in other places.

I'll keep the CRT.initRuntime() and the assertion in place. I'll add a comment* explaining why we need to initialize the CRT here.

ianbotsf · 2025-03-24T23:26:11Z

...ocol/http-client/common/test/aws/smithy/kotlin/runtime/http/operation/HttpInterceptorTest.kt

-        val cause = assertNotNull(ex.cause)
+        val cause = assertNotNull(ex.cause ?: ex)


Question: Why is there a fallback in this test? Why would the exception have a cause in one run/target but not another?

I spent a few hours on this one and could not figure out the root cause.

On JVM, the exception ex is a copy of its cause (ex.cause), but the cause is the one which has all of the suppressed exceptions. On all other platforms, there is just a single exception ex which holds all of the suppressed exceptions.

This test is focused on suppressed exceptions, which do work consistently across platforms, so I think it's ok. I could leave a FIXME / open up a backlog task to investigate the ex.cause if you'd like?

Yes it'd be good to understand what's duplicating it. A FIXME seems correct.

ianbotsf · 2025-03-24T23:27:46Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/hashing/EcdsaNative.kt

-public actual fun ecdsaSecp256r1(key: ByteArray, message: ByteArray): ByteArray = TODO("Not yet implemented")
+public actual fun ecdsaSecp256r1(key: ByteArray, message: ByteArray): ByteArray = error("This function should not be invoked on Native, which uses the CrtAwsSigner.")


Nit: It feels weird to have an actual function that we'll never implement and should never call. That, to me, indicates that our expect declaration is on the wrong component and should be higher up.

ianbotsf · 2025-03-24T23:29:11Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/hashing/EcdsaNative.kt

-// FIXME Implement using aws-c-cal: https://github.com/awslabs/aws-c-cal/blob/main/include/aws/cal/ecc.h
-// Will need to be implemented and exposed in aws-crt-kotlin. Or maybe we can _only_ offer the CRT signer on Native?
-// Will require updating DefaultAwsSigner to be expect/actual and set to CrtSigner on Native.


Question: Did we go down the road of binding to CRT's crypto primitives and abandon it for some reason? Or did we just decide to use the whole CRT signer for a more important reason?

No, I didn't even start binding to CRT for this. We previously discussed using CrtAwsSigner as the default on Native so that's where I went.

It sounds like you've changed your mind on this, we can talk about it and decide what to do

We agreed to keep the CrtAwsSigner as default on Native and evaluate the decision if/when we get user feedback

lauzadis added 18 commits March 19, 2025 09:42

Re-enable aws-signing-tests

dd715a1

Re-enable testReadAfterExecutionSuppressedException

61c9b69

ktlint

f5660b5

Re-enable some tests

1281338

Initialize CRT in CrtAwsSigner

986907f

Update FIXME message

9f8999e

Ignore SigV4a test on Native

3fca267

Re-enable AwsChunkedByteReadChannelTestBase

ba8f1cb

Re-enable AwsChunkedTestBase

3bbc14a

Initialize CRT runtime to get correct error code

00d10a5

Update FIXME message

eac0502

ktlint

a0d2fc5

Implement transferRequestBody

d8878a3

Update FIXME message

1eebd5e

Make DefaultAwsSigner expect/actual, point to CRT signer on Native

27af57e

ktlint

8d73a86

Unignore passing test

ed5da6b

Update FIXME message

5ad9d5e

lauzadis requested a review from a team as a code owner March 20, 2025 14:50

lauzadis added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Mar 20, 2025

CI

cf45c86

lauzadis added the acknowledge-api-break Acknowledge that a change is API breaking and may be backwards-incompatible. Review carefully! label Mar 21, 2025

CI

6b1756b

0marperez approved these changes Mar 24, 2025

View reviewed changes

lauzadis added 6 commits March 24, 2025 10:58

CI

eeb8463

Use setup-build action in artifact-size-metrics to pull in aws-crt-ko…

c5905cd

…tlin as an includeBuild

Try to fix path

11dac4e

CI glitch: size-check Expected — Waiting for status to be reported

2857ecd

Temporarily remove size-check

3580da4

Revert "Temporarily remove size-check"

d4bb8e4

This reverts commit 3580da4.

lauzadis added acknowledge-artifact-size-increase and removed acknowledge-artifact-size-increase labels Mar 24, 2025

ianbotsf reviewed Mar 24, 2025

View reviewed changes

lauzadis added 3 commits March 25, 2025 11:56

Add a FIXME comment for ex / ex.cause

56d6f71

Address nit / run { ... }

4ea1a06

Remove getter for DefaultAwsSigner values

d17bf67

ianbotsf approved these changes Mar 25, 2025

View reviewed changes

Add an explanation for CRT.initRuntime

5621fe0

aws-sdk-kotlin-ci merged commit f7c8f82 into kn-main Mar 26, 2025
18 checks passed

aws-sdk-kotlin-ci deleted the kn-misc branch March 26, 2025 14:24

		val cause = assertNotNull(ex.cause)
		val cause = assertNotNull(ex.cause ?: ex)

		public actual fun ecdsaSecp256r1(key: ByteArray, message: ByteArray): ByteArray = TODO("Not yet implemented")
		public actual fun ecdsaSecp256r1(key: ByteArray, message: ByteArray): ByteArray = error("This function should not be invoked on Native, which uses the CrtAwsSigner.")

kn/misc: enable various ignored tests #1257

kn/misc: enable various ignored tests #1257

Uh oh!

Conversation

lauzadis commented Mar 20, 2025

Issue #

Description of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lauzadis Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lauzadis Mar 25, 2025 •

edited

Loading