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

Support credential caching by sdk users. #2375

Merged
merged 2 commits into from
Jan 3, 2019

Conversation

llamahunter
Copy link
Contributor

Add an Expirer interface that Providers can implement, and add a suitable
implementation to Expiry class used by most Providers. Add a method on
Credentials to get the expiration time of the underlying Provider, if
Expirer is supported, without exposing Provider to callers.

If there is an existing bug or feature this PR is answers please reference it here.
Provides partial solution to issue #1329 discussion.

Add an Expirer interface that Providers can implement, and add a suitable
implementation to Expiry class used by most Providers. Add a method on
Credentials to get the expiration time of the underlying Provider, if
Expirer is supported, without exposing Provider to callers.
@llamahunter
Copy link
Contributor Author

@jasdel could you take a look at this PR? Needless to say, it would be great to get it accepted and released ASAP so that I can submit a pull to the aws-iam-authenticator project that depends on this. Would rather not hand vendor this directly in their project.

@llamahunter
Copy link
Contributor Author

It would be great to get this accepted and released soon. It's blocking my ability to create a pull request for aws-iam-authenticator fixing the lack of SSO credential caching there.

Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

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

Thanks for putting together this PR. I think we could take this feature, with a few updates to protecting against race retrieving the expiration time, and handling credentials that have already "expired".

aws/credentials/credentials.go Outdated Show resolved Hide resolved
aws/credentials/credentials.go Show resolved Hide resolved
@jasdel jasdel added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed review-needed labels Dec 26, 2018
Expire() directive by returing an expired time for ExpiresAt()
@llamahunter
Copy link
Contributor Author

Let me know if you want me to squash these changes together.

@llamahunter
Copy link
Contributor Author

Any chance this pull could be accepted soon?

@jasdel jasdel removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Dec 28, 2018
@jasdel
Copy link
Contributor

jasdel commented Dec 28, 2018

Thanks for the updates, I'll review these.

The forceRefresh flag is used by the SDK to mark current credentials as no longer valid. This can happen when credentials have been revoked. When credentials are revoked, the service will send an specific error code. The SDK identifies the error code and marks the credentials as expired so that the next API request will refresh the credentials.

@llamahunter
Copy link
Contributor Author

But.... no one calls Credentials.Expire() or sets forceRefresh to true after Credentials.Get() is called. It seems unused in any non testing/example part of the SDK. Unimplemented feature?

@llamahunter
Copy link
Contributor Author

Or is this API for applications written on top of the SDK to force credential expiration? Seems odd to push that up out of the SDK, tho.

@jasdel
Copy link
Contributor

jasdel commented Dec 28, 2018

The SDK's built in request handler AfterRetryHandler will call Expire if the service's API response has an error that signifies the credentials are no longer valid.

Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

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

Thanks for making these updates the changes look good. We won't be able to merge the change in today, but will be able to after the new year.

Thanks for taking the time to create this PR, and updates.

@llamahunter
Copy link
Contributor Author

apologies... I somehow missed that one when searching. So, if I'm caching credentials at a higher level in the calling program, and the credential I'm using is expired, the AfterRetryHandler will call Expire on the Credential, and then... it looks like... automatically retry? I don't think I will need to do anything special so long as I support the forceRetry flag on ExpiresAt()

@jasdel
Copy link
Contributor

jasdel commented Dec 28, 2018

Will your application be caching the credenitals.Value or credentials.Credentials value? I suggest using the credentials.Credentials value as it will always be up-to-date. In addition the Credentials provides synchronous locks for usage concurrently.

Using the forceRefresh flag in ExpiresAt will help ensure any entity using the Credentials value to cache AWS Credentials will correctly know when to expect the credentials to expire, or if they are forced expired by another entity such as SDK's API requests.

@llamahunter
Copy link
Contributor Author

It will be caching credentials.Value to disk, and only calling Credentials.Get() if the on disk cache of the credentials.Value has an expiration time that is in the past. This is per your suggestions in issue #1329.

@llamahunter
Copy link
Contributor Author

Also, FWIW, it seems to work fine. Already have implemented it for aws-iam-authenticator. Just waiting for this pull request to go in before submitting a pull to that project.

@llamahunter
Copy link
Contributor Author

HNY! What's the schedule look like for pulling this and publishing a new release including it?

@jasdel jasdel merged commit 1fc64be into aws:master Jan 3, 2019
@jasdel
Copy link
Contributor

jasdel commented Jan 3, 2019

Thanks for creating this PR. I've merged it into master, and it will be included in the next tagged release. I expect a tagged release to be created this week.

jasdel added a commit that referenced this pull request Jan 3, 2019
Adds change PRs: #2380, #2373, #2375 to pending change log.
@aws-sdk-go-automation aws-sdk-go-automation mentioned this pull request Jan 3, 2019
@llamahunter llamahunter deleted the expirer-provider branch January 3, 2019 23:15
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