-
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
handle Entra auth for ASO API managed clusters #5211
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5211 +/- ##
==========================================
+ Coverage 52.66% 52.99% +0.33%
==========================================
Files 273 272 -1
Lines 29189 29427 +238
==========================================
+ Hits 15371 15596 +225
+ Misses 13029 13027 -2
- Partials 789 804 +15 ☔ View full report in Codecov by Sentry. |
/assign |
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
Makes sense to me, and the docs and tests are great. All I could find to comment on was a doc line. Happy to approve unless someone else is slated to review.
LGTM label has been added. Git tree hash: 9b6d3e9d8446171a8549683aa17b83e3f8d9156e
|
Only running this here to make sure #5272 really does break this test: |
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.
minor nit, non blocking
/lgtm
LGTM label has been added. Git tree hash: 3a9411eff083f1ecc4c475f4367360e83e449236
|
/retest |
@nojnhuh: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
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: 4adc28e3d5987f284ef608f58e6647f86eee6a77
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mboersma 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 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
There are two commits here. The first adds an implementation of a cache that remembers
azcore.TokenCredential
s based on the auth parameters like type, tenant ID, client ID.TokenCredentials
take care of fetching new access tokens themselves when they expire, so CAPZ should be able to keep them around in the cache indefinitely. Later I plan to hook this cache into AzureClusterIdentity for #1077.The second commit ports similar logic from #4008 from the AzureManagedControlPlane API to AzureASOManagedControlPlane to handle AKS clusters with local accounts disabled. I added docs with more context.
The cache is necessary because CAPZ needs a way to get the same access token multiple times across reconciles so it doesn't get stuck in a cycle of:
TokenCredential
With the same
TokenCredential
handle, repeated calls toGetToken()
will return the same access token until the current one expires, and then it will return a new one.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 #5017
Special notes for your reviewer:
I plan to cover this with e2e tests via AKS Automatic in #5208.
TODOs:
Release note: