OCPBUGS-59734: fix(azure): resolve credential caching issues around UAMI support #1238
Conversation
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bryan-cox, sjenning The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/test hypershift-e2e-aks |
1 similar comment
|
/test hypershift-e2e-aks |
bryan-cox
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-60103, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
…untsClient Similar to the azureclient fix, use consistent azureCredentialsKey for both storing and loading cached credentials instead of mixing azureCredentialsKey and userAssignedIdentityCredentialsFilePath. This prevents repeated credential recreation in the Azure driver's storageAccountsClient method.
Use consistent azureCredentialsKey for both storing and loading cached credentials instead of mixing azureCredentialsKey and userAssignedIdentityCredentialsFilePath. This prevents repeated credential recreation in the Azure driver's storageAccountsClient method. Signed-off-by: Bryan Cox <[email protected]>
|
/test hypershift-e2e-aks |
|
/test hypershift-e2e-aks |
|
/test hypershift-e2e-aks |
|
/test hypershift-e2e-aks |
|
Hey @bryan-cox! Can you elaborate on your decision of caching the entire storage driver, vs for example caching the credentials within the driver? |
@flavianmissi it's still in WIP so I wouldn't consider what is here to be the final solution. |
|
Understood, I'll hold my horses 🤠 |
bryan-cox
changed the title
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-59734, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
bryan-cox
force-pushed
the
inotify-bug
branch
2 times, most recently
from
860c446 to
e704b6b
Compare
|
/test hypershift-e2e-aks |
|
/test hypershift-e2e-aks |
bryan-cox
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-59734, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@bryan-cox: This pull request references Jira Issue OCPBUGS-59734, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. 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 openshift-eng/jira-lifecycle-plugin repository. |
- Add ensureUAMICredentials function to obtain and cache Azure TokenCredential using User Assigned Managed Identity (UAMI) - Function loads credentials from global cache or creates new ones with proper environment configuration - Add comprehensive unit tests covering environment variable handling, cache behavior, and error scenarios - Tests use table-driven pattern following codebase conventions and t.Setenv for proper environment handling - Update Azure client instantiation to use UAMI credentials when available
|
@bryan-cox: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
|
@flavianmissi this is ready for review now 😄 |
|
The logs show the credentials being stored once then loaded from the cache then on ''' I0813 00:21:04.989721 1 azure.go:1348] Loaded UAMI credentials from cache |
Summary
This PR fixes credential caching issues in Azure storage operations and adds caching support for User Assigned Managed Identity (UAMI) credentials.
Caching at the driver level was not enough so a global cache was introduced so that we are not getting new credentials over and over from Azure.
Changes
fix(azure): fix credential caching key mismatch in driver storageAccountsClient
Resolves credential caching key inconsistencies in the storage accounts client
fix(azure): fix credential caching key mismatch in azureclient
Fixes credential caching key mismatches in the Azure client implementation
feat(azure): add ensureUAMICredentials function with comprehensive tests
ensureUAMICredentialsfunction to obtain and cache Azure TokenCredential using User Assigned Managed Identity (UAMI)t.Setenvfor proper environment handlingTesting
ensureUAMICredentialsfunctionImpact