[AKS] Add arguments --asg-ids and --allowed-host-ports for az aks create | az aks nodepool add | az aks nodepool update
#27900
Conversation
clnv
requested review from
andyliuliming,
jsntcy,
yanzhudd and
zhoxing-ms
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @lodrem, |
❌AzureCLI-BreakingChangeTest
|
microsoft-github-policy-service
bot
requested review from
FumingZhang and
yonzhan
|
AKS |
microsoft-github-policy-service
bot
added
Auto-Assign
| if asg_ids or allowed_host_ports: | ||
| agentpool.network_profile = self.models.AgentPoolNetworkProfile() | ||
| if asg_ids is not None: | ||
| agentpool.network_profile.application_security_groups = asg_ids | ||
| if allowed_host_ports is not None: | ||
| agentpool.network_profile.allowed_host_ports = allowed_host_ports |
There was a problem hiding this comment.
Does this profile support partial update? For example, for an existing nodepool network profile, both application_security_groups and allowed_host_ports have been configured. Now the user only specifies application_security_groups when updating. Then the request body will be application_security_groups=new value, allowed_host_ports=null. Is this a valid scenario and the behavior expected?
There was a problem hiding this comment.
Does this profile support partial update?
No. If this feature is turned on, you’ve got to set both values when updating the network profile. You can change ASGs from your own to a managed one if you don’t specify ‘–asg-ids’, and also the other way round.
There was a problem hiding this comment.
Queued live test to validate the change, test passed!
|
Please refer to this guideline Submitting Pull Requests to update the PR description |
Updated. PTAL thanks |
| - name: --nodepool-allowed-host-ports | ||
| type: string | ||
| short-summary: Expose host ports on the node pool. When specified, format should be a comma-separated list of ranges with protocol, eg. 80/TCP,443/TCP,4000-5000/TCP. |
There was a problem hiding this comment.
If the input is a list, it is a better way to define the parameter with nargs='+' and it is not necessary to manually separate the string. After defining with that, the input should be a space-separated list.
| asg_ids = self.context.get_asg_ids() | ||
| allowed_host_ports = self.context.get_allowed_host_ports() | ||
| if asg_ids or allowed_host_ports: | ||
| agentpool.network_profile = self.models.AgentPoolNetworkProfile() |
There was a problem hiding this comment.
This could overwrite existing fields in network_profile by creating a new one. I will fix it in a following PR.
|
It is suggested to specify the related command in the header of the history notes, e.g., The other two history notes are the same like this. |
|
@yanzhudd updated. |
| '--nodepool-asg-ids={asg1_id} ' | ||
| '--nodepool-asg-ids={asg2_id} ' |
There was a problem hiding this comment.
This is not a correct usage, arg parse would only take the latter option/value pair
| - name: --nodepool-allowed-host-ports | ||
| type: string | ||
| short-summary: Expose host ports on the node pool. When specified, format should be a comma-separated list of ranges with protocol, eg. 80/TCP,443/TCP,4000-5000/TCP. | ||
| short-summary: Expose host ports on the node pool. When specified, format should be a space-separated list of ranges with protocol, eg. 80/TCP,443/TCP,4000-5000/TCP. |
There was a problem hiding this comment.
If this should be a space-separated list, the example should be 80/TCP 443/TCP 4000-5000/TCP instead of 80/TCP,443/TCP,4000-5000/TCP.
There was a problem hiding this comment.
And this is a behavior change compared to aks-preview, is this accepted by PM?
…s create` | `az aks nodepool add` | `az aks nodepool update` (Azure#27900)
6 participants
Related command
Description
Preview PR: Azure/azure-cli-extensions#5467
Testing Guide
Create cluster with managed ASG:
az aks create \ --name ${CLUSTER_NAME} \ --nodepool-allowed-host-ports 80/tcp,53/udp,4000-5000/tcpCreate cluster with custom ASGs:
Add nodepool with custom ASGs:
Add nodepool with managed ASGs:
az aks nodepool add \ --name ${AGENTPOOL_NAME} \ --allowed-host-ports 80/tcp,53/udp,4000-5000/tcpUpdate nodepool ASG and allowed host ports:
History Notes
az aks nodepool add: Add NSG Control arguments--asg-idsand--allowed-host-portsaz aks nodepool update: Add NSG Control arguments--asg-idsand--allowed-host-portsaz aks create: Add NSG Control arguments--nodepool-asg-idsand--nodepool-allowed-host-portsThis 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.