Prerequisites to Enable smithy client #3317

sbera87 · 2025-02-27T16:42:04Z

Issue #, if available:

Description of changes:
This set of changes are needed to enable Smithy client for S3 with applicable backwards compatible changes.

Relevant s3 smithy code generation from PR: Smithy Identity and Auth Refactor (S3) - Rollback Fixed #3298
Added legacy APIs in smithy client for backwards compatibility
Modify Generate Presigned URL to be smithy compliant workflow

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.

sbera87 · 2025-02-27T19:54:03Z

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

@@ -290,18 +460,21 @@ void AwsSmithyClientBase::AttemptOneRequestAsync(std::shared_ptr<AwsSmithyClient
        return;
    }

-    pRequestCtx->m_interceptorContext->SetTransmitRequest(pRequestCtx->m_httpRequest);
-    for (const auto& interceptor : m_interceptors)
+    if (pRequestCtx->m_interceptorContext)


just added code block under null guard since in requestless cases, this is currently not created in requestless call flow.
This is done to emulate legacy behavior:
with request case:

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

Line 549 in 44dacf9

InterceptorContext context{request};

vs

request less case:

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

Line 616 in 44dacf9

HttpResponseOutcome AWSClient::AttemptOneRequest(const std::shared_ptr<Aws::Http::HttpRequest>& httpRequest,

sbera87 · 2025-02-27T19:55:27Z

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

+
+
+
+void AwsSmithyClientBase::MakeRequestAsyncHelper(std::shared_ptr<AwsSmithyClientAsyncRequestContext>& pRequestCtx,


This helper abstracts endpoint resolution into a callback and is invoked contextually by different variations of APIs used for backwards compatibilty

sbera87 · 2025-02-28T17:05:19Z

generated/src/aws-cpp-sdk-s3/source/S3Client.cpp

    Aws::Map<Aws::String, Aws::String> params;
    params.emplace("bucketName", bucket);
    ServiceSpecificParameters serviceSpecificParameters{params};
    auto serviceParams = Aws::MakeShared<ServiceSpecificParameters>(ALLOCATION_TAG, serviceSpecificParameters);
-    return AWSClient::GeneratePresignedUrl(endpoint, method, customizedHeaders, expirationInSeconds, Aws::Auth::SIGV4_SIGNER,
-        nullptr, nullptr, serviceParams);
+    EndpointUpdateCallback endpointCallback = [&](Aws::Endpoint::AWSEndpoint& endpoint){


Changes to resolve endpoint after select auth scheme

sbera87 · 2025-02-28T17:08:43Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

-            Aws::Vector<AuthSchemeOption> authSchemeOptions = m_authSchemeResolver->resolveAuthScheme(identityParams);
+            Aws::Vector<AuthSchemeOption> authSchemeOptions;
+            /*for backwards compatibility, signer name override is used to filter auth schemes by id equivalent to legacy signer name*/
+            if(ctx.m_signerNameOverride.has_value())


This is added to make the backwards compatible api functional.
In legacy client, signer name passed was used to fetch the signer object via
auto signer = GetSignerByName(signerName);
In smithy design, this name passed is simply re-mapped and used to filter desired auth scheme (with requested signer) from list of supported auth schemes

This is added to make the backwards compatible api functional.

This is really confusing me, you say we added this for backwards compatibility, but then we don't declare GetSignerByName so we break backwards incompatibility with that

i.e. this compiles before but is broken after. So why are we doing something for backwards compatibility, then we are actually breaking backwards compatibility?

#include <aws/core/Aws.h> #include <aws/s3/S3Client.h> using namespace Aws; using namespace Aws::S3; class MyS3Client : public Aws::S3::S3Client { public: MyS3Client() = default; ~MyS3Client() override = default; void Foo() { std::cout << this->GetSignerByName("some_name") << std::endl; } }; auto main() -> int { SDKOptions options{}; options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Debug; InitAPI(options); { MyS3Client client{}; client.Foo(); } ShutdownAPI(options); return 0; }

sbera87 · 2025-02-28T17:09:51Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

@@ -240,8 +314,141 @@ namespace client
            }
            return GeneratePresignedUrl(uri, method, signerRegionOverride, signerServiceNameOverride, expirationInSeconds, customizedHeaders, serviceSpecificParameters);
        }
+
+        ResponseT MakeRequest(const Aws::Http::URI& uri,


Legacy APIs that need to be supported as external clients found to use some variations.

can we move this to a different class? its makes it hard to understand what is going on here if we add them in the smithy client, move it to a new class then have the smithy client just reference it. we do not want to copy and paste the old work flow into the new one.

Actually, The interface is old which we have to maintain for backwards compatibility, but internally, it follows the same design as smithy MakeRequestDeserialize API, i.e use executor to submit an Async API implementation with resolve endpoint action as a callback and then deserialize the outcome .

In this example the call flow is: MakeRequest -> MakeRequestWithUriAsync -> MakeRequestAsyncHelper

sbera87 · 2025-02-28T17:10:42Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h

-        using ClientError = AWSError;
-        using SigningError = AWSError;
+        using AWSCoreError = Aws::Client::AWSError<CoreErrors>;
+        using ClientError = AWSCoreError;


Needed to ensure backwards compatibility . External clients use AwsError as a templated type

sbera87 · 2025-02-28T17:11:07Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h

+                std::shared_ptr<Aws::Utils::Threading::Executor> pExecutor
+            ) const;
+
+        const std::shared_ptr<Aws::Client::AWSErrorMarshaller>& GetErrorMarshaller() const


Needed as External Users use this

sbera87 · 2025-02-28T17:11:57Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientBase.h

@@ -200,7 +237,7 @@ namespace client
        Aws::Vector<std::shared_ptr<smithy::interceptor::Interceptor>> m_interceptors{};
        std::shared_ptr<smithy::client::UserAgentInterceptor> m_userAgentInterceptor;
    private:
-        void UpdateAuthSchemeFromEndpoint(const Aws::Endpoint::AWSEndpoint& endpoint, AuthSchemeOption& authscheme) const;
+        void UpdateAuthSchemeFromEndpoint(std::shared_ptr<AwsSmithyClientAsyncRequestContext>& pRequestCtx) const;


Interface updated to facilitate legacy API usage where endpoint may not be available

sbiscigl

I think we've got a maintainibilty issue here. You've more or less copy and pasted AWSClient into the SmithyClient, which I don't think is the way forward here. If we want to maintain backward compatible interfaces for newer clients we should maintain the interface, but not copy and paste the code directly.

sbiscigl · 2025-02-28T20:10:16Z

generated/src/aws-cpp-sdk-s3/include/aws/s3/S3ClientConfiguration.h

@@ -62,6 +62,10 @@ namespace Aws
            IdentityProviderSupplier identityProviderSupplier = [](const S3Client &client) -> std::shared_ptr<S3ExpressIdentityProvider> {
                return Aws::MakeShared<DefaultS3ExpressIdentityProvider>("S3ClientConfiguration", client);
            };
+            using SmithyIdentityProviderSupplier = std::function<std::shared_ptr<SmithyS3ExpressIdentityProvider> (const S3Client &)>;


so SmithyS3ExpressIdentityProvider and S3ExpressIdentityProvider are more or less the same thing, and we really should only have one, could we make this a CRT Variant so that either could be provided and we internally can choose what do differently based on which type is present? having two of the same configuration seems misleading.

Though the functionality is achieved the same, the interfaces are different and hence added to support both old and new
class AWS_S3_API S3ExpressIdentityProvider : public smithy::IdentityResolverBase<S3ExpressIdentity>
vs
class AWS_S3_API SmithyS3ExpressIdentityProvider : public smithy::IdentityResolverBase<smithy::AwsCredentialIdentityBase>

I have made changes in next commit to address this so that there is only one identity provider supplier as before.

sbiscigl · 2025-02-28T20:12:53Z

generated/src/aws-cpp-sdk-s3/include/aws/s3/S3ExpressAuthSchemeResolver.h

+                }
+            }
+
+            return {smithy::SigV4AuthSchemeOption::sigV4AuthSchemeOption};


