Feat: Support account ID based endpoints #3718

peterrsongg · 2025-03-21T21:38:37Z

Description

Adds support for Account ID based endpoints. For most assume role operations, the account Id will be taken from the assumed role arn. For SSO we take the accountID from the request. Most static credentials were updated to include accountId as a value in their constructors in a backwards compatible way.

NOTE One other thing I did as part of this is move Services and EndpointUrl to CredentialProfile and away from CredentialProfileOptions. This is because these aren't being used to create unique credential types. It was causing the CredentialProfileType enum to get exponentially larger, making us have to account for all the combinations of EndpointUrl and Services. This also meant that I removed a bunch of CredentialProfileTypes. I figured this was safe to do because it is V4 and this is where we can make breaking changes. Without this change, any addition to CredentialProfileOptions would make the CredentialProfileType enum grow even larger which i thought is unsustainable.

Motivation and Context

Testing

DRY_RUN-2958ec6b-8cf7-417d-8454-5283d4caa519
PS dry run passed

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project
My change requires a change to the documentation
I have updated the documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed

License

I confirm that this pull request can be released under the Apache 2 license

dscpinheiro

I know I'm not one of the reviewers, but I started looking earlier today and had some comments I don't want to lose.

Also, I don't quite understand all the code removal in sdk/src/Core/Amazon.Runtime/CredentialManagement/Internal/CredentialProfileTypeDetector.cs (even after reading the PR description, it doesn't look that would have to be completed at the same time as account-ID based endpoints)

sdk/src/Core/Amazon.Runtime/Credentials/AssumeRoleWithWebIdentityCredentials.cs

sdk/src/Core/Amazon.Runtime/Credentials/AssumeRoleAWSCredentials.cs

sdk/src/Core/Amazon.Runtime/CredentialManagement/AWSCredentialsFactory.cs

dscpinheiro · 2025-03-24T18:56:15Z

.../UnitTests/Custom/Runtime/Credentials/AccountIdEndpointTests/accountid-source-testcases.json

Is this file needed? I assume the tests above are hand-written, right? (If that's the case, I'd prefer not including the JSON from the SEP here).

It isn't needed since it's actually not being read from but I like having it here as reference so someone can know where the tests came from. I also found some mistakes in the SEP's test cases so I pasted the correct version here. (I intend to raise a CR to the sep repo with the fixes).

That's interesting, you should create the SEP CR as soon you can.
We should be confident the issues are with the test cases and not our implementation (not to mention other SDKs have already released support for this feature).

Edit: By as soon as you can I mean before merging this PR even.

generator/.DevConfigs/8a48524d-da8b-4c04-b57e-ad196a51debb.json

normj

With the rework of CredentialProfileTypeDetector how confident do you feel with the existing test coverage and your testing? I like simplifying the logic but there is a lot of specific credential profile configuration I want to be sure we are breaking.

sdk/src/Core/Amazon.Runtime/CredentialManagement/CredentialProfile.cs

normj · 2025-03-25T00:29:49Z

sdk/src/Core/Amazon.Runtime/Credentials/BasicAWSCredentials.cs

+        /// </summary>
+        /// <param name="accessKey"></param>
+        /// <param name="secretKey"></param>
+        /// <param name="accountId"></param>


Lets add some descriptions for this parameters. I think in the past access key and secret key were obvious but account id not so much, particular what it is used for. Also if there are format requirements like the account id with no hyphens we should call that out. Once add comments for the parameters update the other constructor with the docs for access key and secret key.

normj · 2025-03-25T00:30:44Z

sdk/src/Core/Amazon.Runtime/Credentials/AssumeRoleImmutableCredentials.cs

        }

+        public AssumeRoleImmutableCredentials(string awsAccessKeyId, string awsSecretAccessKey, string token, DateTime expiration, string accountId) : this (awsAccessKeyId, awsSecretAccessKey, token, expiration)


Constructor needs docs. For account be sure to add any formatting requirements like no hyphens.

normj · 2025-03-25T00:33:21Z

