Add accountID endpoint builtin and accountID endpoint mode config #2479

wty-Bryant · 2024-02-02T19:46:41Z

Merge accountID into endpoint rules codegen as an endpoint param builtin retrieved from credentials, add accountIDEndpoint mode config to opt in/out accountID based endpoint routing

lucix-aws · 2024-02-05T15:34:26Z

aws/accountid/account_id.go

@@ -0,0 +1,41 @@
+package accountid


We don't need a new package for this. It can go in the base aws package.

lucix-aws · 2024-02-05T15:35:19Z

aws/accountid/mode/mode.go

@@ -0,0 +1,29 @@
+package mode


Same, base package.

AccountIDEndpointMode. aid is fine for a development branch name but as a consumer-facing identifier we have to be clearer.

lucix-aws · 2024-02-05T16:37:41Z

aws/accountid/mode/mode.go

+
+// enums of valid AIDMode
+const (
+	Preferred AIDMode = "preferred"


It's idiomatic in go to prepend the variants of each enum value with the enum's name.

e.g. AccountIDEndpointModePreferred.

lucix-aws · 2024-02-05T16:40:07Z

codegen/sdk-codegen/aws-models/dynamodb.json

@@ -25,10 +25,35 @@
            {
                "id": "Service",
                "namespace": "*"
+            },


Remove all the model changes in the commit. They should only be present locally for testing purposes.

lucix-aws · 2024-02-05T17:14:08Z

aws/accountid/mode/mode.go

+
+// enums of valid AIDMode
+const (
+	Preferred AIDMode = "preferred"


We should have a none/unset sentinel i.e. AccountIDEndpointModeUnset (which is just the empty string).

lucix-aws · 2024-02-05T17:14:52Z

aws/accountid/mode/mode.go

+)
+
+// SetFromString converts config string to corresponding AIDMode or reports error if the value is not enumerated
+func (mode *AIDMode) SetFromString(s string) error {


There's no real reason to expose this, since sourcing the value from config is an implementation detail for us, let's keep it that way.

lucix-aws · 2024-02-05T17:16:32Z

...codegen/src/main/java/software/amazon/smithy/aws/go/codegen/LegacyEndpointContextSetter.java

@@ -63,7 +64,13 @@ public void renderPreEndpointResolutionHook(GoSettings settings, GoWriter writer
                if $T(ctx) {
                    return next.HandleFinalize(ctx, in)
                }
-                """, SdkGoTypes.Aws.Middleware.GetRequiresLegacyEndpoints);
+                if err := $T(getIdentity(ctx), m.options.AccountIDEndpointMode); err != nil {


This doesn't really belong in this "legacy context" integration, it should have its own entity in codegen that generates it.

lucix-aws · 2024-02-05T17:23:30Z

...y-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/AddAwsConfigFields.java

+                    .build(),
+            AwsConfigField.builder()
+                    .name(SDK_AID_ENDPOINT_MODE)
+                    .type(getAwsAccountIDModeSymbol("AIDMode"))


Add and use SdkGoTypes.Aws.AccountIDEndpointMode (once you've moved the enum there) instead.

lucix-aws · 2024-02-05T17:30:44Z

service/dynamodb/endpoints.go

@@ -489,7 +490,7 @@ type endpointParamsBinder interface {
 	bindEndpointParams(*EndpointParameters)
 }

-func bindEndpointParams(input interface{}, options Options) *EndpointParameters {
+func bindEndpointParams(input interface{}, options Options, ctx context.Context) *EndpointParameters {


Shouldn't we be seeing the account ID binding getting generated here? Specifically somewhere from line 496 on?

I regenerated the client using model without accountID builtin in my last commit. I will regenerate that using mocked model

lucix-aws · 2024-02-07T18:43:05Z

aws/accountid_endpoint_mode.go

@@ -0,0 +1,12 @@
+package aws
+
+// AccountIDEndpointMode switches on/off the account ID based endpoint routing


Suggested change

// AccountIDEndpointMode switches on/off the account ID based endpoint routing

// AccountIDEndpointMode controls how a resolved AWS account ID is handled for endpoint routing.

lucix-aws · 2024-02-07T18:44:39Z

aws/accountid_endpoint_mode.go

+// enums of valid AccountIDEndpointMode
+const (
+	AccountIDEndpointModeUnset     AccountIDEndpointMode = ""
+	AccountIDEndpointModePreferred AccountIDEndpointMode = "preferred"


[nonblocking] You can leave the type declaration off of everything but the first line, the compiler figures it out

e.g.

type Foo = string const ( Foo1 Foo = "1" Foo2 = "2" // is implicitly type Foo as is everything after it )

lucix-aws · 2024-02-07T18:45:30Z

aws/config.go

+	// AccountIDEndpointMode indicates how aws account ID is applied in endpoint2.0 routing.
+	// Will be set to preferred by default. Supported modes are: Preferred, Required and Disabled


Suggested change

// AccountIDEndpointMode indicates how aws account ID is applied in endpoint2.0 routing.

// Will be set to preferred by default. Supported modes are: Preferred, Required and Disabled

// Controls how a resolved AWS account ID is handled for endpoint routing.

lucix-aws · 2024-02-07T18:46:21Z

aws/accountid_endpoint_mode.go

+// AccountIDEndpointMode switches on/off the account ID based endpoint routing
+type AccountIDEndpointMode string
+
+// enums of valid AccountIDEndpointMode


Rather than having the comment on the declaration block, give each one a comment explaining what it does.

lucix-aws · 2024-02-07T18:46:56Z

...y-aws-go-codegen/src/main/java/software/amazon/smithy/aws/go/codegen/AddAwsConfigFields.java

@@ -81,6 +81,8 @@ public class AddAwsConfigFields implements GoIntegration {

    private static final String REQUEST_MIN_COMPRESSION_SIZE_BYTES = "RequestMinCompressSizeBytes";

+    private static final String SDK_AID_ENDPOINT_MODE = "AccountIDEndpointMode";


[nonblocking] We'd rather see this spelled out SDK_ACCOUNTID_ENDPOINT_MODE but it's not a huge deal.

lucix-aws · 2024-02-07T18:49:58Z

.../main/java/software/amazon/smithy/aws/go/codegen/customization/AccountIDEndpointRouting.java

+
+public class AccountIDEndpointRouting implements GoIntegration {
+    @Override
+    public void renderPreEndpointResolutionHook(GoSettings settings, GoWriter writer, Model model) {


We should make sure this happens after the early return that's triggered when we detect they're using v1 endpoint resolution. You may need to fiddle with the integration order to accomplish that.

lucix-aws · 2024-02-07T18:54:27Z

.../main/java/software/amazon/smithy/aws/go/codegen/customization/AccountIDEndpointRouting.java

+            return;
+        }
+        goDelegator.useShapeWriter(settings.getService(model), goTemplate("""
+        func accountID(identity $auth:T, mode $accountIDEndpointMode:T) *string {


A few things here

the name here is too broad, this is likely to collide with random identifiers we generate in the package. I'd call it resolveAccountID, bindAccountID, etc.

you're adding the builtin that calls this in a different location than where it's defined. We should move the accountID function you have here to be generated in the builtins integration instead. (checkAccountID is used here and can stay in this integration)

lucix-aws · 2024-02-07T18:55:09Z

.../main/java/software/amazon/smithy/aws/go/codegen/customization/AccountIDEndpointRouting.java

+        }
+        goDelegator.useShapeWriter(settings.getService(model), goTemplate("""
+        func accountID(identity $auth:T, mode $accountIDEndpointMode:T) *string {
+            if ca, ok := identity.(*$credentialsAdapter:T); ok && (mode == $aidModePreferred:T || mode == $aidModeRequired:T) {


Instead of checking all the modes where it's on, I'd just check the one mode where it's off (disabled) and return early

lucix-aws

fixandship!

lucix-aws · 2024-02-10T00:04:05Z

aws/accountid_endpoint_mode.go

+	// AccountIDEndpointModeUnset indicates the AWS account ID will not be used for endpoint routing
+	AccountIDEndpointModeUnset AccountIDEndpointMode = ""
+
+	// AccountIDEndpointModePreferred indicates the AWS account ID will be used for endpoint routing if offered


Suggested change

// AccountIDEndpointModePreferred indicates the AWS account ID will be used for endpoint routing if offered

// AccountIDEndpointModePreferred indicates the AWS account ID will be used for endpoint routing if present

lucix-aws · 2024-02-10T00:11:28Z

.../main/java/software/amazon/smithy/aws/go/codegen/customization/AccountIDEndpointRouting.java

+                    return $errorf:T("the accountID is configured to be required, but the " +
+                        "identity provider could not be converted to a valid credentials adapter " +
+                        "and provide an accountID, should switch to a valid credentials provider " +
+                        "or change to another account id endpoint mode")


We don't have to specific about the type assertion failing in this path - we can just say an accountID wasn't found.

Suggested change

return $errorf:T("the accountID is configured to be required, but the " +

"identity provider could not be converted to a valid credentials adapter " +

"and provide an accountID, should switch to a valid credentials provider " +

"or change to another account id endpoint mode")

return $errorf:T("accountID is required but not set")

(use this value in the next branch as well)

lucix-aws · 2024-02-10T00:15:12Z

...n/src/main/java/software/amazon/smithy/aws/go/codegen/customization/AwsEndpointBuiltins.java

+        }
+        goDelegator.useShapeWriter(settings.getService(model), goTemplate("""
+        func resolveAccountID(identity $auth:T, mode $accountIDEndpointMode:T) *string {
+            if mode == $aidModeDisabled:T || mode == $aidModeUnset:T {


An unset value is supposed to imply preferred, according to the spec:

preferred, or no value - Default behavior

So this should only check for disabled

Tianyi Wang and others added 3 commits January 31, 2024 12:03

Add accountID builtin codegen, aid mode config and check

923009e

add accountid and associated suppressions to dynamodb endpoint rules

96c864e

Modify accountID endpoint builtin

b4ad6e1

wty-Bryant requested a review from a team as a code owner February 2, 2024 19:46

Tianyi Wang and others added 2 commits February 2, 2024 15:22

Add comment for new exported func and const

729b665

undo most generated code to shrink diff

65e74da

lucix-aws requested changes Feb 5, 2024

View reviewed changes

Tianyi Wang added 2 commits February 6, 2024 14:54

Modify accountID endpoint builtin codegen and config

bf49ee5

remove unexported service feature

02e7fde

wty-Bryant force-pushed the feat-endpoint-builtin branch from 05d0878 to 02e7fde Compare February 6, 2024 21:21

Tianyi Wang added 2 commits February 6, 2024 16:30

restore dynamodb model and service

5764f5c

restore ep context setter format

358cd5e

lucix-aws reviewed Feb 7, 2024

View reviewed changes

Modify accountid endpoint codegen integration

6496537

lucix-aws approved these changes Feb 10, 2024

View reviewed changes

Modify error msg and mode check of aid endpoint mode

d7c2b29

wty-Bryant merged commit f40adb7 into feat-aid-endpoints Feb 12, 2024
10 checks passed

wty-Bryant deleted the feat-endpoint-builtin branch February 12, 2024 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add accountID endpoint builtin and accountID endpoint mode config #2479

Add accountID endpoint builtin and accountID endpoint mode config #2479

wty-Bryant commented Feb 2, 2024

lucix-aws Feb 5, 2024

lucix-aws Feb 5, 2024

lucix-aws Feb 5, 2024

lucix-aws Feb 5, 2024

lucix-aws Feb 5, 2024

lucix-aws Feb 5, 2024

lucix-aws Feb 5, 2024

lucix-aws Feb 5, 2024

lucix-aws Feb 5, 2024 •

edited

Loading

lucix-aws Feb 5, 2024

wty-Bryant Feb 5, 2024

lucix-aws Feb 7, 2024

lucix-aws Feb 7, 2024

lucix-aws Feb 7, 2024

lucix-aws Feb 7, 2024

lucix-aws Feb 7, 2024

lucix-aws Feb 7, 2024

lucix-aws Feb 7, 2024

lucix-aws Feb 7, 2024

lucix-aws left a comment

lucix-aws Feb 10, 2024

lucix-aws Feb 10, 2024

lucix-aws Feb 10, 2024

		@@ -0,0 +1,12 @@
		package aws

		// AccountIDEndpointMode switches on/off the account ID based endpoint routing

	// AccountIDEndpointMode switches on/off the account ID based endpoint routing
	// AccountIDEndpointMode controls how a resolved AWS account ID is handled for endpoint routing.

		// AccountIDEndpointMode indicates how aws account ID is applied in endpoint2.0 routing.
		// Will be set to preferred by default. Supported modes are: Preferred, Required and Disabled

	// AccountIDEndpointMode indicates how aws account ID is applied in endpoint2.0 routing.
	// Will be set to preferred by default. Supported modes are: Preferred, Required and Disabled
	// Controls how a resolved AWS account ID is handled for endpoint routing.

		@@ -81,6 +81,8 @@ public class AddAwsConfigFields implements GoIntegration {

		private static final String REQUEST_MIN_COMPRESSION_SIZE_BYTES = "RequestMinCompressSizeBytes";

		private static final String SDK_AID_ENDPOINT_MODE = "AccountIDEndpointMode";

	// AccountIDEndpointModePreferred indicates the AWS account ID will be used for endpoint routing if offered
	// AccountIDEndpointModePreferred indicates the AWS account ID will be used for endpoint routing if present

Add accountID endpoint builtin and accountID endpoint mode config #2479

Add accountID endpoint builtin and accountID endpoint mode config #2479

Conversation

wty-Bryant commented Feb 2, 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

lucix-aws Feb 5, 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

lucix-aws 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

lucix-aws Feb 5, 2024 •

edited

Loading