why are we returning sigv4 instead of a empty list?

This is to address existing test cases like
TEST_F(BucketAndObjectOperationTest, TestS3AccessPointARN)

S3Client s3Client(config); GetObjectRequest getObjectRequest; getObjectRequest.SetBucket(bucketArn); getObjectRequest.SetKey("fakeObjectKey"); auto getObjectOutcome = s3Client.GetObject(getObjectRequest); ASSERT_FALSE(getObjectOutcome.IsSuccess());

when auth scheme is not found we currently have an assertion for builds which don't strip away asserts.
assert(authSchemeOptionIt != authSchemeOptions.end());
The fallback is added to not hit this assertion. without this it exits with a different core error and not the expected CoreErrors::ENDPOINT_RESOLUTION_FAILURE in the legacy test case.
This core error is only returned when resolve endpoint fails which in smithy workflow happens after select auth schemes.

that doesnt make sense, this should return a empty list. doing something because "it makes a test pass" is not a good reason for a implementation detail

get object shuold be using a auth scheme that is recognized, why isnt it.

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

sbiscigl · 2025-03-03T15:41:34Z

generated/src/aws-cpp-sdk-s3/include/aws/s3/S3ExpressAuthSchemeResolver.h

+                        auto authSchemeName = endpoint.GetAttributes()->authScheme.GetName();
+                        if(strcmp(authSchemeName.c_str(),Aws::Auth::ASYMMETRIC_SIGV4_SIGNER) == 0)
+                        {
+                        return {smithy::SigV4aAuthSchemeOption::sigV4aAuthSchemeOption};


per the SRA

The result is a list of authentication schemes, in order of preference, that should be used for the request. The SDK will choose the first scheme that it supports, or fail the request if no schemes are supported.

so why are we returning a single auth scheme option? we should be returning the list of auth scheme options that are supported, then we choose the first one after we call.

So, its a vector of one for the appropriate scheme id. If you see smithy v4, v4a authscheme resolvers they return respective vector of one authscheme options. Since this is a child of multi auth scheme resolver it simply uses if else condition block for the different scheme ids.

So, its a vector of one for the appropriate scheme id.

no you should be returning a vector of supported auth schemes, and using the first supported one. i.e. if you are using v4a it should return [v4a, v4] then the caller gets to decide which one is supported, thats the way the interface is supposed to wok

sbiscigl · 2025-03-03T15:45:21Z

generated/src/aws-cpp-sdk-s3/source/S3Client.cpp

-          return Aws::MakeShared<ServiceSpecificParameters>(ALLOCATION_TAG, serviceSpecificParameters);
-        }());
-      return DeleteBucketOutcome(MakeRequest(request, endpointResolutionOutcome.GetResult(), Aws::Http::HttpMethod::HTTP_DELETE));
+       [&]() -> std::shared_ptr<Http::ServiceSpecificParameters> {


your code change is changing formatting on unchanged lines -- please fix

I double checked. This honors the indentation used in the new smithy code gen template. Changing this will impact other smithy clients too.

well if it was a problem before we should fix it, it doesnt honor the correct indentation, please fix it

As discussed, this is just the change smithy codegen brings with own consistent indentation that it is consistent with
.

sbiscigl · 2025-03-03T15:49:05Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

@@ -190,13 +210,68 @@ namespace client
            return m_serializer->Deserialize(std::move(httpResponseOutcome), GetServiceClientName(), requestName);
        }

