feat: support default checksums #1475
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… jmespath-flatten
… flexible-checksums
This comment has been minimized.
This comment has been minimized.
0marperez
added
the
acknowledge-artifact-size-increase
|
A new generated diff is ready to view. |
This comment has been minimized.
This comment has been minimized.
lauzadis
reviewed
Dec 13, 2024
| /** | ||
| * For any operations that require a checksum, set CRC32 if the user has not already configured a checksum. | ||
| */ | ||
| private val useCrc32Checksum = object : ProtocolMiddleware { |
There was a problem hiding this comment.
question/correctness: S3 Express' spec says CRC32 is required. If we change our SDK's default algorithm to CRC64NVME in the future, how will we ensure CRC32 is still used for S3 Express? That's what this integration was responsible for.
Does S3 Express support CRC64NVME?
| 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" } |
There was a problem hiding this comment.
info: Log the composite checksum header name
Comment on lines
18
to
19
| * 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. |
There was a problem hiding this comment.
correctness/breaking change: I don't think we should be removing these headers, what if users need them for other purposes?
Isn't it easier to just ignore composite checksums in the flexible checksums implementation?
| /** | ||
| * Disable checksums entirely for s3:UploadPart requests. | ||
| * Disables checksums for s3:UploadPart requests that use S3 express. |
Comment on lines
-7
to
-12
| @{protocol-name} | ||
| @restJson1 | ||
| @sigv4(name: "event-stream-test") | ||
| @service(sdkId: "EventStreamTest") | ||
| service TestService { version: "123", operations: [TestStreamOp] } | ||
|
|
||
| {op-traits} |
There was a problem hiding this comment.
I added the templates back
I still don't see them
buildSrc/build.gradle.kts
Outdated
There was a problem hiding this comment.
Does this need to exist? buildSrc is treated as an included build which means every subproject will have these config options applied.
It seems odd to need this just for codegen tests
There was a problem hiding this comment.
It's the best way I could find to share a data class between subprojects, do you know of alternatives that would work better? I might've missed them
There was a problem hiding this comment.
You can make a new project that both of those subprojects depend on
There was a problem hiding this comment.
This is still a work in progress, I don't want to block the review because of this. I think worst case scenario we can come back and refactor this without it being a breaking change
| @@ -265,6 +277,45 @@ class PresignerGenerator : KotlinIntegration { | |||
| ) | |||
| } | |||
|
|
|||
| checksumAlgorithmMember(op, ctx)?.let { checksumAlgorithmMember -> | |||
| withBlock("input.#L?.value?.let { checksumAlgorithmString ->", "}", checksumAlgorithmMember) { | |||
There was a problem hiding this comment.
nit/naming/style: avoid Hungarian notation, checksumAlgorithmName
| "#T.#T<Presigner> { #S }", | ||
| coroutineContext, | ||
| warn, | ||
| "The requested checksum algorithm is not supported for pre-signed URL checksums, sending request without checksum.", |
There was a problem hiding this comment.
We should include which checksum algorithm the user chose and which are supported
| @@ -287,4 +338,24 @@ class PresignerGenerator : KotlinIntegration { | |||
| * > "my-object/example/photo.user". This is an incorrect path for that object. | |||
| */ | |||
| private fun normalizeUriPath(service: ServiceShape) = service.sdkId != "S3" | |||
|
|
|||
| /** | |||
| * Gets the checksum algorithm member if a user can configure request checksums otherwise null | |||
There was a problem hiding this comment.
if a user can configure request checksums
style/docs: This seems awkward to me
"Gets the checksum algorithm member if configured on a request, otherwise null"
| resolved + flexibleChecksumsRequestMiddleware | ||
| resolved + flexibleChecksumsRequestMiddleware + configBusinessMetrics | ||
|
|
||
| private val configBusinessMetrics = object : ProtocolMiddleware { |
There was a problem hiding this comment.
nit: middleware name and value's name should match
… flexible-checksums
|
A new generated diff is ready to view. |
|
A new generated diff is ready to view. |
lauzadis
requested changes
Dec 19, 2024
| import aws.smithy.kotlin.runtime.http.interceptors.FlexibleChecksumsResponseInterceptor | ||
|
|
||
| /** | ||
| * S3 variant of the flexible checksum interceptor where composite checksums are not validated |
There was a problem hiding this comment.
nit/docs: ... of the [FlexibleChecksumsResponseInterceptor]...
There was a problem hiding this comment.
correctness: This file should go in services/s3/common/src/aws/sdk/kotlin/services/s3/internal with all the other S3-specific interceptors
| /** | ||
| * Determines if an operation is set up to send flexible request checksums | ||
| */ | ||
| private fun requestChecksumsConfigured(ctx: ProtocolGenerator.GenerationContext, op: OperationShape): Boolean { |
There was a problem hiding this comment.
naming: "configured" implies that this is dependent on user behavior, but it's just based on the model. I suggest making this an extension function OperationShape.hasRequestAlgorithmMember(...)
Comment on lines
96
to
100
| val interceptor = if (ctx.model.expectShape<ServiceShape>(ctx.settings.service).isS3) { | ||
| AwsRuntimeTypes.Http.Interceptors.S3FlexibleChecksumResponseInterceptor | ||
| } else { | ||
| RuntimeTypes.HttpClient.Interceptors.FlexibleChecksumsResponseInterceptor | ||
| } |
There was a problem hiding this comment.
docs: let's add a comment here about why S3 needs a custom interceptor
There was a problem hiding this comment.
Now that I look at this again, we could replace S3FlexibleChecksumResponseInterceptor with something more generic that could be applied to other services. In case other services start sending composite checksums in the future. What does the team think?
Comment on lines
36
to
37
| private fun usingS3Express(executionContext: ExecutionContext): Boolean = | ||
| executionContext.getOrNull(AttributeKey(S3_EXPRESS_ENDPOINT_PROPERTY_KEY)) != S3_EXPRESS_ENDPOINT_PROPERTY_VALUE |
There was a problem hiding this comment.
style: Could be an extension function off ExecutionContext
Comment on lines
+21
to
+23
| internal class S3ExpressDefaultChecksumAlgorithm( | ||
| private val isS3UploadPart: Boolean, | ||
| ) : HttpInterceptor { |
There was a problem hiding this comment.
style: This works but I think the previous implementation using two different interceptors was cleaner. It's not clear to me why this had to deviate so much from what we already had
Comment on lines
+43
to
+48
| - name: Save Test Reports | ||
| if: failure() | ||
| uses: actions/upload-artifact@v3 | ||
| with: | ||
| name: test-reports | ||
| path: '**/build/reports' |
There was a problem hiding this comment.
What are these test reports used for?
|
A new generated diff is ready to view. |
|
|
A new generated diff is ready to view. |
Affected ArtifactsChanged in size
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.