Skip to content

[Compute] BREAKING CHANGE: Fix #10728: az vm create: create subnet automatically if vnet is specified and subnet not exists #12115

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arrownj merged 6 commits into from
Feb 13, 2020
Merged

[Compute] BREAKING CHANGE: Fix #10728: az vm create: create subnet automatically if vnet is specified and subnet not exists #12115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a2e20e4
[Compute] Fix #10728: `az vm create`: create subnet automatically if …
arrownj Feb 10, 2020
f30efac
{Compute} update test_vm_commands to fix merge conflict
arrownj Feb 10, 2020
1fd5ce3
{Compute} fix test recording file for profile hybrid_2018_03_01 & hyb…
arrownj Feb 10, 2020
dae7dff
{Compute} update subnet help message & fail to create error message
arrownj Feb 12, 2020
f413924
{Compute} update fail to create subnet error message
arrownj Feb 12, 2020
e1db954
{Compute} update test recording due to merge conflict
arrownj Feb 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/azure-cli/azure/cli/command_modules/vm/_params.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ def load_arguments(self, _):
with self.argument_context(scope, arg_group='Network') as c:
c.argument('vnet_name', help='Name of the virtual network when creating a new one or referencing an existing one.')
c.argument('vnet_address_prefix', help='The IP address prefix to use when creating a new VNet in CIDR format.')
c.argument('subnet', help='The name of the subnet when creating a new VNet or referencing an existing one. Can also reference an existing subnet by ID. If omitted, an appropriate VNet and subnet will be selected automatically, or a new one will be created.')
c.argument('subnet', help='The name of the subnet when creating a new VNet or referencing an existing one. Can also reference an existing subnet by ID. If both vnet-name and subnet are omitted, an appropriate VNet and subnet will be selected automatically, or a new one will be created.')
c.argument('subnet_address_prefix', help='The subnet IP address prefix to use when creating a new VNet in CIDR format.')
c.argument('nics', nargs='+', help='Names or IDs of existing NICs to attach to the VM. The first NIC will be designated as primary. If omitted, a new NIC will be created. If an existing NIC is specified, do not specify subnet, VNet, public IP or NSG.')
c.argument('private_ip_address', help='Static private IP address (e.g. 10.0.0.5).')
Expand Down
34 changes: 30 additions & 4 deletions src/azure-cli/azure/cli/command_modules/vm/custom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,11 +660,37 @@ def create_vm(cmd, vm_name, resource_group_name, image=None, size='Standard_DS1_

nic_dependencies = []
if vnet_type == 'new':
vnet_name = vnet_name or '{}VNET'.format(vm_name)
subnet = subnet or '{}Subnet'.format(vm_name)
nic_dependencies.append('Microsoft.Network/virtualNetworks/{}'.format(vnet_name))
master_template.add_resource(build_vnet_resource(
cmd, vnet_name, location, tags, vnet_address_prefix, subnet, subnet_address_prefix))
vnet_exists = False
if vnet_name:
from azure.cli.command_modules.vm._vm_utils import check_existence
vnet_exists = \
check_existence(cmd.cli_ctx, vnet_name, resource_group_name, 'Microsoft.Network', 'virtualNetworks')
if vnet_exists:
from azure.cli.core.commands import cached_get, cached_put, upsert_to_collection
from azure.cli.command_modules.vm._validators import get_network_client
client = get_network_client(cmd.cli_ctx).virtual_networks
vnet = cached_get(cmd, client.get, resource_group_name, vnet_name)

Subnet = cmd.get_models('Subnet', resource_type=ResourceType.MGMT_NETWORK)
subnet_obj = Subnet(
name=subnet,
address_prefixes=[subnet_address_prefix],
Copy link
Member

@qwordy qwordy Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default value: subnet_address_prefix='10.0.0.0/24'
It may be incompatible with vnet. You can deduce a valid value from vnet.
I request a change here.

address_prefix=subnet_address_prefix
)
Comment on lines +676 to +680
Copy link
Member

@jiasli jiasli Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a subnet is not as simple as adding a new resource. We need to take the vnet's address space and subnet's prefix into consideration.

It is very hard to deduce the new subnet's prefix based on the existing vnet's address space and existing subnet's prefix.

This PR will always try to add subnet 10.0.0.0/24 to the vnet. It'll succeed only if 10.0.0.0/24 is valid within the vnet, which is very unlikely in most cases. In other cases:

  1. If the existing vnet 10.0.0.0/16 already has subnet 10.0.0.0/24, the creation will fail because the space is already occupied

  2. If the existing vnet is 10.3.0.0/16, the creation will fail because the prefix is not in the allowed address space:

    Azure Error: NetcfgInvalidSubnet
    Message: Subnet 'vm3Subnet' is not valid in virtual network 'vnet3'.

  3. If the existing vnet's address space is the same as the existing subnet's prefix - both 10.0.0.0/16, the subnet creation will fail in any case, because there is no space left

  4. If the existing vnet's address space is 10.0.0.0/24, how do we decide the mask of the subnet? 24, 25? It is not practical to do the decision for the user

My suggestion: If --vnet-name and --subnet are neither specified, az create will detect if there is already a vnet and subnet in the resource group and put the vm there. We can apply the same when only --vnet-name is specified with an existing vnet - if there is a subnet, put the vm there; if not, error out, instead of calculating the available prefix and creating a subnet voluntarily.

cc @myronfanqiu

Copy link
Contributor Author

@arrownj arrownj Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we discussed the cases you mentioned above before. Previously, there are some problems with the vnet/subnet prefix. However, for this PR, we don't want to change these behaviors, just keep them as before.

What we changed in this PR this that, when vnet is specified and exists, no matter subnet is specified or not, if the subnet not exists, create it without removing other existing subnets.

As the cases above, if the user not specify subnet prefix, it will use a default value (10.0.0.0/24) which may be wrong, however this is not the problem we want to solve in this PR.

Copy link
Member

@yungezz yungezz Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to share some context, not related with this PR. There's default vnet/subnet azure CLI created when creating VM. This is convenient scenario CLI providing: minimum effort to get started on a VM.

Copy link
Member

@qwordy qwordy Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a subnet is not as simple as adding a new resource. We need to take the vnet's address space and subnet's prefix into consideration.

It is very hard to deduce the new subnet's prefix based on the existing vnet's address space and existing subnet's prefix.

This PR will always try to add subnet 10.0.0.0/24 to the vnet. It'll succeed only if 10.0.0.0/24 is valid within the vnet, which is very unlikely in most cases. In other cases:

1. If the existing vnet `10.0.0.0/16` already has subnet `10.0.0.0/24`, the creation will fail because the space is already occupied

2. If the existing vnet is `10.3.0.0/16`, the creation will fail because the prefix is not in the allowed address space:
   > Azure Error: NetcfgInvalidSubnet
   > Message: Subnet 'vm3Subnet' is not valid in virtual network 'vnet3'.

3. If the existing vnet's address space is the same as the existing subnet's prefix - both `10.0.0.0/16`, the subnet creation will fail in any case, because there is no space left

4. If the existing vnet's address space is `10.0.0.0/24`, how do we decide the mask of the subnet? 24, 25? It is not practical to do the decision for the user

My suggestion: If --vnet-name and --subnet are neither specified, az create will detect if there is already a vnet and subnet in the resource group and put the vm there. We can apply the same when only --vnet-name is specified with an existing vnet - if there is a subnet, put the vm there; if not, error out, instead of calculating the available prefix and creating a subnet voluntarily.

cc @myronfanqiu

1, retry 25, 26..., or 23, 22...
2, we can create subnet with same address of vnet
3. retry 25, 26..., or 23, 22...
4, Azure CLI always makes decision for users so that users can use a very simple command to create resources.

I agree it's complex and tricky. Users might be astonished when a strange subnet is created.

upsert_to_collection(vnet, 'subnets', subnet_obj, 'name')
try:
cached_put(cmd, client.create_or_update, vnet, resource_group_name, vnet_name).result()
except Exception:
raise CLIError('Subnet({}) does not exist, but failed to create a new subnet with address '
'prefix {}. It may be caused by name or address prefix conflict. Please specify '
'an appropriate subnet name with --subnet or a valid address prefix value with '
'--subnet-address-prefix.'.format(subnet, subnet_address_prefix))
if not vnet_exists:
vnet_name = vnet_name or '{}VNET'.format(vm_name)
nic_dependencies.append('Microsoft.Network/virtualNetworks/{}'.format(vnet_name))
master_template.add_resource(build_vnet_resource(
cmd, vnet_name, location, tags, vnet_address_prefix, subnet, subnet_address_prefix))

if nsg_type == 'new':
nsg_rule_type = 'rdp' if os_type.lower() == 'windows' else 'ssh'
Expand Down
Loading