Conversation
lauzadis
added
the
no-changelog
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
...ws-signing-common/common/src/aws/smithy/kotlin/runtime/auth/awssigning/AuthTokenGenerator.kt
Outdated
Show resolved
Hide resolved
| val config = AwsSigningConfig { | ||
| credentials = creds | ||
| this.region = region | ||
| service = serv |
There was a problem hiding this comment.
Nit: You can do this.service = service to avoid creating serv. Same for credentials: this.credentials = credentials.value
There was a problem hiding this comment.
No it's not possible, I tried already. It's because those service and credentials are class parameters not function parameters
There was a problem hiding this comment.
I thought about it some more and was able to get this to work: service = this@AuthTokenGenerator.service
| if (!::credentials.isInitialized || (credentials.expiresAt - clock.now()).absoluteValue <= credentialsRefreshBuffer) { | ||
| val resolved = credentialsProvider.resolve() | ||
| credentials = ExpiringValue(resolved, resolved.expiration ?: (clock.now() + DEFAULT_CREDENTIALS_EXPIRATION)) | ||
| } |
There was a problem hiding this comment.
Question: Why duplicate this logic from our caching layer? Why not simply call the resolve method on the provider like we do in regular signing?
There was a problem hiding this comment.
Shouldn't we try to cache credentials instead of resolving them for every call to generateAuthToken?
There was a problem hiding this comment.
I guess the default chain will cache credentials, but not if a user passes in a custom credentials provider.
There was a problem hiding this comment.
Yes, people who use a custom creds provider without caching will face the same behaviors in regular requests and in auth token generation, not all of which are necessarily negative. If someone intentionally wants fresh credentials every time or they have their own caching mechanism, we'll bypass that by adding in a new caching layer. We've got a clean, easy way for users to wrap their custom provider chains/impls in our caching provider which should be good enough.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Affected ArtifactsSignificantly increased in size
|
3 participants
Issue #
Description of changes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.