sdk/src/Core/Amazon.Runtime/Credentials/_bcl+netstandard/SSOImmutableCredentials.cs

        {
            if (string.IsNullOrEmpty(token)) throw new ArgumentNullException(nameof(token));
            Expiration = expiration;
+            AccountId = accountId;


You are passing accountId to the base constructor so setting this again here does not seem necessary.

sdk/src/Core/Amazon.Runtime/Credentials/ImmutableCredentials.cs

sdk/src/Core/Amazon.Runtime/Credentials/SessionAWSCredentials.cs

peterrsongg · 2025-03-25T05:07:00Z

With the rework of CredentialProfileTypeDetector how confident do you feel with the existing test coverage and your testing? I like simplifying the logic but there is a lot of specific credential profile configuration I want to be sure we are breaking.

That was added as part of my work for service-specific endpoints and we have extensive test coverage that was added when I did that work. The tests here. I've always wanted to clean this class up b/c I realized only after I did that work that I should've added them to CredentialProfile and not CredentialProfileOptions. I'm confident we won't break any credential profile configuration, specifically the type of credentials we will be resolving, because in AWSCredentialsFactory all of the cases for endpointUrl + services credential profile types returned the same credentials anyways.

peterrsongg changed the title ~~Petesong/acct id ep~~ Feat: Support account ID based endpoints Mar 21, 2025

peterrsongg requested a review from muhammad-othman March 24, 2025 18:42

dscpinheiro requested review from normj and dscpinheiro March 24, 2025 18:42

peterrsongg removed the request for review from dscpinheiro March 24, 2025 18:42

dscpinheiro added the v4 label Mar 24, 2025

dscpinheiro reviewed Mar 24, 2025

View reviewed changes

normj requested changes Mar 25, 2025

View reviewed changes

peterrsongg requested review from dscpinheiro and normj March 25, 2025 15:58

peterrsongg added 8 commits March 25, 2025 10:43

feat: support acct id based endpoints

c61b98d

Move Services to CredentialProfile and remove it from Options

589e127

Move endpointurl and services to credentialprofile and not options

5a205f3

Fix tests add cleanup for acct-id-tests

7b28fe2

clean up

c0c02c6

Update ProcessAWSCredentials constructor

bab3c9d

fix failing test

498b08e

Address PR feedback

8fcde17

peterrsongg force-pushed the petesong/acct-id-ep branch from aed93cf to 8fcde17 Compare March 25, 2025 17:44

dscpinheiro removed the request for review from muhammad-othman March 26, 2025 17:21

normj approved these changes Mar 28, 2025

View reviewed changes

dscpinheiro approved these changes Mar 28, 2025

View reviewed changes

dscpinheiro merged commit d40e23a into v4-development Mar 28, 2025
1 check passed

dscpinheiro deleted the petesong/acct-id-ep branch March 28, 2025 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Support account ID based endpoints #3718

Feat: Support account ID based endpoints #3718

peterrsongg commented Mar 21, 2025 •

edited

Loading

dscpinheiro left a comment

dscpinheiro Mar 24, 2025

peterrsongg Mar 25, 2025

dscpinheiro Mar 26, 2025 •

edited

Loading

normj left a comment

normj Mar 25, 2025

peterrsongg Mar 25, 2025

normj Mar 25, 2025

peterrsongg Mar 25, 2025

normj Mar 25, 2025

peterrsongg commented Mar 25, 2025

		}

		public AssumeRoleImmutableCredentials(string awsAccessKeyId, string awsSecretAccessKey, string token, DateTime expiration, string accountId) : this (awsAccessKeyId, awsSecretAccessKey, token, expiration)

Feat: Support account ID based endpoints #3718

Feat: Support account ID based endpoints #3718

Conversation

peterrsongg commented Mar 21, 2025 • edited Loading

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

dscpinheiro left a comment

Choose a reason for hiding this comment

dscpinheiro Mar 24, 2025

Choose a reason for hiding this comment

peterrsongg Mar 25, 2025

Choose a reason for hiding this comment

dscpinheiro Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

normj left a comment

Choose a reason for hiding this comment

normj Mar 25, 2025

Choose a reason for hiding this comment

peterrsongg Mar 25, 2025

Choose a reason for hiding this comment

normj Mar 25, 2025

Choose a reason for hiding this comment

peterrsongg Mar 25, 2025

Choose a reason for hiding this comment

normj Mar 25, 2025

Choose a reason for hiding this comment

peterrsongg commented Mar 25, 2025

peterrsongg commented Mar 21, 2025 •

edited

Loading

dscpinheiro Mar 26, 2025 •

edited

Loading