feat: support Sigv4 for non AWS services #2385

gosar · 2021-05-11T23:58:02Z

Description

This has changes to middleware-signing and codegen to support generating clients for non AWS services that have SigV4 auth.

Testing

I generated client for the bootleg demo service we are working on and with SigV4 enabled on the service, confirmed the client is able to successfully call it. See https://github.com/adamthom-amzn/smithy-typescript-ssdk-demo/pull/20 for testing details.

I ran yarn generate-clients. There is only a change is comment for all generated clients, but it is actually going back to what it was few weeks back before my earlier changes for supporting non AWS services changed the comment slightly.

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

JordonPhillips

Why is this bifurcation needed? It looks to me like all you're doing is avoiding the region provider, but should you?

EDIT: I may be confusing two things. A region provider can be a thing that pulls the region from, say, environment variables or EC2 metadata. Or it could be the thing that uses the region to resolve an endpoint. The latter we definitely want to cut out, but the former should be kept if possible. If they're inextricably linked then I guess we have no choice, but it'd be good to leave a todo so we can explore splitting them up in the future.

JordonPhillips · 2021-05-12T14:21:43Z

...codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AddAwsRuntimeConfig.java

+            if (isAwsService(settings, model)) {
+                writer.writeDocs("The AWS region to which this client will send requests")
+                        .write("region?: string | __Provider<string>;\n");
+            } else {
+                writer.writeDocs("The AWS region to use as signing region for AWS Auth")
+                        .write("region?: string | __Provider<string>;\n");
+            }


Writing the actual region can be outside of this block since it doesn't change.

EDIT: actually it seems like the rest of the changes in this PR serve to effectively make region required, so I guess you'd want to enforce that here

I had the if to write a specific comment in each case.

It is not required in the sense that you don't need to provide it when instantiating the client. It is link for AWS clients where it comes in to ClientDefaultValues from here

Yes, but you can do that and still write the region separately. The methods in TypeScriptWriter return the object so you can chain calls like this, but it's not necessary.

packages/middleware-signing/src/configurations.ts

...t-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/EndpointGenerator.java

gosar · 2021-05-12T20:21:54Z

Why is this bifurcation needed? It looks to me like all you're doing is avoiding the region provider, but should you?

EDIT: I may be confusing two things. A region provider can be a thing that pulls the region from, say, environment variables or EC2 metadata. Or it could be the thing that uses the region to resolve an endpoint. The latter we definitely want to cut out, but the former should be kept if possible. If they're inextricably linked then I guess we have no choice, but it'd be good to leave a todo so we can explore splitting them up in the future.

I guess you are asking about bifurcation of AwsAuth v/s CustomAwsAuth. Yeah it's the latter. The RegionInfoProvider determines the endpoint and signingService, signingRegion.

gosar · 2021-05-13T06:57:03Z

Based on discussion with @AllanZhengYP,

Any alternate naming suggestions for CustomAwsAuth?

I'm going to call this SigV4Auth (for non-AWS services) and AwsAuth will represent SigV4 auth for AWS services.

There was existing reference to input.signingName in middleware-signing but I couldn’t see how one provides that input configuration. Is it safe to add signingName in config interface for every AWS service?

It seems safe to add signingName for every AWS service. I'll mark it @internal. The existing logic was probably based on assumption that the AWS service model may not have SigV4 trait, or it didn't exist back then. Given that now SigV4 trait is going to be present for every AWS service, we could just rely on that value instead of value from RegionInfoProvider(RIP). But that cleanup for AWS services, can be handled separately. I'm only going to change the logic for non-AWS service in this PR.

There is no regionInfo to determine signingService. The signingName will instead directly come from input. And region will be used as signingRegion.

Write `signingName` from the SigV4 trait for non AWS service. `region` is used for the signingRegion. A separate SigV4Auth is used for SigV4 logic for non AWS services.

codecov-commenter · 2021-05-13T18:00:17Z

Codecov Report

❗ No coverage uploaded for pull request base (main@597e689). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2385   +/-   ##
=======================================
  Coverage        ?   59.00%           
=======================================
  Files           ?      484           
  Lines           ?    26078           
  Branches        ?     6192           
=======================================
  Hits            ?    15387           
  Misses          ?    10691           
  Partials        ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 597e689...36fcace. Read the comment docs.

The models for the protocol tests don't seem to have the SigV4 trait, so can't rely solely on it for using the Region plugin. e.g, https://github.com/awslabs/smithy/blob/main/smithy-aws-protocol-tests/model/restJson1/main.smithy#L11-L13

…vices

gosar · 2021-05-13T19:09:11Z

clients/client-accessanalyzer/AccessAnalyzerClient.ts

