-
Notifications
You must be signed in to change notification settings - Fork 431
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 credentials from AzureClusterIdentity #5283
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5283 +/- ##
==========================================
+ Coverage 53.00% 53.03% +0.03%
==========================================
Files 272 272
Lines 29429 29404 -25
==========================================
- Hits 15598 15595 -3
+ Misses 13027 13010 -17
+ Partials 804 799 -5 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
g.Expect(err).NotTo(HaveOccurred()) | ||
cred, err := provider.GetTokenCredential(context.Background(), "", tt.ActiveDirectoryAuthorityHost, "") | ||
_, err = provider.GetTokenCredential(context.Background(), "", tt.ActiveDirectoryAuthorityHost, "") |
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.
why aren't we asserting the first response object?
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.
That value is returned by a mock defined by the test, so it doesn't really mean anything.
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.
The main validation of that is covered by verifying the arguments we pass to the p.cache.GetOrStore...
calls in the cacheExpect
of each test case.
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.
I see that GetTokenCredential
returns non-nil every time err
is nil, maybe I'm being extra-defensive and protecting against the possibility that GetTokenCredential
changes and an undesirable outcome is identified only in the TokenCredential
object.
Non-blocking.
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.
/lgtm
/approve
LGTM label has been added. Git tree hash: da43590508b97601bc43c1df3f70462dc8c96177
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis 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 |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR leverages the credential cache added in #5211 for credentials defined in AzureClusterIdentity resources. This lets CAPZ save token handles between reconciles instead of re-authenticating every reconciliation loop.
These graphs show the reconciles involving authentication requests made by CAPZ when creating two workload clusters that used the same credentials before and after implementing the cache:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1077
Special notes for your reviewer:
The most interesting change here is in azure/scope/identity.go, which inserts the cache in place of the direct SDK calls. The rest of the changes plumb the cache created in main to there.
TODOs:
Release note: