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

Refactor AWS credentials to support credential expiry and greater modularity #119

Merged
merged 70 commits into from
Nov 17, 2023

Conversation

sjperkins
Copy link
Contributor

@sjperkins sjperkins commented Oct 2, 2023

In #115 (review), the following code base was pointed out: https://github.com/aws/aws-sdk-go/tree/main/aws/credentials.

I've refactored the AWS credentials to generally follow the referenced patterns and return a CachedCredentialProvider which in turn proxies other CredentialProviders. See the following Class diagram:

classDiagram
   direction UD
   class AwsCredentialProvider {
     <<Abstract>>
     +GetCredentials() ResultAwsCredentials
     +ExpiresAt() ResultTime
     +IsExpired() bool
   }
   class ExpiryCredentialProvider {

     +Time expiration_
     +Function clock_
     +SetExpiration(Time, Duration) 
   }

   AwsCredentialProvider <|-- ExpiryCredentialProvider
   AwsCredentialProvider <|-- FileCredentialProvider
   AwsCredentialProvider <|-- EnvironmentCredentialProvider
   AwsCredentialProvider <|-- ChainedCredentialProvider
   ExpiryCredentialProvider <|-- EC2CredentialProvider
   AwsCredentialProvider <|-- CachedCredentialProvider
   EnvironmentCredentialProvider  *--  ChainedCredentialProvider
   FileCredentialProvider  *--  ChainedCredentialProvider
   EC2CredentialProvider *-- ChainedCredentialProvider
   ChainedCredentialProvider *-- CachedCredentialProvider
   note for ExpiryCredentialProvider "Implements \nexpiry with a \nTime Variable and\nClock function"
   note for EnvironmentCredentialProvider "Once retrieved,\nnever expires"
   note for FileCredentialProvider "Once retrieved,\nnever expires"
   note for ChainedCredentialProvider  "Iteratively retrieves credentials\nfrom a list of providers\nIsExpiry + ExpiresAt proxies\nprovider which last retrieved creds"
   note for CachedCredentialProvider "Caches Credentials\nRetrieves new credentials on expiry\nGuards encapsulated Provider with a mutex"
Loading

Then

  • CachedCredentialProvider: Credential retrieval protected by a mutex and credentials cached until they expire
    • ChainCredentialProvider: Retrieves the first valid credentials form a list of credentials
      • EnvironmentCredentialProvider (never expires)
      • FileCredentialProvider (never expires)
      • EC2MetadataCredentialProvider (time based expiry, inherits from ExpiryCredentialProvider)

In particular, the ExpiryCredentialProvider implements a clock based expiry mechanism that will primarily be used with the EC2Metadata server.

Other than that,

  • each provider is split into it's own header, source and test case
  • Credential provider code has been moved into the tensorstore/kvstore/s3/credentials folder.
  • Manual IWYU has been applied.

#115 will depend on this, I can merge this code into that PR if you'd prefer to review the changes that way.

@sjperkins sjperkins mentioned this pull request Oct 2, 2023
2 tasks
@laramiel
Copy link
Collaborator

laramiel commented Oct 2, 2023

Is this intended to replace #115 or is it more or less part of the changeset where #115 gets submitted first? NM, you suggest this happens first. Ok, I'll switch and look at it next.

My comment there was more for reference since I couldn't find the documentation to some of the AWS types; the go code seemed like the easier code to read to discern aws intent...

I'm not sure whether decomposing the responsibilities is very useful when there is really only a single user/implementation of most of these(but I may bias towards functions where others don't).

@sjperkins
Copy link
Contributor Author

sjperkins commented Nov 15, 2023

Maybe running the clangd language server will give you better suggestions than iwyu... iwyu certainly has some misses. In general you can drop an iwyu-included header if none of the types declared in the header are directly spelled in the source files. Unless the file is adl-used...

I installed:

and configured clangd as follows:

If:
  PathMatch: tensorstore/kvstore/s3/*
Diagnostics:
  UnusedIncludes: Strict
  MissingIncludes: Strict

but, while it did seem to be fairly good at spotting unused includes, it didn't seem to detect missing includes all that well. I've applied best effort here but its still been a very manual process and I don't feel confident about it. I may need to spend more time working on configuring it, maybe on the weekend.

@laramiel
Copy link
Collaborator

Can you update the change description to better reflect the current state? I'll import and see what else I need; maybe only the updated description.

@sjperkins sjperkins changed the title AWS credentials refactor to include expiration, chaining and caching Refactor AWS credentials to support credential expiry Nov 17, 2023
@sjperkins sjperkins changed the title Refactor AWS credentials to support credential expiry Refactor AWS credentials to support credential expiry and greater modularity Nov 17, 2023
@sjperkins
Copy link
Contributor Author

Can you update the change description to better reflect the current state?

I've done so.

@sjperkins sjperkins marked this pull request as ready for review November 17, 2023 05:02
@copybara-service copybara-service bot merged commit 4e331ac into google:master Nov 17, 2023
8 checks passed
@sjperkins sjperkins deleted the aws-credentials-expiration branch November 18, 2023 04:30
@laramiel
Copy link
Collaborator

Ok, it looks like that didn't squash properly. Well next time. :)

@sjperkins
Copy link
Contributor Author

sjperkins commented Nov 18, 2023

Ok, it looks like that didn't squash properly

Maybe because there wasn't a merge from master for this commit daed8c2.

Squashing the PR into one commit may be a viable future strategy, although doing so loses the PR history.

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.

2 participants