search

kubernetes-sigs / cluster-api-provider-azure

Cluster API implementation for Microsoft Azure
https://capz.sigs.k8s.io/
Apache License 2.0
291 stars 421 forks source link

RFE: [v1alpha4] redesign user-facing API #618

Closed CecileRobertMichon closed 3 years ago

CecileRobertMichon commented 4 years ago

⚠️ 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

OSDisk. DiskSizeGB should be optional, see https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/658#discussion_r433405993

CecileRobertMichon commented 4 years ago

Remove AzureMachine FailureDomains and AvailabilityZones (in favor of Machine.FailureDomains) https://github.com/kubernetes-sigs/cluster-api-provider-azure/issues/679

CecileRobertMichon commented 4 years ago

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 commented 4 years ago

https://github.com/kubernetes-sigs/cluster-api-provider-azure/issues/812

alexeldeib commented 4 years ago

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

CecileRobertMichon commented 4 years ago

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 commented 4 years ago

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

CecileRobertMichon commented 3 years ago

ResourceGroup -> ResourceGroupName ?

CecileRobertMichon commented 3 years ago

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 commented 3 years ago

/assign

CecileRobertMichon commented 3 years ago

Status:

nader-ziada commented 3 years ago

OSDisk. DiskSizeGB should be optional done in #1398

CecileRobertMichon commented 3 years ago

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

/close

k8s-ci-robot commented 3 years ago

@CecileRobertMichon: Closing this issue.

In response to [this](https://github.com/kubernetes-sigs/cluster-api-provider-azure/issues/618#issuecomment-854052648): >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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.