Skip to content

fix: account ID based endpoints #1535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 25, 2025
Merged

fix: account ID based endpoints #1535

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1b60ff0
fix: account ID based endpoints
0marperez Feb 20, 2025
acb78a0
misc: pr feedback v1
0marperez Feb 20, 2025
07d9385
misc: simplify test
0marperez Feb 21, 2025
56bd6eb
misc: smithy-kotlin version bump
0marperez Feb 25, 2025
233c6f1
Merge branch 'main' into fix-acc-id-ep
0marperez Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions codegen/aws-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/endpoints/AwsBuiltins.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package aws.sdk.kotlin.codegen.endpoints

object AwsBuiltins {
const val ACCOUNT_ID = "AWS::Auth::AccountId"
const val ACCOUNT_ID_ENDPOINT_MODE = "AWS::Auth::AccountIdEndpointMode"
const val USE_FIPS = "AWS::UseFIPS"
const val USE_DUAL_STACK = "AWS::UseDualStack"
const val S3_ACCELERATE = "AWS::S3::Accelerate"
Expand Down
8 changes: 8 additions & 0 deletions ...s-sdk-codegen/src/main/kotlin/aws/sdk/kotlin/codegen/endpoints/BindAwsEndpointBuiltins.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@ fun renderBindAwsBuiltins(ctx: ProtocolGenerator.GenerationContext, writer: Kotl
AwsRuntimeTypes.Config.Endpoints.resolveAccountId,
AccountIdEndpointBuiltinCustomization.AccountIdEndpointModeProp.propertyName,
)

AwsBuiltins.ACCOUNT_ID_ENDPOINT_MODE -> {
Copy link
Member

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment at the top of this function says "In practice, all of these values are sourced from the client config."

Is that still true? Isn't AccountId being sourced exclusively from endpoint rules in your manual test cases?

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that still true? Isn't AccountId being sourced exclusively from endpoint rules in your manual test cases?

Account ID isn't being sourced exclusively from endpoint rules. It's sourced from client config as a built in and from the operation context params using JMESPath.

The endpoint rules use both values for different conditions

writer.write(
"#L = config.#L.toString().lowercase()", // DDB endpoint rules assume lowercase value
Copy link
Member

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comment about DDB since this rule applies for all AWS endpoints

Copy link
Contributor

@ianbotsf ianbotsf Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: DDB isn't adding any special constraints here. The spec says:

The value will be propagated to endpoint rules as the built-in endpoint parameter AWS::Auth::AccountIdEndpointMode. This setting is an enum with the values required, disabled and preferred.

We should tweak this comment so that we don't get confused later about which services require lowercase values.

Copy link
Contributor Author

@0marperez 0marperez Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just remove the comment in that case

Copy link
Member

@lauzadis lauzadis Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to mention that the enum values are expected to be lowercase

Copy link
Contributor

@ianbotsf ianbotsf Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially since it's different from how we treat basically all other enums in Kotlin.

it.defaultName(),
AccountIdEndpointBuiltinCustomization.AccountIdEndpointModeProp.propertyName,
)
}
}
}
}
Expand Down
Loading