RFE: [v1alpha4] redesign user-facing API

[CecileRobertMichon]

⚠️ Cluster API Azure maintainers can ask to turn an issue-proposal into a CAEP when necessary. This is to be expected for large changes that impact multiple components, breaking changes, or new large features.

Currently, the AzureCluster and AzureMachine specs are mostly flat. While this works as there are few configuration knobs, it is probably not the best long term as we add more features and configuration options. This is marked as [v1alpha4] because it would be a breaking change but I want to start thinking about this ahead of time so that we have a proposal ready when the time to switch to v1alpha4 comes. Ideally, we should come up with a logical way to break down the spec in AzureCluster, AzureMachine, and AzureMachinePool. For example, Storage, Compute, etc.

Open for discussion. Will bring this up at the next CAPZ office hours.

Please add any API breaking changes you'd like to see happen in v1alpha4 as comments on this issue

/kind proposal

[CecileRobertMichon]

type Subnets should be type Subnets [SubnetRole]*SubnetSpec instead of type Subnets []*SubnetSpec and we should remove the Role field in SubnetSpec.

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/api/v1alpha3/types.go#L78

This assumes there should be one and only one subnet per "role" (ie. ControlPlane, Node), which is the case today but we should determine if there would be a case for this not being true in the future.

[CecileRobertMichon]

This assumes there should be one and only one subnet per "role" (ie. ControlPlane, Node), which is the case today but we should determine if there would be a case for this not being true in the future.

I don't think that's always going to be true so it might actually be better to have a map where the key is Name :

type Subnets [string]*SubnetSpec

because Subnet name is and should be unique within a vnet, https://docs.microsoft.com/en-us/azure/virtual-network/virtual-network-manage-subnet#add-a-subnet, and that's an Azure infrastructure constraint, unlike role which is subject to change.

EDIT (01/28/21): We'll need to change this array of pointers to an array of objects to update to controller runtime 0.8.1 https://github.com/kubernetes-sigs/cluster-api/pull/4109/files#diff-50eabdf2e788bbcbe8320b5f3dc2e2595b62a1bf09c762c626b46f8dc3cdfcccR58

[CecileRobertMichon]

OSDisk. DiskSizeGB should be optional, see #658 (comment)

[CecileRobertMichon]

Remove AzureMachine FailureDomains and AvailabilityZones (in favor of Machine.FailureDomains) #679

[CecileRobertMichon]

Subnet CIDRBlock + ipv6 CIDR should become a list of CIDRBlocks to match Azure APIs (a subnet can have multiple CIDRs)
cc @jsturtevant

EDIT: done in #646

[CecileRobertMichon]

#812

[alexeldeib]

is CAPA undertaking a similar effort in v1a4? Curious if there are any common patterns emerging around BYO-resource type stuff

[CecileRobertMichon]

is CAPA undertaking a similar effort in v1a4? Curious if there are any common patterns emerging around BYO-resource type stuff

Not that I'm aware of, from what I've seen CAPA tags resources as owned by the cluster and uses that to differentiate between managed (lifecycle owned by CAPA) and unmanaged (BYO) resources. @detiber / @ncdc might have some additional context.

[CecileRobertMichon]

rename IngressRules to SecurityRules (maybe?) to be consistent with Azure naming

[CecileRobertMichon]

ResourceGroup -> ResourceGroupName ?

[CecileRobertMichon]

Consider making OS disk ManagedDisk.StorageAccountType optional as it is not actually a required field for Azure APIs.

VM sizes without "s" support both Standard HDD and Standard SSD, default is Standard HDD. VM sizes with an "s" support Premium SSD, Standard HDD and Standard SSD, defaults to Premium SSD. Some select VM sizes support Ultra SSD for data disks.

[CecileRobertMichon]

/assign

[CecileRobertMichon]

Status:

• type Subnets should be type Subnets [SubnetRole]*SubnetSpec instead of type Subnets []*SubnetSpec and we should remove the Role field in SubnetSpec. won't do
• OSDisk. DiskSizeGB should be optional
• Remove AzureMachine FailureDomains and AvailabilityZones (in favor of Machine.FailureDomains) - done in Cleanup deprecated variables from v1alpha4 #1233
• Subnet CIDRBlock + ipv6 CIDR should become a list of CIDRBlocks to match Azure APIs (a subnet can have multiple CIDRs) - done in ✨ Add single stack IPv6 support #646
• Rename IngressRules to SecurityRules (maybe?) to be consistent with Azure naming - done in Add support for custom egress rules #1299
• ResourceGroup -> ResourceGroupName ? won't do
• making OS disk ManagedDisk.StorageAccountType optional as it is not actually a required field for Azure APIs - done in Make OSDisk Storage Type optional #1321

[nader-ziada]

OSDisk. DiskSizeGB should be optional done in #1398

[CecileRobertMichon]

I think we're good to close this. I don't think the AzureResourceGroup rename is truly needed.

/close

[k8s-ci-robot]

@CecileRobertMichon: Closing this issue.

In response to this:

I think we're good to close this. I don't think the AzureResourceGroup rename is truly needed.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.