-
Notifications
You must be signed in to change notification settings - Fork 424
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
Cache aws credentials #193
Cache aws credentials #193
Conversation
profile = session.DefaultSharedConfigProfile | ||
} | ||
// create a cacheing Provider wrapper around the Credentials | ||
if cacheProvider, err := NewFileCacheProvider(clusterID, profile, roleARN, sess.Config.Credentials); err == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If caching is enabled, wrap the existing Credentials (and its contained Provider) in a new caching provider.
/assign mattlandis |
Hmm... I think I might be using confusing terminology. This is really caching the AWS credentials, not the kubernetes access token. Would you like me to fix up the identifiers and comments to be clearer about what's going on? Also, probably should reference aws/aws-sdk-go#1329 somewhere in the code, so that people understand why this needs to be done, as the aws-sdk-go people say it's not their responsibility to cache credentials, and any app credential caching should not share a cache with the awscli for compatibility reasons. |
@nckturner @mattmoyer any thoughts on this? Any rework needed? Can it be merged & released? |
/assign |
Still waiting for feedback on this. Does it need rework? Can it be merged? |
Hey... it's been about a month now.... please comment. |
Any activity on this review process? Would love to see this merged. |
I can only ask so many times.... @CharlieC3 do you have any feedback on it? |
I'm no official reviewer of this repo, but... I'm assuming you were able to build/test this on your end @llamahunter, but I was not due to dependency issues. Given, I only tried for maybe 15 minutes :) I'll give it another go this week to see if I can test it on my end, otherwise the source code looks good at a quick glance, and the PR comments you left are helpful. |
Hmm... I would hope the branch would be able to build. Go is pretty good about packaging all the dependent foo together. The Travis CI builds succeeded, so I think build problems are probably a local config issue on your side. It seems to work pretty well for me. I have a number of different IAM roles I can adopt to switch to different permissions levels within the cluster. Here's a section of my .kube/config file
and my corresponding .aws/config
My default profile has a corresponding entry in .aws/credentials
and so I don't have the --cached argument for that user in my .kube/config. The other two users use okta-credential-process to get SSO credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need something similar to this PR to relieve the pain of MFA caching. Apologies on the time to review.
Thx for the feedback. Will review above recommendations and push new code to the PR in about a week. Currently out of the country traveling w/o computer. |
Create a disk cache for credentials, based on cluster, profile, and roleARN, and use cached credentials until they expire. Requires that the credential supports Expirer interface (i.e. works for SSO credenital_process credentials, but not for static unexpiring credentials). Caching based on profile/roleARN permits easy switching of kubectl config between different user contexts by configuring different profiles using AWS_PROFILE env variable in the exec block of .kube/config
Addressed the review comments and rebased against master. |
Appreciate the changes, taking a look. |
Tested this PR after the changes and it works great. Let's get it merged! Specifically tested:
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: llamahunter, nckturner The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
🙌 Any timeline as to when we can see this in a release? |
@robwittman working on it! Hopefully this week. |
Looks cool. Thanks for tackling this. Look forward to trying it in a release. I'd like to consider swapping out our existing MFA caching solution to replace it with this. I tried to derive answers to some technical questions about this by reviewing the code changes but wasn't able to. If these get addressed in the release notes, please ignore and just ping me with a link to that when it ships. some assumed questions / answers How do you enable caching?I assume Where does it store it's cached credentials on disk?I assume somewhere in What does it cache?I assume something like this is cached on disk and that it lasts for 15 minutes before re-prompting for MFA. Yea?
|
See Credentials are stored in It caches the |
@llamahunter Thanks for all the hard work on this! Much appreciated! |
What is your SSO system? If using Okta, you must set the time in the ~/.okta/config.properties to set the OKTA_STS_DURATION. YMMV if using a different SSO system. |
@truncs I don't believe the duration is configurable due to a lack of configuration options in the AWS API. |
Longer durations up to 12 hrs for federated access using roles for about a year now. https://aws.amazon.com/about-aws/whats-new/2018/03/longer-role-sessions/ |
@llamahunter According to the links in my previous comment, and comments from the other PRs, it seems like So it's possible both duration timeouts are getting used here, but the shorter one is not configurable and is consequently the one we're being affected by. Keep in mind I'm not familiar with this project's source code, but the description of the 15 minute timeout duration for the |
I think you're talking about token timeouts. This is about caching credentials, not tokens. I think there's another issue addressing the caching of tokens, for which there is a built in limit by AWS of 10-15 minutes. |
See the list of 'possibly addresses in alternate way' issues listed at the start of this pull request. |
@llamahunter You might be right, I was under the impression they were affected by the same issue described in this comment. |
Yes, per their comment, they appear to be using Okta SSO 2FA stuff, like we do. If they set the IAM federated role connected to their Okta sign on to expire in 12 hours, and enable caching in aws-iam-authenticator, I suspect their lives will be a lot more pleasant. I believe aws-iam-authenticator still fetches the token every time, tho, which, in my experience, is a short time delay (< 1 second). It would be nice if the token were cached for the full 15 minutes to skip kubectl talking to anything other than the EKS k8s control plane endpoint on every invocation. |
@llamahunter we use 2FA functionality provided by AWS. |
Fixes #183
Possibly fixes or addresses the following in an alternate way: #151, #99, #63, #140