Skip to content
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.

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

Account ID endpoint routing #3031

Closed
wants to merge 29 commits into from
Closed

Conversation

dlm6693
Copy link
Contributor

@dlm6693 dlm6693 commented Oct 4, 2023

Adds the ability for AWS services to route customers to account ID-specific endpoints. This PR can be reviewed in two parts.

Part 1 (commits 1-2)

Adds account_id as an optional input to Credentials, RefreshableCredentials and ReadOnlyCredentials. It is also added to DeferredRefreshableCredentials as an internal variable.

It is resolved to a non-null value when possible in a number of supported credential providers:

  • AssumeRoleProvider/AssumeRoleWithWebIdentityProvider: parsed from AssumedRoleUser.Arn in assume role response.
  • SSOProvider: sourced from sso_account_id profile parameter in AWS config file
  • EnvProvider: sourced from AWS_ACCOUNT_ID environment variable
  • ConfigProvider: sourced from aws_account_id profile parameter in AWS config file
  • SharedCredentialsProvider: sourced from aws_account_id profile parameter in AWS shared credentials file
  • ProcessProvider: sourced from AccountId parameter returned in credential process response or aws_account_id profile parameter if the former isn't available.

It can also be configured statically to a botocore client via the aws_account_id arg in Session.create_client or the account_id arg in Session.set_credentials

Part 2 (commits 3-4)

Adds the config setting account_id_endpoint_mode and used during endpoint resolution. When enabled and configured in a service's ruleset, the endpoint resolver will attempt to resolve AccountId as an input parameter to the endpoint provider.

  • Account ID is treated as an endpoint builtin parameter Aws::Auth::AccountId defaulting to None. If the config setting is enabled, the parameter is configured in the ruleset and the builtin hasn't been set during the before-endpoint-resolution event, account ID will be resolved from credentials.
  • valid config settings for account_id_endpoint_mode are preferred, required and disabled (case sensitive). Defaults to preferred if not supplied by a customer.
  • Account ID based routing is always disabled for UNSIGNED requests and presigned URLs

Updates since opening the PR

  • Account ID based routing is not disabled for presigned URLs
  • endpoint builtin resolution is layered through a couple of abstractions. CredentialBuiltinResolver is responsible for actually resolving the account ID value (and future credential builtin values). If/when other types of builtin resolvers are required they will all be passed to EndpointBuiltinResolver. This construct has one public API resolve which is called by the ruleset resolver.

@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e1c055e) 93.46% compared to head (eccd175) 93.50%.
Report is 22 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3031      +/-   ##
===========================================
+ Coverage    93.46%   93.50%   +0.04%     
===========================================
  Files           66       66              
  Lines        14007    14129     +122     
===========================================
+ Hits         13091    13212     +121     
- Misses         916      917       +1     
Files Coverage Δ
botocore/args.py 100.00% <100.00%> (ø)
botocore/config.py 100.00% <ø> (ø)
botocore/configprovider.py 94.78% <ø> (ø)
botocore/credentials.py 98.51% <100.00%> (+0.08%) ⬆️
botocore/exceptions.py 99.56% <100.00%> (+<0.01%) ⬆️
botocore/session.py 97.20% <100.00%> (ø)
botocore/regions.py 92.54% <98.18%> (+0.92%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/unit/test_credentials.py Outdated Show resolved Hide resolved
tests/functional/test_credentials.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jonathan343 jonathan343 left a comment

Choose a reason for hiding this comment

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

Dropping a few comments after a first pass. I can take a deeper look sometime tomorrow.

botocore/exceptions.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/config.py Outdated Show resolved Hide resolved
botocore/config.py Outdated Show resolved Hide resolved
botocore/config.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/credentials.py Outdated Show resolved Hide resolved
botocore/credentials.py Outdated Show resolved Hide resolved
botocore/exceptions.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
tests/functional/test_credentials.py Show resolved Hide resolved
tests/functional/test_credentials.py Outdated Show resolved Hide resolved
tests/unit/test_args.py Outdated Show resolved Hide resolved
tests/unit/test_credentials.py Outdated Show resolved Hide resolved
tests/unit/test_endpoint_provider.py Outdated Show resolved Hide resolved
@dlm6693 dlm6693 force-pushed the account-id-endpoints branch 3 times, most recently from 8335a05 to b3b5034 Compare October 10, 2023 17:44
botocore/exceptions.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/session.py Outdated Show resolved Hide resolved
.changes/next-release/feature-endpoints-57979.json Outdated Show resolved Hide resolved
botocore/args.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
Comment on lines 193 to 199
def test_account_id_refresh(self):
metadata = self.metadata.copy()
metadata['account_id'] = '123456789012'
self.refresher.return_value = metadata
self.mock_time.return_value = datetime.now(tzlocal())
self.assertTrue(self.creds.refresh_needed())
self.assertEqual(self.creds.account_id, '123456789012')
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is a little opaque at first glance. We should make it clear that it's setting the account ID on an otherwise unset pair of credentials during refresh. That could be done with a few word docstring and asserting self.creds.account_id is None before mocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accessing self.creds.account_id triggers the refresh. I've updated to change the expiry time so we can refresh again and set account ID the second time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since there is extra work required to trigger a refresh twice in the same test, I refactored to make the unset check a separate test.

davidlm added 2 commits November 1, 2023 15:48
botocore/args.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/args.py Outdated Show resolved Hide resolved
botocore/regions.py Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
botocore/regions.py Outdated Show resolved Hide resolved
@nateprewitt
Copy link
Contributor

Resolving for inclusion at a later date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants