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

EC2CredentialsFetcher seems to force refresh, when expiration time has not been reached. #1921

Closed
tmc24win opened this issue Feb 22, 2019 · 7 comments
Labels

Comments

@tmc24win
Copy link

@tmc24win tmc24win commented Feb 22, 2019

It seems like the logic in this function (isWithinExpirationThreshold() line 198) should evaluate greater than the expiration threshold.

(credentialsExpiration.getTime() - System.currentTimeMillis()) < EXPIRATION_THRESHOLD;

I checked that our iam server response returns an expiration date that is 10 minutes from now() and this provider is still forcing a refresh on every request. Since this is a synchronized code block it is severely impacting performance.

It also would be good to add some TRACE or DEBUG message, when the provider has to refresh the credentials.

@tmc24win

This comment has been minimized.

Copy link
Author

@tmc24win tmc24win commented Feb 22, 2019

It is possible that I am not understanding the intended functionality regarding EXPIRATION_THRESHOLD, but it seems like if the credentials are within 15 minutes, it constantly will force a re-fresh of the same credentials with the same expiration date negatively impacting performance. It would be beneficial to add the debug logging and possibly improve the documentation around what the purpose of the expiration threshold is.

@debora-ito debora-ito added the guidance label Feb 22, 2019
@debora-ito

This comment has been minimized.

Copy link
Collaborator

@debora-ito debora-ito commented Feb 22, 2019

Hi @tmc24win, your second comment is correct, if the credentials expiration date are within the 15 minutes they will be refreshed.

This doc about Using IAM Roles with EC2 mentions the 15 minutes expiration, but I agree with you, there should be a clearer documentation about this rule. I'll request that to the documentation team.

There is a feature request (#1893) to drop EXPIRATION_THRESHOLD to 5 minutes and make it configurable, you can +1 it to help the team prioritize its implementation.

@tmc24win

This comment has been minimized.

Copy link
Author

@tmc24win tmc24win commented Feb 22, 2019

Thanks, I will +1 that issue and add my comments to the other issue.

@debora-ito

This comment has been minimized.

Copy link
Collaborator

@debora-ito debora-ito commented Feb 25, 2019

Do you have any suggestion where you'd like to see the documentation improvement?

@tmc24win

This comment has been minimized.

Copy link
Author

@tmc24win tmc24win commented Feb 25, 2019

After getting more understanding of the EC2CredentialsFetcher, it remains unclear to me what the difference between the REFRESH_THRESHOLD and the EXPIRATION_THRESHOLD is. It also seems having a refresh threshold greater than even a minute doesn't provide much use, because until the expiration time is actually reached, it continue to receive the same token.

Are you also planning to remove the synchronized method?

@debora-ito

This comment has been minimized.

Copy link
Collaborator

@debora-ito debora-ito commented Feb 25, 2019

The credentials are refreshed if:

  • The credentials expiration are within the EXPIRATION_THRESHOLD of 15 minutes; OR
  • the last attempt to refresh credentials is greater than the REFRESH_THRESHOLD.

Currently REFRESH_THRESHOLD is 60 minutes. So if your credentials do not expire within 15 minutes but the last time you used them was more than 60 minutes ago, they'll be refreshed.

@debora-ito

This comment has been minimized.

Copy link
Collaborator

@debora-ito debora-ito commented Mar 4, 2019

Closing this, let's keep the synchronized discussion in the feature request thread. Feel free to reopen if you have more questions.

@debora-ito debora-ito closed this Mar 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.