[AKS] az aks install-desktop: Add AKS Desktop installation - #34100
Tom Gamble (gambtho) wants to merge 3 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Critical archive traversal and other unresolved installation and validation issues must be addressed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds cross-platform az aks install-desktop support with secure release downloads, integrity verification, native installation, and Linux archive handling.
Changes:
- Registers the command,
--version, and help examples. - Implements platform/release selection, downloads, verification, extraction, and installation.
- Adds mocked workflow tests.
File summaries
| File | Summary and findings |
|---|---|
src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py |
Adds installer tests. Nit (1 vote): cleanup assertion is vacuous because the mock does not create the downloaded path. |
src/azure-cli/azure/cli/command_modules/acs/custom.py |
Implements the installation workflow. Critical (1 vote): archive extraction remains vulnerable to traversal through symlink members. Moderate (3 votes): temporary DMG/DEB files may be deleted before desktop handlers finish consuming them. Moderate (1 vote): null digests raise TypeError instead of ClientRequestError. Moderate (1 vote): an empty --version bypasses validation and installs the latest release. |
src/azure-cli/azure/cli/command_modules/acs/commands.py |
Registers the new command. |
src/azure-cli/azure/cli/command_modules/acs/_params.py |
Adds the --version parameter. |
src/azure-cli/azure/cli/command_modules/acs/_help.py |
Adds command help and examples. |
Review details
Suppressed comments (3)
src/azure-cli/azure/cli/command_modules/acs/custom.py:2504
- When the GitHub asset payload contains an explicit
"digest": null,asset.getreturnsNoneandre.fullmatchraisesTypeErrorinstead of the intendedClientRequestError. Treat non-string values as an invalid digest so malformed or older release metadata fails cleanly.
if not re.fullmatch(r'sha256:[0-9a-fA-F]{64}', digest):
src/azure-cli/azure/cli/command_modules/acs/custom.py:2422
- Because this guard is truthiness-based,
az aks install-desktop --version ""skips semantic-version validation and silently falls through to/latest, installing a different version than the user supplied. Use an explicitis not Nonecheck so every supplied value, including an empty string, is rejected.
if version:
src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_custom.py:1052
- This cleanup assertion is vacuous:
mock_downloadis a mock and never creates the path, soexistsis already false even ifTemporaryDirectoryfails to remove a downloaded artifact. Make the mock createdestinationbefore returning, then assert that the path is absent after the workflow.
self.assertFalse(os.path.exists(mock_download.call_args[0][1]))
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
Tom Gamble (gambtho)
changed the title
az aks install-desktop: Add AKS Desktop installation
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
|
|
||
| logger.warning('Getting AKS Desktop release metadata from "%s".', release_url) | ||
| try: | ||
| release = json.loads(_urlopen_read(release_url)) |
There was a problem hiding this comment.
[P2] Make the GitHub metadata request recover from rate limiting. This call is unauthenticated, so it gets GitHub's shared 60-requests/hour quota; moreover, the error's recommendation to pass --version cannot recover because that path simply calls another unauthenticated REST endpoint (/releases/tags/...) and receives the same 403/429. This module's adjacent kubelogin flow already recognizes this failure mode and offers authentication/fallback. Please provide an authenticated or non-API metadata path here and make the recommendation actually bypass the exhausted quota.
| if getattr(tarfile, 'data_filter', None) is None: | ||
| raise FileOperationError( | ||
| 'Safe AKS Desktop archive extraction requires an updated Python with tarfile.data_filter support.') |
There was a problem hiding this comment.
[P2] Preserve archive installs on all supported Python runtimes. Azure CLI declares python_requires='>=3.10.0', but tarfile.data_filter is absent from upstream CPython 3.10.0-3.10.11 and 3.11.0-3.11.3. On those valid runtimes every archive-only target (Linux ARM, non-Debian x64, or x64 without xdg-open) deterministically fails before extraction. Please use an equivalent safe fallback on pre-backport runtimes (or otherwise align the command/runtime support contract) rather than making the new command unusable for that supported subset.
FumingZhang
left a comment
FumingZhang
left a comment
There was a problem hiding this comment.
please consider adding a scenario test case to help validate the change
🤖 PR Validation —⚠️ Review suggested
Summary
az aks install-desktopwith latest-stable installation by default and optional--versionValidation
test_customsuite: 80 tests run, 1 skipped, no failures.xdg-openhandoff with an isolated harmless handler confirms the retained installer remains readable after the command returns.git diff --checkpass.az aks install-desktop -hrenders the arguments, examples and installer-retention guidance./polishand CodeRabbit reviews completed; review findings were addressed.