Refactor AKS client credential configuration - #1305
Merged
Diamond (diamondpowell) merged 4 commits intoSep 2, 2026
Merged
Conversation
|
For reviewers only: reply |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
Changes are cohesive, well-covered by updated tests, and no functional issues were found beyond a small documentation nit.
Pull request overview
Refactors Azure authentication so AKSClient no longer constructs credentials internally; instead, callers must inject an Azure credential while higher-level CRUD wrappers use a shared configure_credential helper to build a consistent default.
Changes:
- Added
utils.azure_auth.configure_credential()to centralize Azure credential construction. - Updated
AKSClientto require an injected credential (removing implicit credential creation inside the client). - Updated Azure CRUD wrappers and unit tests to use/pin the new credential injection boundary.
File summaries
| File | Description |
|---|---|
| modules/python/utils/azure_auth.py | Introduces shared helper to construct DefaultAzureCredential or ManagedIdentityCredential. |
| modules/python/tests/utils/test_azure_auth.py | Adds focused unit tests for the new credential helper behavior. |
| modules/python/clients/aks_client.py | Requires caller-provided credential and removes implicit auth construction. |
| modules/python/crud/azure/node_pool_crud.py | Uses shared credential helper when creating AKSClient. |
| modules/python/crud/azure/machine_crud.py | Uses shared credential helper when creating AKSMachineClient. |
| modules/python/tests/crud/test_azure_node_pool_crud.py | Updates NodePoolCRUD tests to assert credential injection via helper. |
| modules/python/tests/test_machine_crud.py | Updates MachineCRUD tests to assert credential injection via helper. |
| modules/python/tests/test_aks_machine_client.py | Updates AKSMachineClient tests to pass an injected credential. |
| modules/python/tests/clients/test_aks_client.py | Updates AKSClient tests and adds coverage that credential is required. |
Review details
Suppressed comments (1)
modules/python/clients/aks_client.py:109
operation_timeout_minutesis documented as minutes throughout the code (and passed to KubernetesClient as minutes), but the inline comment here says "seconds", which is misleading for callers.
kube_config_file: Optional[str] = os.path.expanduser("~/.kube/config"),
result_dir: Optional[str] = None,
operation_timeout_minutes: int = 10, # Timeout for each step in seconds
):
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Liyu Ma (liyu-ma)
approved these changes
Sep 2, 2026
Diamond (diamondpowell)
deleted the
dipowell/refactor-aks-client-credential
branch
September 2, 2026 13:38
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
This refactor separates Azure credential construction from
AKSClientso callers can explicitly provide the credential they need while the CRUD wrappers use one shared configuration path. It enables the workload CRUD package and pipelines to authenticate without relying on credentials created implicitly inside the client.After this merges, PR #1231 will be updated to consume the new
AKSClientcredential contract. PR #1305 must merge first.Summary
configure_credentialhelperAKSClientValidation