Conversation
UtheMan
requested review from
FumingZhang,
jsntcy,
yanzhudd and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @UtheMan, |
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| aks create | cmd aks create added parameter custom_ca_trust_certificates |
||
| aks update | cmd aks update added parameter custom_ca_trust_certificates |
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
microsoft-github-policy-service
bot
added
Auto-Assign
|
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>
|
UtheMan
changed the title
--custom-ca-trust-certificates
There was a problem hiding this comment.
LGTM
Queued live test to validate the change, test passed!
Please fix failed style checks.
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/_consts.py:227:1: E302 expected 2 blank lines, found 1
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/_validators.py:838:1: E302 expected 2 blank lines, found 1
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/_validators.py:843:86: W292 no newline at end of file
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py:8078:1: W293 blank line contains whitespace
/mnt/vss/_work/1/s/src/azure-cli/azure/cli/command_modules/acs/managed_cluster_decorator.py:8093:18: W291 trailing whitespace
UtheMan
force-pushed
the
mikolaj/custom-ca-ga-cli
branch
from
8df7e35 to
44a0c87
Compare
|
Requeued the live test, test passed!
|
yanzhudd
changed the title
--custom-ca-trust-certificatesaz aks create/update: Add Custom CA Trust certificates option --custom-ca-trust-certificates
| "scaleSetPriority": "Regular", "scaleSetEvictionPolicy": "Delete", "spotMaxPrice": | ||
| -1.0, "nodeTaints": [], "enableEncryptionAtHost": false, "enableUltraSSD": false, | ||
| "enableFIPS": false, "name": "c000003"}], "linuxProfile": {"adminUsername": | ||
| "azureuser", "ssh": {"publicKeys": [{"keyData": "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCL1KGP0wR222Ot/MECoGTmP+Owj+jxfcN8NCbI+FvQ7lYWGmUIgYMCzbLj0+aJAfcjLDT68yM9nEW4w9mK5i99P2va1jCnrh1l5UcstSm38x210xUJE7F0zJRAc8yZ1saYBfdEKZxuwm392AukQHXVxkF2WyZty3J/26m7xYuA7UkLCTAspLCegVO7rqf1mlfANnbFSsx/mq5daLtDEsfYJP49voLhIvrtOvG1iKREtUJ6mDWYtMdj3nZFwJlpPk/8zYsi+9RBNZTiYOpHuvWuVrZ3fHBWDgJAi2A8guDDHdmMJuNXQOuMQFg1jmZI7CB1W4k2ctq22ALPoQ3+upPH |
There was a problem hiding this comment.
They may be scanned as potential secrets, so please replace them with fake ones (refer to this PR: https://github.com/Azure/azure-cli/pull/29951/files).
There was a problem hiding this comment.
Hi @yanzhudd , do you mean the SSH public key? Or the certificates from line 113?
There was a problem hiding this comment.
I modified the recording to remove the certificates, please let me know if this works @yanzhudd
There was a problem hiding this comment.
Actually this makes the CI checks fail, I reverted the commit for now. The certs passed in the input are dummy fake ones.
@FumingZhang do you maybe know how this should be handled? AKS-RP validation requires valid certificates to be sent in input so if we modify the recording that we compare everything to, it would fail correct?
There was a problem hiding this comment.
Actually you can replace the value in recording files, in replay mode, the request won't be sent to ARM/AKS RP, so it's safe to put in any value you want there.
But the problem is, the property is defined with type bytearray and in that case, the value must be base64 encoded, even though I set the literal value to testcert, the encoded value dGVzdGNlcnQ= still looks like a cred and I’m not sure it could pass the test.
See example in latest commit #972a888 in branch fuming/ca-fix-test-0325
There was a problem hiding this comment.
Is it okay to leave the test certs as is in this case? Or should they be changed? Not sure what the best way to address this is @FumingZhang / @yanzhudd
There was a problem hiding this comment.
Hi @FumingZhang / @yanzhudd / @yonzhan I used Fuming's suggestion for test data formatting, could you please take another look and let me know if this works? Thanks!
|
Please fix CI issues |
This reverts commit 2ff3744.
UtheMan
force-pushed
the
mikolaj/custom-ca-ga-cli
branch
from
1518bdd to
284e9dc
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
yanzhudd
changed the title
az aks create/update: Add Custom CA Trust certificates option --custom-ca-trust-certificatesaz aks create/update: Add parameter --custom-ca-trust-certificates for custom CA trust certificates
5 participants
Related command
Description
Adds
--custom-ca-trust-certificatesoption to enable users to pass custom CAs to their AKS nodes using GA CLITesting Guide
History Notes
[AKS]
az aks create: Add--custom-ca-trust-certificatesparameter to support custom CA trust feature[AKS]
az aks nodepool add: Add--custom-ca-trust-certificatesparameter to support custom CA trust featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.