+        Aws::String GeneratePresignedUrl(
+            const Aws::String& bucket,


why is the string "bucket" here? the client is not aware of what a bucket is, there are several other presigned operations that exist and do not fit into this paradigm.

The above example is already handled in another PR (where other rest xml clients use the API you mentioned.) This PR deals with S3 related changes only

I have made changes to this to eliminate bucket from the interface

sbiscigl · 2025-03-03T16:08:01Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

-            Aws::Vector<AuthSchemeOption> authSchemeOptions = m_authSchemeResolver->resolveAuthScheme(identityParams);
+            Aws::Vector<AuthSchemeOption> authSchemeOptions;
+            /*for backwards compatibility, signer name override is used to filter auth schemes by id equivalent to legacy signer name*/
+            if(ctx.m_signerNameOverride.has_value())


This is added to make the backwards compatible api functional.

This is really confusing me, you say we added this for backwards compatibility, but then we don't declare GetSignerByName so we break backwards incompatibility with that

i.e. this compiles before but is broken after. So why are we doing something for backwards compatibility, then we are actually breaking backwards compatibility?

#include <aws/core/Aws.h> #include <aws/s3/S3Client.h> using namespace Aws; using namespace Aws::S3; class MyS3Client : public Aws::S3::S3Client { public: MyS3Client() = default; ~MyS3Client() override = default; void Foo() { std::cout << this->GetSignerByName("some_name") << std::endl; } }; auto main() -> int { SDKOptions options{}; options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Debug; InitAPI(options); { MyS3Client client{}; client.Foo(); } ShutdownAPI(options); return 0; }

sbiscigl · 2025-03-03T16:08:34Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClientAsyncRequestContext.h

@@ -70,6 +70,10 @@ namespace smithy
            ResponseHandlerFunc m_responseHandler;
            std::shared_ptr<Aws::Utils::Threading::Executor> m_pExecutor;
            std::shared_ptr<interceptor::InterceptorContext> m_interceptorContext;
+            //For Backwards Compatible APIs


what backwards compatible APIs?

sbiscigl · 2025-03-03T16:13:13Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

@@ -240,8 +314,141 @@ namespace client
            }
            return GeneratePresignedUrl(uri, method, signerRegionOverride, signerServiceNameOverride, expirationInSeconds, customizedHeaders, serviceSpecificParameters);
        }
