-
Notifications
You must be signed in to change notification settings - Fork 637
Fix session surviving cluster purge and recreate through cache #4162
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
Fix session surviving cluster purge and recreate through cache #4162
Conversation
|
|
|
Welcome @roehrijn! |
|
Hi @roehrijn. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@roehrijn: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
* session needs to be removed from cache in case of faulty retrieval from credentials providers
1014a29 to
3636d8f
Compare
|
Hello @roehrijn. Would you please update the description to include release notes? Thanks. /test ? |
|
@Skarlso: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test pull-cluster-api-provider-aws-e2e-eks |
|
Hi @Skarlso,
I thought I already did this - propably before your comment. But now also added the release nodes prompt. Is it correct now? Looking forward to get this into main. Btw., we're having this fix in our own fork and successfully using it in production since a few weeks. |
|
Yep looks alright now. Thanks. I'll take a look later on. I've been sick with Covid these past couple of days. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Skarlso 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 |
|
Hi @Skarlso, hope you're well again. No worries, I also had several gaps of multiple days where I had no time to look into this PR. Now I'm happy about my first merged contribution to a CNCF project. More to come ... |
What type of PR is this?
/kind bug
What this PR does / why we need it:
AWS sessions need to be removed from cache in case of faulty retrieval from credentials providers. Otherwise a defective session can survive a delete of a cluster and recreate with same name but new credentials.
Steps to reproduce:
Result: Due to the two-layered caching approach in
sessionForClusterWithRegion()(pkg/cloud/scope/session.go:108) the session created using the credentials from step #1 is able to survive and still be used for the newly created cluster, which results in various 401/403 errors from AWS API.This PR makes sure that the old session is explcitly evicted from cache as soon as it is no longer possible to retrieve the credentials from cached credential providers.
Checklist:
Release note:
Jan Röhrich <[email protected]>, Mercedes-Benz Tech Innovation GmbH (Provider Information)