Conversation
UtheMan
requested review from
FumingZhang,
jsntcy,
yanzhudd and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to the Azure CLI's az aks update command to allow complete removal of custom CA trust certificates by passing an empty file to the custom-ca-trust-certificates parameter.
- Modified the update logic to handle empty certificate files for removal
- Added comprehensive test coverage for the new removal functionality
- Updated both unit tests and integration tests to verify the behavior
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| managed_cluster_decorator.py | Updated the certificate update logic to allow setting empty certificates when parameter is explicitly provided |
| test_managed_cluster_decorator.py | Added unit test to verify removal of certificates using empty file |
| test_aks_commands.py | Added integration test to verify end-to-end certificate removal functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py
Show resolved
Hide resolved
microsoft-github-policy-service
bot
added
Auto-Assign
|
Please fix CI issues |
FumingZhang
changed the title
custom-ca-trust-certificates field by passing an empty file az aks update: Add option to remove certificates passed to custom-ca-trust-certificates field by passing an empty file
FumingZhang
changed the title
az aks update: Add option to remove certificates passed to custom-ca-trust-certificates field by passing an empty file az aks update: Add support to remove existing certificates by setting the value of --custom-ca-trust-certificates to an empty file
| if self.context.raw_param.get("custom_ca_trust_certificates") is not None: | ||
| ca_certs = self.context.get_custom_ca_trust_certificates() |
There was a problem hiding this comment.
I'm not sure if it's a good idea to allow this option to accept an empty string as a value, and that would also remove the existing certificates. But there might be a chance user is doing so by mistake (compared to provided an emtpy file)
There was a problem hiding this comment.
Modified this to block when empty string is passed
|
|
||
| @AllowLargeResponse() | ||
| @AKSCustomResourceGroupPreparer(random_name_length=17, name_prefix='clitest', location='westus2') | ||
| def test_aks_update_remove_custom_ca_trust_certificates(self, resource_group, resource_group_location): |
There was a problem hiding this comment.
the live test failed with following error
E azure.cli.testsdk.exceptions.JMESPathCheckAssertionError: Query 'securityProfile.customCaTrustCertificates' doesn't yield expected value '['testcert', 'testcert']', instead the actual value is '['-----BEGIN CERTIFICATE-----\nxxxx\n-----END CERTIFICATE-----']'. Data:
There was a problem hiding this comment.
you can mark the test case as @live_only(), so it won't run in replay mode
There was a problem hiding this comment.
Modified the recording files instead to allow replay tests to pass.
UtheMan
force-pushed
the
mikolaj/remove-ca
branch
from
ceb43d2 to
3e2b627
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
UtheMan
force-pushed
the
mikolaj/remove-ca
branch
from
db0a73b to
64958e6
Compare
5 participants
Related command
az aks update
Description
Customers can now use CLI to completely remove certificates passed using
custom-ca-trust-certificatesfield.Testing Guide
azdev test --live acs.test_aks_update_remove_custom_ca_trust_certificates
History Notes
N/A