@@ -226,7 +226,7 @@ export interface ClientDefaults extends Partial<__SmithyResolvedConfiguration<__
  serviceId?: string;

  /**
-   * The AWS region to which this client will send requests or use as signingRegion
+   * The AWS region to which this client will send requests


These are basically going back what the comment was before some of my earlier changes. See

aws-sdk-js-v3/clients/client-accessanalyzer/AccessAnalyzerClient.ts

Line 229 in 9ab71e8

* The AWS region to which this client will send requests

JordonPhillips

Still have that one splitup comment, but all good after that.

AllanZhengYP

It looks good to me! I only found some files changed unexpectly.

AllanZhengYP · 2021-06-02T21:51:31Z

protocol_tests/aws-restjson/tests/functional/restjson1.spec.ts

      "x-long": "123",
+      "x-byte": "1",


I don't think this change is needed.

AllanZhengYP · 2021-06-02T21:51:56Z

protocol_tests/aws-restjson/tests/functional/restjson1.spec.ts

      "x-memberepochseconds": "1576540098",
+      "x-defaultformat": "Mon, 16 Dec 2019 23:48:18 GMT",


This change is not needed.

AllanZhengYP · 2021-06-02T21:51:58Z

protocol_tests/aws-restxml/tests/functional/restxml.spec.ts

        "x-long": "123",
+        "x-byte": "1",


This change is not needed.

AllanZhengYP · 2021-06-02T21:52:03Z

protocol_tests/aws-restxml/tests/functional/restxml.spec.ts

        "x-memberepochseconds": "1576540098",
+        "x-defaultformat": "Mon, 16 Dec 2019 23:48:18 GMT",


This change is not needed.

github-actions · 2021-06-18T00:01:24Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

gosar requested a review from AllanZhengYP May 11, 2021 23:58

JordonPhillips requested review from JordonPhillips and adamthom-amzn May 12, 2021 14:14

JordonPhillips suggested changes May 12, 2021

View reviewed changes

gosar requested a review from JordonPhillips May 12, 2021 20:45

gosar added 2 commits May 13, 2021 10:57

feat(middleware-signing): support SigV4 for non AWS services

c6dc029

There is no regionInfo to determine signingService. The signingName will instead directly come from input. And region will be used as signingRegion.

feat(codegen): support SigV4 for non AWS services

3b3ae92

Write `signingName` from the SigV4 trait for non AWS service. `region` is used for the signingRegion. A separate SigV4Auth is used for SigV4 logic for non AWS services.

gosar force-pushed the sigv4 branch from a7592c3 to 3b3ae92 Compare May 13, 2021 18:00

gosar added 2 commits May 13, 2021 11:54

chore(clients): update clients with updates for SigV4 for non AWS ser…

bd25fda

…vices

gosar commented May 13, 2021

View reviewed changes

test(middleware-signing): add test for non AWS service SigV4

1b9be32

JordonPhillips approved these changes May 25, 2021

View reviewed changes

chore(codegen): reduce duplication for writing region

494c044

gosar requested a review from JordonPhillips May 25, 2021 21:22

JordonPhillips approved these changes Jun 1, 2021

View reviewed changes

AllanZhengYP approved these changes Jun 2, 2021

View reviewed changes

Reverting unnecessary changes to protocol tests

36fcace

gosar requested a review from AllanZhengYP June 3, 2021 21:40

AllanZhengYP approved these changes Jun 3, 2021

View reviewed changes

gosar merged commit 0a251aa into aws:main Jun 3, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support Sigv4 for non AWS services #2385

feat: support Sigv4 for non AWS services #2385

gosar commented May 11, 2021 •

edited

Loading

JordonPhillips left a comment •

edited

Loading

JordonPhillips May 12, 2021

gosar May 12, 2021

JordonPhillips May 25, 2021

gosar commented May 12, 2021

gosar commented May 13, 2021

codecov-commenter commented May 13, 2021 •

edited

Loading

gosar May 13, 2021

JordonPhillips left a comment

AllanZhengYP left a comment

AllanZhengYP Jun 2, 2021

AllanZhengYP Jun 2, 2021

AllanZhengYP Jun 2, 2021

AllanZhengYP Jun 2, 2021

github-actions bot commented Jun 18, 2021

		"x-memberepochseconds": "1576540098",
		"x-defaultformat": "Mon, 16 Dec 2019 23:48:18 GMT",

feat: support Sigv4 for non AWS services #2385

feat: support Sigv4 for non AWS services #2385

Conversation

gosar commented May 11, 2021 • edited Loading

Description

Testing

JordonPhillips left a comment • 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

gosar commented May 12, 2021

gosar commented May 13, 2021

codecov-commenter commented May 13, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

JordonPhillips left a comment

Choose a reason for hiding this comment

AllanZhengYP 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

Choose a reason for hiding this comment

github-actions bot commented Jun 18, 2021

gosar commented May 11, 2021 •

edited

Loading

JordonPhillips left a comment •

edited

Loading

codecov-commenter commented May 13, 2021 •

edited

Loading