+
+        ResponseT MakeRequest(const Aws::Http::URI& uri,


can we move this to a different class? its makes it hard to understand what is going on here if we add them in the smithy client, move it to a new class then have the smithy client just reference it. we do not want to copy and paste the old work flow into the new one.

sbiscigl · 2025-03-03T16:18:01Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

+            const char* signerRegionOverride = nullptr,
+            const char* signerServiceNameOverride = nullptr) const
+        {
+            std::shared_ptr<Aws::Utils::Threading::Executor> pExecutor = Aws::MakeShared<Aws::Utils::Threading::SameThreadExecutor>(ServiceNameT);


you've more or less copy and pasted the old code into the new class, this is not what we want. this diverges the flow significantly. these functions should_call MakeRequestDeserialize and should not diverge the call path. This is just creating both clients in a a single class. we should defer calling to the new methods and emit a warning log about calling these methods directly.

Its actually new code in old interface. But, I have changed all the legacy APIs to call MakeRequestDeserialize and moved them to a different class using CRTP pattern in smithy client

sbiscigl · 2025-03-05T16:13:37Z

generated/src/aws-cpp-sdk-s3/include/aws/s3/S3ExpressAuthSchemeResolver.h

+                        auto authSchemeName = endpoint.GetAttributes()->authScheme.GetName();
+                        if(strcmp(authSchemeName.c_str(),Aws::Auth::ASYMMETRIC_SIGV4_SIGNER) == 0)
+                        {
+                        return {smithy::SigV4aAuthSchemeOption::sigV4aAuthSchemeOption};


So, its a vector of one for the appropriate scheme id.

no you should be returning a vector of supported auth schemes, and using the first supported one. i.e. if you are using v4a it should return [v4a, v4] then the caller gets to decide which one is supported, thats the way the interface is supposed to wok

sbiscigl · 2025-03-05T16:15:47Z

generated/src/aws-cpp-sdk-s3/include/aws/s3/S3ExpressAuthSchemeResolver.h

+                }
+            }
+
+            return {smithy::SigV4AuthSchemeOption::sigV4AuthSchemeOption};


that doesnt make sense, this should return a empty list. doing something because "it makes a test pass" is not a good reason for a implementation detail

get object shuold be using a auth scheme that is recognized, why isnt it.

sbiscigl · 2025-03-05T16:18:39Z

generated/src/aws-cpp-sdk-s3/source/S3Client.cpp

