Smithy refactor Json protocol with Single direction EventStreaming (v4/v4a/bearer) #3212

sbera87 · 2024-12-09T02:17:37Z

Issue #, if available:

Description of changes:

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 · 2024-12-09T02:55:07Z

...p-sdk-transcribestreaming/include/aws/transcribestreaming/TranscribeStreamingServiceClient.h

+#include <smithy/client/AwsSmithyClient.h>
+#include <smithy/identity/auth/built-in/SigV4AuthSchemeResolver.h>
+#include <smithy/identity/auth/built-in/SigV4AuthScheme.h>
+#include <smithy/client/serializer/JsonOutcomeSerializer.h>


sbera87 · 2024-12-09T14:48:04Z

generated/src/aws-cpp-sdk-logs/include/aws/logs/CloudWatchLogsClient.h

 #include <aws/logs/CloudWatchLogsServiceClientModel.h>
+#include <smithy/client/AwsSmithyClient.h>


sbiscigl · 2024-12-12T20:29:54Z

tools/scripts/run_code_generation.py

@@ -330,8 +352,7 @@ def generate_single_client(service_name: str,
        run_command += ["--endpoint-tests", f"{endpoints_filepath}/{model_files.endpoint_tests}"]
    run_command += ["--service", service_name]
    run_command += ["--outputfile", output_filename]
-
-    if service_name in SMITHY_SUPPORTED_CLIENTS:
+    if use_smithy:


why did we move this to a boolean parameter, the boolean parameter seems just to be the evaluation of the statement before? seems that we dont gain much by adding the parameter.

So the backstory is that is global and when I pass a global to collect_available_models on main thread (append to it) and then on another thread use it the changes were not visible.
because Python's Global Interpreter Lock (GIL) ensures thread safety for certain operations, but it doesn't guarantee the immediate visibility of changes to shared mutable objects (like set) across threads.

hence instead of passing the entire set, all i actually need is the indicator service within set or not. which is a boolean

sbiscigl · 2024-12-12T20:33:52Z

tools/scripts/run_code_generation.py

-]
+}
+
+SMITHY_EXCLUSION_CLIENTS = {


instead of generating a deny list, is there anyway we could get this from the model itself?

so, this deny list is basically an interim way to filter services further under a protocol. From the model, there is no simple way to say detect multi auth or streaming single/bi .
We will need to parse for every model every operation for the former and operation and its request response for the latter. Hence, to simplify this, I ran a different script outside the purview of this and added the results here.

sbiscigl · 2024-12-12T20:35:28Z

tools/scripts/run_code_generation.py

+                if ("protocol" in  model["metadata"] and 
+                    (model["metadata"]["protocol"] == "json" or model["metadata"]["protocol"] == "rest-json")):
+                    if key not in SMITHY_EXCLUSION_CLIENTS:
+                        SMITHY_SUPPORTED_CLIENTS.add(key)


i think the intention here was that the SMITHY_SUPPORTED_CLIENTS global variable was meant to be a hardcoded set of clients to use for smithy generation not for something to be updated

we should move this allow/deny list logic to a function instead of updating a global variable.

I can use a local variable to deepcopy from it before using it. Is that ok?

...r/src/main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/CppClientGenerator.java

sbiscigl · 2024-12-12T20:41:11Z

...r/src/main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/CppClientGenerator.java

@@ -688,7 +698,16 @@ protected List<SdkFileEntry> generateSmithyClientSourceFile(final List<ServiceMo

            VelocityContext context = createContext(serviceModels.get(i));
            context.put("CppViewHelper", CppViewHelper.class);
-            context.put("AuthSchemeResolver", "SigV4AuthSchemeResolver");
+            Optional<String> firstAuthScheme = serviceModels.get(i).getAuthSchemes().stream().filter(entry->ResolverMapping.containsKey(entry)).findFirst();
+            if(firstAuthScheme.isPresent())


seems like this is the same in both generate header and generate souice, seems like we could extract it to a private method.

sbiscigl · 2024-12-12T21:01:42Z

...c/main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/JsonCppClientGenerator.java

        for (int i = 0; i < serviceModels.size(); i++) {
+            if(serviceModels.get(i).isUseSmithyClient())
+            {
+                continue;


i think we could take this moment to refactor some of the legacy cruft we have here. instead of making this two loops we could make a single loop with a map statement and rework how generateSmithyClientSourceFile is called. instead of doing two iterations over the service models we could do something like

return serviceModels.stream() .map(model -> { if (model.isUseSmithyClient()) { return GenerateSmithyClientSourceFile(model) } else { return GenerateLegacyClientSourceFile(model); } }) .collect(Colleectors.toList());

where the logic in this loop is factored out to GenerateLegacyClient and GenerateSmithySourceFile is refactored to accept either a list or a single file in a overloaded function

Sure, let me make a commit to improve this

sbiscigl

Alright i think we need to step back to the design portion of this because something went wrong in design in relation to how we handle stream signing. We cannot move forward with this until we deal with it properly. We cannot add a different API for stream signing vs regular signing. the high level ask of this task is align closer with other SDKs on how they do identity and auth and this sharply deviates from them.

I think this boils down to the question

"How do we do stream signing with SRA identity and auth"

We can work on a path forward but as a team we will likely have to have a group discussion about how we want to refactor stream signing. Will talk to you offline about this. We should take a look at how java v2 handled this because they were able to handle this without deviating from the interface.

Looking at their code for how they handle auth for event streams its not immdiately apparent how they do it but lets set up some time to talk to them about they handle it.

sbiscigl · 2024-12-17T22:24:23Z

src/aws-cpp-sdk-core/include/smithy/identity/signer/AwsSignerBase.h

@@ -46,6 +46,8 @@ namespace smithy {

        // signer may copy the original httpRequest or create a new one
        virtual SigningFutureOutcome sign(std::shared_ptr<HttpRequest> httpRequest, const IdentityT& identity, SigningProperties properties) = 0;
+        // for requests with event stream
+        virtual bool SignEventMessage(Aws::Utils::Event::Message&, Aws::String&, const IdentityT&) const { return false; };


We really cant add this to the interface as defined in the SRA a singer should only be the following interface

interface Signer <IdentityT extends Identity > { Result<HTTPRequest, HTTPSignerError> sign(HTTPRequest: HTTPRequest, identity: IdentityT , signingProperties: PropertyBag); }

why we're doing all of this is to conform the SRA so that we are less different than other SDKs when it comes to identity and auth flow.

sbiscigl · 2024-12-18T14:39:47Z

...pp-sdk-core/include/smithy/identity/resolver/built-in/DefaultAwsCredentialIdentityResolver.h

 #include <aws/core/auth/AWSCredentials.h>
 #include <aws/core/auth/AWSCredentialsProviderChain.h>
-
+#include <smithy/identity/auth/AuthSchemeResolverBase.h>


no actual code changed in this file but includes did. This is more of a comment on the whole PR, but we really need to cut down on lines changed so that the PR is easier to review. Theres ideas in here that would be much easier to review if it were clearer on what actually changed and what didnt.

could you please take some time to clean up this PR and remove files and lines that are changed when nothing changed? it goes a long way to make PRs reviewable.

sbiscigl · 2024-12-18T14:44:02Z

src/aws-cpp-sdk-core/include/smithy/identity/auth/AuthScheme.h

@@ -26,6 +26,7 @@ namespace smithy {

        virtual std::shared_ptr<IdentityResolverBase<IdentityT>> identityResolver() = 0;

-        virtual std::shared_ptr<AwsSignerBase<IdentityT>> signer() = 0;
+        // The same auth scheme can have a different signer based on operation supports request with event stream
+        virtual std::shared_ptr<AwsSignerBase<IdentityT>> signer(bool isEventStreaming = false) = 0;


yeah this also doesnt conform with the SRA

interface AuthScheme <T extends Identity > { String schemeId(); Option <IdentityResolver <T>> identityResolver(cf: IdentityResolverConfiguration); Signer<T> signer(); } }

we really need to go back to the drawing board if what the SRA recommends doesnt work for us with stream signing, and figure out the discconect.

…onflixt with some services

sbiscigl

some functionality comments, but this PR is really hard to review, you need to focus on cleaning it up, there are hundreds of non code-genned files changed without any content changing, that needs to be cleaned up so that we can review easier, its really hard to tell what is content change and formatting changes at this point

sbiscigl · 2025-01-14T16:01:10Z

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

@@ -164,7 +167,7 @@ namespace client
                                 false/*retryable*/);
        }

-        SigningOutcome SignRequest(std::shared_ptr<HttpRequest> httpRequest, const AuthSchemeOption& targetAuthSchemeOption) const override
+        SigningOutcome SignHttpRequest(std::shared_ptr<HttpRequest> httpRequest, const AuthSchemeOption& targetAuthSchemeOption) const override


why are we renaming this method? doesnt make sense to update things unless we have a reason

there is a good reason behind this which was brought up. Some clients use SignRequest as class name which when used to inherit from smithy client template causes shadowing error. The name of the function has appropriate name now since it is actually signing http request and is the minimal change to address the problem. This is tested against 375 clients with no issue for now.

Example of the the error:
KMSClient.cpp:1173:35: error: must use 'class' tag to refer to type 'SignRequest' in this scope 1173 | SignOutcome KMSClient::Sign(const SignRequest& request) const

sbiscigl · 2025-01-14T16:01:53Z

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

-                                     const char* requestName,
-                                     Aws::Http::HttpMethod method,
-                                     EndpointUpdateCallback&& endpointCallback) const
+        ResponseT MakeRequestDeserialize(Aws::AmazonWebServiceRequest const* const request, const char* requestName,


why are we formatting the method signature here? lets wait until we format the entire project, here it just creates noise

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

sbiscigl · 2025-01-14T16:05:10Z

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

@@ -60,8 +67,8 @@ namespace Aws
    namespace Endpoint
    {
        class AWSEndpoint;
-    }
-}
+    }  // namespace Endpoint


formatting, why is this important to add it now? please reduce noise on lines changed that dont impact functionality

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

sbiscigl · 2025-01-14T16:20:06Z

src/aws-cpp-sdk-core/include/smithy/identity/signer/built-in/SigV4EventStreamSigner.h

@@ -0,0 +1,63 @@
+


do we actually use the event stream signer?, if not we really should remove it from this PR.

sbiscigl · 2025-01-14T16:25:01Z

src/aws-cpp-sdk-core/include/smithy/identity/signer/built-in/SigV4aSigner.h

+        }
+
+        virtual ~AwsSigV4aSigner() {};
+    protected:


