- Notifications
You must be signed in to change notification settings - Fork3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[AKS]az aks create/update: Add parameter--custom-ca-trust-certificates for custom CA trust certificates#31107
Conversation
️✔️AzureCLI-FullTest
|
Hi@UtheMan, |
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| aks create | cmdaks create added parametercustom_ca_trust_certificates | ||
| aks update | cmdaks update added parametercustom_ca_trust_certificates |
Thank you for your contribution! We will review the pull request and get back to you soon. |
The git hooks are available forazure-cli andazure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (forazure-cli) or main branch (forazure-cli-extensions). pip install azdev --upgradeazdev setup -c<your azure-cli repo path> -r<your azure-cli-extensions repo path> |
--custom-ca-trust-certificatesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
Queuedlive 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
8df7e35 to44a0c87CompareRequeued thelive test, test passed!
|
--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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hi@yanzhudd , do you mean the SSH public key? Or the certificates from line 113?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I modified the recording to remove the certificates, please let me know if this works@yanzhudd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 totestcert, the encoded valuedGVzdGNlcnQ= still looks like a cred and I’m not sure it could pass the test.
See example in latest commit#972a888 in branchfuming/ca-fix-test-0325
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 commit2ff3744.
1518bdd to284e9dcCompare/azp run |
| Azure Pipelines successfully started running 3 pipeline(s). |
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
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 inSubmitting Pull Requests.
I adhere to theCommand Guidelines.
I adhere to theError Handling Guidelines.