-          return Aws::MakeShared<ServiceSpecificParameters>(ALLOCATION_TAG, serviceSpecificParameters);
-        }());
-      return DeleteBucketOutcome(MakeRequest(request, endpointResolutionOutcome.GetResult(), Aws::Http::HttpMethod::HTTP_DELETE));
+       [&]() -> std::shared_ptr<Http::ServiceSpecificParameters> {


well if it was a problem before we should fix it, it doesnt honor the correct indentation, please fix it

sbiscigl · 2025-03-05T16:19:38Z

generated/src/aws-cpp-sdk-s3/source/S3ExpressSignerProvider.cpp

@@ -21,12 +21,12 @@ Aws::Auth::S3ExpressSignerProvider::S3ExpressSignerProvider(
        region,
        signingPolicy,
        urlEscapePath) {
-    m_signers.emplace_back(Aws::MakeShared<Aws::S3::S3ExpressSigner>(CLASS_TAG,
+    m_signers.emplace_back(std::static_pointer_cast<Aws::Client::AWSAuthSigner>(Aws::MakeShared<Aws::S3::S3ExpressSigner>(CLASS_TAG,


remove the static pointer class it should not be needed

For the return auth schemes list comment, we can't return all the auth schemes indiscriminately in a list for multi auth case. For the multi auth support, we do a resolve endpoint twice, the first time happens in select auth scheme.

Based on the auth scheme from endpoint, we build the list as aforementioned. What you are referring to as pick the first supported auth scheme actually happens here inside the resolver. This was done based on previous PR comments to move this auth scheme vector selection inside the auth scheme resolver class from smithy client class (else it would end up doing double endpoint resolution for all clients and not just S3).

So, for the negative test case I mentioned, epResolutionOutcome is not a success and hence we have empty auth schemes list which would trigger the assertion

Removed the cast in next commit

sbiscigl · 2025-03-05T16:20:33Z

src/aws-cpp-sdk-core/include/smithy/client/AwsLegacyClient.h

+namespace client
+{   
+    /*
+        This Class exists only to provide a way to access the legacy client APIs


niiiiiiiiiiiice, i like this a lot

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

sbera87 force-pushed the s3smithyon branch 2 times, most recently from dea11b7 to f0684a4 Compare February 27, 2025 19:50

sbera87 commented Feb 27, 2025

View reviewed changes

sbera87 force-pushed the s3smithyon branch 3 times, most recently from 3547603 to f03166d Compare February 28, 2025 16:58

sbera87 commented Feb 28, 2025

View reviewed changes

sbera87 force-pushed the s3smithyon branch from f03166d to 966ca62 Compare February 28, 2025 17:17

sbera87 requested review from sbiscigl and SergeyRyabinin February 28, 2025 18:49

sbiscigl reviewed Mar 3, 2025

View reviewed changes

sbera87 force-pushed the s3smithyon branch 2 times, most recently from dfa0208 to 16c4b73 Compare March 4, 2025 14:48

sbera87 requested a review from sbiscigl March 4, 2025 14:53

sbera87 force-pushed the s3smithyon branch from 7cbf351 to 309698a Compare March 4, 2025 15:38

sbiscigl reviewed Mar 5, 2025

View reviewed changes

sbera87 force-pushed the s3smithyon branch 4 times, most recently from 5e42f3e to c6089cf Compare March 5, 2025 20:26

sbera87 requested a review from sbiscigl March 5, 2025 20:28

sbera87 force-pushed the s3smithyon branch 3 times, most recently from 2e8a268 to b5c604b Compare March 6, 2025 16:10

sbera87 force-pushed the s3smithyon branch 2 times, most recently from 8de0047 to 3686975 Compare March 6, 2025 19:51

sbera87 changed the title ~~Enable S3 smithy client~~ Prerequisites to Enable smithy client Mar 6, 2025

sbiscigl approved these changes Mar 7, 2025

View reviewed changes

Pre-requisites for Enabling Smithy Clients

defa0b4

sbera87 force-pushed the s3smithyon branch from 3686975 to defa0b4 Compare March 7, 2025 17:09

SergeyRyabinin approved these changes Mar 7, 2025

View reviewed changes

sbera87 merged commit ea08eb7 into main Mar 7, 2025
3 of 4 checks passed




		void AwsSmithyClientBase::MakeRequestAsyncHelper(std::shared_ptr<AwsSmithyClientAsyncRequestContext>& pRequestCtx,

Prerequisites to Enable smithy client #3317

Prerequisites to Enable smithy client #3317

Conversation

sbera87 commented Feb 27, 2025 • edited Loading

sbera87 Feb 27, 2025 • 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

sbera87 Feb 28, 2025 • 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

sbiscigl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbera87 Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbera87 Mar 4, 2025 • 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

Choose a reason for hiding this comment

sbera87 Mar 4, 2025 • 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

sbera87 Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbera87 commented Feb 27, 2025 •

edited

Loading

sbera87 Feb 27, 2025 •

edited

Loading

sbera87 Feb 28, 2025 •

edited

Loading

sbera87 Mar 3, 2025 •

edited

Loading

sbera87 Mar 4, 2025 •

edited

Loading

sbera87 Mar 4, 2025 •

edited

Loading

sbera87 Mar 5, 2025 •

edited

Loading