[Credentials Cache PR 5] Add IdentityProvider.invalidate#7108
[Credentials Cache PR 5] Add IdentityProvider.invalidate#7108alextwoods wants to merge 9 commits into
Conversation
…validation Add invalidate() method to IdentityProvider interface returning CompletableFuture<Void> to allow the SDK to signal providers to mark cached credentials for refresh when target services reject them with ExpiredToken, InvalidToken, or AuthFailure errors. Key changes: - IdentityProvider: async invalidate(IdentityT) default method - SelectedAuthScheme: carry identityProvider reference (4-arg constructor) - AuthSchemeResolver: pass identityProvider when constructing SelectedAuthScheme - AuthErrorInvalidationHelper: detect auth errors and trigger invalidation - CachedSupplier: invalidate(Predicate) + nextAllowedRefreshTime backoff - All caching providers: invalidate with accessKeyId matching - All delegating providers: forward invalidate to delegate - EndpointResolverUtilsSpec: propagate identityProvider in codegen
alextwoods
force-pushed
the
alexwoo/credential_cache_invalidate
branch
from
e3d6466 to
192c33f
Compare
alextwoods
added
the
api-surface-area-approved-by-team
| @Override | ||
| public CompletableFuture<Void> invalidate(AwsCredentialsIdentity identity) { | ||
| String rejectedAccessKeyId = identity.accessKeyId(); | ||
| return CompletableFuture.runAsync(() -> |
There was a problem hiding this comment.
CompletableFuture.runAsync runs by ForkJoinPool.commonPool() by default and the default number of threads equals to Runtime.getRuntime().availableProcessors() - 1. Since this invalidate is basically sync operation, so it may not matter even for tiny instances, but should we consider avoiding using it anyway in case we have a true async implementation of invalidate in the future and we forget about this? We could use joinLikeSync and return a completed future here. WDYT?
There was a problem hiding this comment.
I honestly went back and forth on this a few times and am still on the fence about. I started it as fully sync before we switched the interface to match the other methods on IdentityProvider, and I switched to just doing the invalidate sync and returning a completed future, but looking at the implementation of invalidate in the CachedSupplier, it waits to acquire the lock - if another thread is updating the cache (which may be a relatively long running service call), we may need to actually wait for some time, making this maybe appropriately more actually async. However, I share the concern with using the common pool to run this so I keep going back and forth. What do you think?
There was a problem hiding this comment.
For now I rolled this back to be effectively sync and return a completed future, but still worth discussing.
| CompletableFuture<?>[] futures = credentialsProviders.stream() | ||
| .map(provider -> { | ||
| try { | ||
| return invalidateProvider(provider, identity) |
There was a problem hiding this comment.
Should we just invalidate last used provider instead of all of them? I'm a bit concerned it may cause surprising behavior for customers if the rest of the credentials providers are valid and actively used elsewhere in their application
2 participants
This PR merges to feature/master/credential_cache, NOT to master
[Credentials Cache PR 5]
Motivation and Context
Add invalidate() method to IdentityProvider interface returning CompletableFuture to allow the SDK to signal providers to mark cached credentials for refresh when target services reject them with ExpiredToken, InvalidToken, or AuthFailure errors.
Modifications
Key changes:
Testing
new and existing tests.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License