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

Add option for refreshing expired AWS token #57

Conversation

andrejvanderzee
Copy link
Contributor

We are running in Kubernetes context with ephemeral AWS credentials which are refreshed every X hours. This PR adds an option that re-reads credentials from AWS shared credential file and retries the request.

@andrejvanderzee andrejvanderzee requested a review from a team as a code owner June 30, 2020 13:32
@CLAassistant
Copy link

CLAassistant commented Jun 30, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #57 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #57   +/-   ##
=======================================
  Coverage   72.37%   72.37%           
=======================================
  Files          12       12           
  Lines         992      992           
=======================================
  Hits          718      718           
  Misses        212      212           
  Partials       62       62           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97429fa...5143caf. Read the comment docs.

@eytan-avisror
Copy link
Collaborator

Thanks for the contribution @andrejvanderzee 👍
Changes look good outside of the removal of caching.
We need to get caching to work with your changes since it's very critical for scalability.

@andrejvanderzee andrejvanderzee force-pushed the invalidate_creds_in_retry_chain branch from 327520e to 82b3a1b Compare June 30, 2020 17:08
@andrejvanderzee andrejvanderzee force-pushed the invalidate_creds_in_retry_chain branch from 82b3a1b to 5143caf Compare June 30, 2020 17:11
@andrejvanderzee
Copy link
Contributor Author

@eytan-avisror Yes you are right I guess I overlooked. Fixed the caching statements. Can you please have another look?

@eytan-avisror
Copy link
Collaborator

Thanks @andrejvanderzee it looks good.
Did you get a chance to test this by running it and make sure it's functional?
This project doesn't have an automated functional test yet unfortunately.

@andrejvanderzee
Copy link
Contributor Author

Yes I had it running and looks good. Here the debug output of a recovering connection from an expired token 403 response:

time="2020-06-30T13:24:35Z" level=debug msg="active goroutines: 7"
time="2020-06-30T13:24:35Z" level=debug msg="retryable: ExpiredToken: The security token included in the request is expired\n\tstatus code: 403, request id: 606a5f3a-9efb-5126-8345-084fa1a848aa -- sqs/ReceiveMessage, will retry after 1.533618922s"
time="2020-06-30T13:24:40Z" level=debug msg="no messages received in interval"
time="2020-06-30T13:24:40Z" level=debug msg="polling for messages from queue"
time="2020-06-30T13:24:40Z" level=debug msg="active goroutines: 7"
time="2020-06-30T13:24:43Z" level=debug msg="no messages received in interval"

Copy link
Collaborator

@eytan-avisror eytan-avisror left a comment

Choose a reason for hiding this comment

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

🥇

@eytan-avisror eytan-avisror merged commit a2f5198 into keikoproj:master Jun 30, 2020
@andrejvanderzee
Copy link
Contributor Author

@eytan-avisror Thanks for your prompt replies and merging my PR!

@andrejvanderzee
Copy link
Contributor Author

BTW, would it be possible to tag 0.4.1?

@eytan-avisror
Copy link
Collaborator

@andrejvanderzee NP, thanks again for contributing.
Will release 0.4.1 today, we have other fixes as well that will go in.

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.

None yet

3 participants