Refactor: improve use of scope and middle layer Spec types in services

[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, each service in https://github.com/kubernetes-sigs/cluster-api-provider-azure/tree/master/cloud/services has its own spec type defined that is used as input to create the Azure resource. For example, subnets have Spec: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/cloud/services/subnets/subnets.go#L33

// Spec input specification for Get/CreateOrUpdate/Delete calls
type Spec struct {
	Name                string
	CIDR                string
	VnetName            string
	RouteTableName      string
	SecurityGroupName   string
	Role                infrav1.SubnetRole
	InternalLBIPAddress string
}

The values in this spec get populated by the azuremachine or azurecluster _reconciler (in this case azureclustet since subnets are part of AzureCluster) like so: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/controllers/azurecluster_reconciler.go#L154

However, the Reconcile(), Get()andDelete()` functions already have access to the everything inside the spec from the service subnetSvc which contains the cluster scope: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/controllers/azurecluster_reconciler.go#L43.

To simplify the code and remove the need for extra types and translation layers (api -> spec -> azure), we should remove the superfluous spec objects and instead rely on the information that is already available in scope.AzureCluster.Spec., like we already do in some instances: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/cloud/services/subnets/subnets.go#L49

In addition, some of the values in "Spec" are set by default in azure_reconcile.go at the moment, eg: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/controllers/azurecluster_reconciler.go

We should move those to defaulting webhooks (see #427) so that the source of truth is the AzureCluster and AzureMachine spec.

/kind proposal

[CecileRobertMichon]

cc @jsturtevant

[CecileRobertMichon]

/assign

[CecileRobertMichon]

Thinking about this more, this does not work if the same Reconcile() func is used to create two different resources, for eg. https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/controllers/azurecluster_reconciler.go#L211 and https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/controllers/azuremachine_reconciler.go#L229

[alexeldeib]

Doing everything through scope makes sharing for e.g. machine/machinepool and cluster/managedcluster more painful 🙁 Can we try to pass necessary values from the reconcile loop to the services?

The services shouldn't care about what type they're reconciling against; just the Azure stuff involved.

[CecileRobertMichon]

@alexeldeib that's a very fair point. To be honest, I'm still experimenting right now and not set on a specific design because there are still some unanswered questions, the one you bring up is one of them. I think the main issue is that right now the services don't just do "reconcile an Azure resource", they also have some CAPZ specific logic in there. And they're not doing all of the CAPZ logic either (like CAPA does) because the azuremachine_reconciler and azurecluster_reconciler files have some too. One thing I was looking into doing is changing the use of scope to have an interface that explicitly defines what information is required for each service. For example, the virtualnetworks service needs to be able to get Vnet() but it shouldn't care about the rest of the stuff in ClusterScope, and it shouldn't assume that ClusterScope has ClusterScope.AzureCluster.Spec.Vnet.

I'll definitely consider "Doing everything through scope makes sharing for e.g. machine/machinepool and cluster/managedcluster more painful" as a criteria as I'm thinking about this more, thanks for bringing it up.

[alexeldeib]

Extracting the middle spec type from the scope as part of e.g. reconcile seems interesting. I think mostly it's painful right now because the services use scope liberally to grab bits and pieces of what they need. The dependencies aren't very explicit.

[CecileRobertMichon]

/close in favor of #757

[CecileRobertMichon]

/close

[k8s-ci-robot]

@CecileRobertMichon: Closing this issue.

In response to this:

/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.