why did we move createAwsSigningConfig? if it doesnt change we shoudlnt be moving it.

I am not sure why github shows it like this. I re-did the changes and just added the presigner and sign helper and it still shows as if its moved

sbiscigl · 2025-01-14T17:36:54Z

src/aws-cpp-sdk-core/include/aws/core/utils/event/EventEncoderStream.h

@@ -28,7 +28,6 @@ namespace Aws
            class AWS_CORE_API EventEncoderStream : public Aws::IOStream


we arent using this file in this PR, please remove it

sbiscigl · 2025-01-14T17:37:09Z

src/aws-cpp-sdk-core/include/aws/core/utils/event/EventStreamEncoder.h

@@ -40,7 +42,12 @@ namespace Aws
                 * The signing is done via the signer member.
                 */
                Aws::Vector<unsigned char> EncodeAndSign(const Aws::Utils::Event::Message& msg);
-            private:
+


not used in this PR please remove

sbiscigl · 2025-01-14T21:06:53Z

src/aws-cpp-sdk-core/include/smithy/identity/resolver/AwsBearerTokenIdentityResolver.h

+    AwsBearerTokenIdentityResolver(const Aws::Auth::BearerTokenAuthSignerProvider &bearerTokenProvider) {
+      auto signer = bearerTokenProvider.GetSigner(Aws::Auth::BEARER_SIGNER);
+      if (signer) {
+        m_providerChainLegacy.emplace_back(std::dynamic_pointer_cast<Aws::Client::AWSAuthBearerSigner>(signer)->BearerTokenProvider());


we cant call dynamic_pointer_cast because RTTI can be disabled in the SDK. likely will have to update BearerTokenAuthSignerProvider to have a concrete method to access the bearer token provider.

You are right. This is the case where to maintain backward compatibility we use the legacy signerprovider containing many signers. Since the enum Aws::Auth::BEARER_SIGNER is mapped to the type Aws::Client::AWSAuthBearerSigner, we can just use static cast here

sbera87 · 2025-01-15T04:32:47Z

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

@@ -56,7 +54,7 @@ namespace smithy

            Aws::String m_invocationId;
            Aws::Http::HttpMethod m_method;
-            const Aws::AmazonWebServiceRequest* m_pRequest; // optional
+            const Aws::AmazonWebServiceRequest* m_pRequest{nullptr}; // optional


without this initialization it crashes in integration tests in dry run

sbera87 commented Dec 9, 2024

View reviewed changes

sbera87 requested review from SergeyRyabinin and sbiscigl December 10, 2024 14:51

sbera87 changed the title ~~Smithy refactor Json protocol with EventStreaming (v4/v4a)~~ Smithy refactor Json protocol with EventStreaming (v4/v4a/bearer) Dec 10, 2024

sbera87 force-pushed the smithy_refactor branch from 7947c2c to f0b1bc2 Compare December 10, 2024 19:59

version 0: refactor with smithy client for json protocol non customized

3973d48

sbera87 force-pushed the smithy_refactor branch from f0b1bc2 to 3973d48 Compare December 10, 2024 20:05

cleanup

8867b8b

sbiscigl reviewed Dec 12, 2024

View reviewed changes

sbera87 added 5 commits December 13, 2024 00:07

refactor for smithy/legacy json clients

f333753

tweak

cbc6b8f

al2 compilaiton fix, pr comment address

a99e702

fix for category of services with stream members with examples

7e17cbe

add support for path segment of different types

7b08305

sbera87 requested a review from sbiscigl December 17, 2024 19:18

dry run fix for path segments greater than one case

aeb4287

sbiscigl reviewed Dec 18, 2024

View reviewed changes

sbera87 added 8 commits December 18, 2024 11:53

exclude customization services and rename method to avoid shadowing c…

a9768c8

…onflixt with some services

removing streaming

13393cb

remove event streaming

790a441

remove bidirectional event streaming changes

1d7c618

fix to accomodate more clients

dec045a

fix in codegen for loop variable

453f112

change tests for smithy client not copyable

004145e

namespace resolution of AWS Error

b796422

sbera87 changed the title ~~Smithy refactor Json protocol with EventStreaming (v4/v4a/bearer)~~ Smithy refactor Json protocol with Single direction EventStreaming (v4/v4a/bearer) Jan 5, 2025

sbera87 requested a review from sbiscigl January 6, 2025 18:31

sbera87 added 2 commits January 6, 2025 14:25

efs client changes

802556a

merge changes

22c5b10

sbera87 added 9 commits January 8, 2025 16:40

fix rfc paths for smithy clients

9c042fa

fix query string

601d6e3

changes for presigner

24104d4

presigner support

19b7945

merge changes from mainline

65e9c06

merge useragent changes

95b2966

merges from main

8e9e896

fix merge conflict

a84fdba

exclude bidirectional streaming client

3e3f242

sbiscigl reviewed Jan 14, 2025

View reviewed changes

address pr comments + cleanup

6c73fee

sbera87 requested a review from sbiscigl January 15, 2025 04:28

sbera87 commented Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Smithy refactor Json protocol with Single direction EventStreaming (v4/v4a/bearer) #3212

Smithy refactor Json protocol with Single direction EventStreaming (v4/v4a/bearer) #3212

sbera87 commented Dec 9, 2024 •

edited

Loading

sbera87 Dec 9, 2024

sbera87 Dec 9, 2024

sbiscigl Dec 12, 2024

sbera87 Dec 12, 2024

sbiscigl Dec 12, 2024

sbera87 Dec 12, 2024

sbiscigl Dec 12, 2024

sbera87 Dec 12, 2024

sbiscigl Dec 12, 2024

sbiscigl Dec 12, 2024

sbera87 Dec 12, 2024

sbiscigl left a comment

sbiscigl Dec 17, 2024

sbiscigl Dec 18, 2024

sbiscigl Dec 18, 2024

sbiscigl left a comment

sbiscigl Jan 14, 2025

sbera87 Jan 15, 2025 •

edited

Loading

sbiscigl Jan 14, 2025

sbiscigl Jan 14, 2025

sbiscigl Jan 14, 2025

sbiscigl Jan 14, 2025

sbera87 Jan 15, 2025

sbiscigl Jan 14, 2025

sbiscigl Jan 14, 2025

sbiscigl Jan 14, 2025

sbera87 Jan 15, 2025

sbera87 Jan 15, 2025

		#include <aws/logs/CloudWatchLogsServiceClientModel.h>
		#include <smithy/client/AwsSmithyClient.h>

		@@ -28,7 +28,6 @@ namespace Aws
		class AWS_CORE_API EventEncoderStream : public Aws::IOStream

Smithy refactor Json protocol with Single direction EventStreaming (v4/v4a/bearer) #3212

Are you sure you want to change the base?

Smithy refactor Json protocol with Single direction EventStreaming (v4/v4a/bearer) #3212

Conversation

sbera87 commented Dec 9, 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

sbiscigl left a 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 Jan 15, 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

sbera87 commented Dec 9, 2024 •

edited

Loading

sbera87 Jan 15, 2025 •

edited

Loading