move checksumming operations to a interceptor #3140

sbiscigl · 2024-10-09T14:53:53Z

Description of changes:

Moves the checksumming operations of the SDK behind a SRA interceptor for better modularization

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

sbiscigl · 2024-10-09T14:57:26Z

src/aws-cpp-sdk-core/include/smithy/interceptor/impl/ChecksumInterceptor.h

+            ChecksumInterceptor& operator=(ChecksumInterceptor&& other) noexcept = default;
+            ~ChecksumInterceptor() override = default;
+
+            ModifyRequestOutcome ModifyRequest(InterceptorContext& context) override


all of the code in this file is copied from the existing implentation. I will refactor the logic in the next PR. This PR more or less jsut refactors the code out to a single location before handling the actual logical refactor. So while we can address comments on the logic here, a lot will be changing to it in the next PR.

sbiscigl · 2024-10-09T14:58:58Z

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

-            else if (checksumAlgorithmName == "crc32")
-            {
-                std::shared_ptr<Aws::Utils::Crypto::CRC32> crc32 = Aws::MakeShared<Aws::Utils::Crypto::CRC32>(AWS_CLIENT_LOG_TAG);
-                httpRequest->AddResponseValidationHash("crc", crc32);


this is actually a bug right now, this will result in response validation not being done on for objects using crc32. This is fixed in the interceptor refactor.

SergeyRyabinin · 2024-10-09T18:29:14Z

src/aws-cpp-sdk-core/source/smithy/client/AwsSmithyClientBase.cpp

+    context.SetRequest(pRequestCtx->m_httpRequest);
+    for (const auto& interceptor : m_interceptors)
+    {
+        auto modifiedRequest = interceptor->ModifyRequest(context);


for the interceptor API,
please rename ModifyRequest into something like ModifyBeforeSigningRequest to be compliant with the internal reference design.

yeah that was a miss on my part, it aligns with rusts as well, updating

sbera87 · 2024-10-10T16:58:39Z

src/aws-cpp-sdk-core/include/smithy/interceptor/impl/ChecksumInterceptor.h

+            {
+                const auto& httpRequest = context.GetTransmitRequest();
+                const auto& modeledRequest = context.GetModeledRequest();
+                if (httpRequest == nullptr)


nit : can also use if (!httpRequest)

this is the code style that crt uses so i wanted to keep it consistent

if thats the case, its my advice adopt this to avoid typo '='
if (nullptr == httpRequest)

sbera87 · 2024-10-10T17:04:35Z

src/aws-cpp-sdk-core/include/smithy/interceptor/impl/ChecksumInterceptor.h

+                    }
+                    else if (checksumAlgorithmName == "crc32")
+                    {
+                        if (modeledRequest.IsStreaming())


cleanup: this block can be wrapped in a lambda accepting std:function which can be Aws::Utils::HashingUtils::CalculateCRC32 or Aws::Utils::HashingUtils::CalculateCRC32C or Aws::Utils::HashingUtils::CalculateSHA256
Rest is the same. Just easy to read and more code reuse.

Also we should move away from hardcoded strings like "crc32" unless they are constants defined or just use enum.
And move to switch case, which is much faster than if/else

from my other comment

all of the code in this file is copied from the existing implentation. I will refactor the logic in the next PR. This PR more or less jsut refactors the code out to a single location before handling the actual logical refactor. So while we can address comments on the logic here, a lot will be changing to it in the next PR.

This code could be cleaned up in a lot of different ways, and we plan on it. Its actually the next PR im going to raise. I dont like how this is strucutred and we ill be moving away from this giant if-else block alltogether. This PR is more focused on moving it out of the client and compartmentalizing it, then cleaning it up

sbera87 · 2024-10-10T17:25:17Z

src/aws-cpp-sdk-core/include/smithy/interceptor/impl/ChecksumInterceptor.h

+                const auto& request = context.GetModeledRequest();
+                const auto transmitRequest = context.GetTransmitRequest();
+                const auto response = context.GetTransmitResponse();
+                if (transmitRequest == nullptr || transmitRequest == nullptr)


nit: condense if (!(transmitRequest )
Duplicate : if (transmitRequest == nullptr || transmitRequest == nullptr), you meant response

you meant response

yep good catch

nit: condense if (!(transmitRequest ))

this is the code style that crt uses so i wanted to keep it consistent

sbera87 · 2024-10-10T17:30:37Z

src/aws-cpp-sdk-core/include/smithy/interceptor/impl/ChecksumInterceptor.h

+                {
+                    for (const Aws::String& responseChecksumAlgorithmName : modeledRequest.GetResponseChecksumAlgorithmNames())
+                    {
+                        const auto lowered = Aws::Utils::StringUtils::ToLower(responseChecksumAlgorithmName.c_str());


cleanup: can be condensed for readability/reusability. One approach is we can use base class for different hash types and invoke a method on base type to add response validation hash from a const set of supported hash type clases

from my other comment

all of the code in this file is copied from the existing implentation. I will refactor the logic in the next PR. This PR more or less jsut refactors the code out to a single location before handling the actual logical refactor. So while we can address comments on the logic here, a lot will be changing to it in the next PR.

This code could be cleaned up in a lot of different ways, and we plan on it. Its actually the next PR im going to raise. I dont like how this is strucutred and we ill be moving away from this giant if-else block alltogether. This PR is more focused on moving it out of the client and compartmentalizing it, then cleaning it up

sbera87 · 2024-10-10T17:31:28Z

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

@@ -543,6 +547,17 @@ HttpResponseOutcome AWSClient::AttemptOneRequest(const std::shared_ptr<Aws::Http
        *m_telemetryProvider->getMeter(this->GetServiceClientName(), {}),
        {{TracingUtils::SMITHY_METHOD_DIMENSION, request.GetServiceRequestName()},{TracingUtils::SMITHY_SERVICE_DIMENSION, this->GetServiceClientName()}});

+    InterceptorContext context{request};
+    context.SetTransmitRequest(httpRequest);
+    for (const auto& interceptor : m_interceptors)


where is the check for m_interceptors empty? should it be empty?

m_interceptors could be empty in which case we apply zero interceptors. infact a optimization we could make in teh future is make per-client interceptors. right now all clients will execute a checksum interceptors as they did before. but in the future we could optimize it where if a client doesnt do any checksumming, it will not compose the interceptor

sbera87 · 2024-10-10T17:34:11Z

src/aws-cpp-sdk-core/source/client/AWSClient.cpp

@@ -570,27 +585,13 @@ HttpResponseOutcome AWSClient::AttemptOneRequest(const std::shared_ptr<Aws::Http
        *m_telemetryProvider->getMeter(this->GetServiceClientName(), {}),
        {{TracingUtils::SMITHY_METHOD_DIMENSION, request.GetServiceRequestName()},{TracingUtils::SMITHY_SERVICE_DIMENSION, this->GetServiceClientName()}});

-    if (request.ShouldValidateResponseChecksum())
+    context.SetTransmitResponse(httpResponse);
+    for (const auto& interceptor : m_interceptors)


what if interceptors is empty. will the workflow still be correct, if not maybe return an error

m_interceptors could be empty in which case we apply zero interceptors. infact a optimization we could make in teh future is make per-client interceptors. right now all clients will execute a checksum interceptors as they did before. but in the future we could optimize it where if a client doesnt do any checksumming, it will not compose the interceptor

sbera87 · 2024-10-10T17:35:39Z

src/aws-cpp-sdk-core/include/smithy/interceptor/InterceptorContext.h

            }

        private:
            Aws::Map<Aws::String, Aws::String> m_attributes{};
-            std::shared_ptr<Aws::Http::HttpRequest> m_request{nullptr};
-            std::shared_ptr<Aws::Http::HttpResponse> m_response{nullptr};
+            const Aws::AmazonWebServiceRequest& m_modeledRequest;


storing reference? what happens if caller out of scope. is that possible in this workflow?

nope its a borrow from the parent scope, in the current workflow its not possible

sbera87

looks good overall. Minor changes needed in error checking/handling. Raised some questions for just robustness.

sbiscigl commented Oct 9, 2024

View reviewed changes

sbiscigl mentioned this pull request Oct 9, 2024

Upload fails with InvalidChunkSizeError for large objects #3132

Closed

SergeyRyabinin reviewed Oct 9, 2024

View reviewed changes

SergeyRyabinin approved these changes Oct 9, 2024

View reviewed changes

sbiscigl force-pushed the checksum-interceptors branch from e690685 to 6fbdbca Compare October 10, 2024 15:30

sbera87 reviewed Oct 10, 2024

View reviewed changes

sbera87 approved these changes Oct 10, 2024

View reviewed changes

sbiscigl force-pushed the checksum-interceptors branch 2 times, most recently from efc0b6b to 79ac465 Compare October 10, 2024 20:06

SergeyRyabinin approved these changes Oct 10, 2024

View reviewed changes

sbiscigl force-pushed the checksum-interceptors branch from 79ac465 to e02c64f Compare October 10, 2024 23:12

move checksumming operations to a interceptor

96f1300

sbiscigl force-pushed the checksum-interceptors branch from e02c64f to 96f1300 Compare October 11, 2024 13:10

sbiscigl marked this pull request as ready for review October 11, 2024 15:14

sbiscigl merged commit ac9296d into main Oct 11, 2024
4 checks passed

sbiscigl deleted the checksum-interceptors branch October 11, 2024 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move checksumming operations to a interceptor #3140

move checksumming operations to a interceptor #3140

sbiscigl commented Oct 9, 2024

sbiscigl Oct 9, 2024

sbiscigl Oct 9, 2024

SergeyRyabinin Oct 9, 2024

sbiscigl Oct 9, 2024

sbera87 Oct 10, 2024

sbiscigl Oct 10, 2024

sbera87 Oct 10, 2024

sbera87 Oct 10, 2024

sbiscigl Oct 10, 2024

sbera87 Oct 10, 2024

sbiscigl Oct 10, 2024

sbera87 Oct 10, 2024

sbiscigl Oct 10, 2024

sbera87 Oct 10, 2024

sbiscigl Oct 10, 2024

sbera87 Oct 10, 2024

sbiscigl Oct 10, 2024

sbera87 Oct 10, 2024

sbiscigl Oct 10, 2024

sbera87 left a comment

move checksumming operations to a interceptor #3140

move checksumming operations to a interceptor #3140

Conversation

sbiscigl commented Oct 9, 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

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

sbera87 left a comment

Choose a reason for hiding this comment