💎 Refactor public IP service to get Spec from scope

[CecileRobertMichon]

What this PR does / why we need it: Following up from #713, this is a proof of concept with the Public IPs service to stop using a mix of spec + scope in services as input for Reconcile() and Delete().

Before:

type OldService interface {
	Reconcile(ctx context.Context, spec interface{}) error
	Delete(ctx context.Context, spec interface{}) error
}

https://github.com/CecileRobertMichon/cluster-api-provider-azure/blob/06d0321be5fefd840ea1742b9c16a7ce72b98378/cloud/interfaces.go#L34-L37

After:

type Service interface {
	Reconcile(ctx context.Context) error
	Delete(ctx context.Context) error
}

https://github.com/CecileRobertMichon/cluster-api-provider-azure/blob/06d0321be5fefd840ea1742b9c16a7ce72b98378/cloud/interfaces.go#L27-L30

Instead of taking cluster scope or machine scope types as input, each service will have its own scope interface that explicitly defines what information getters the service needs that the passed in scope (either AzureCluster or AzureMachine) needs to implement: https://github.com/CecileRobertMichon/cluster-api-provider-azure/blob/06d0321be5fefd840ea1742b9c16a7ce72b98378/cloud/services/publicips/service.go#L22-L25

This only changes the Pubic IP service while keep all other services intact (renamed the interface to OldService). The idea is to facilitate review and focus on the concept rather than on each service's details. Once I get reviewer consensus, I will proceed to refactor all the other services by opening a PR for each one. This following PRs will be smaller as this one does most of the heavy lifting in machine scope and cluster scope.

/hold
/cc @devigned @alexeldeib @justaugustus @nader-ziada
Would love to get your thoughts on this approach.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Partially addresses #110 , #627 and #609

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

Refactor public IP service to get Spec from scope
Update mockgen to 8a3d595

[nader-ziada]

I like the approach in general 👍 but was a little confused about a couple of things when I tried to follow the code flow

[CecileRobertMichon]

@nader-ziada

Instead of taking cluster scope or machine scope types as input, each service will have its own scope interface that explicitly defines what information getters the service needs that the passed in scope (either AzureCluster or AzureMachine) needs to implement: https://github.com/CecileRobertMichon/cluster-api-provider-azure/blob/06d0321be5fefd840ea1742b9c16a7ce72b98378/cloud/services/publicips/service.go#L22-L25

Basically, PublicIPScope is a more precise, targetted scope interface, that defines what info the public IP service needs. Both ClusterScope and MachineScope (and maybe MachinePool in the future once it supports public IPs per instance) implement PublicIPScope. This is to allow reusing the same public IP reconcile service for different public IPs, eg. the Public LB public IP and each VM's IP when allocatePublicIP is set to true. This also makes sure that the passed in "scope" has all the required getter funcs implemented rather that making assumptions on the content of cluster scope or machine scope. Does that make sense?

[nader-ziada]

@CecileRobertMichon makes sense, I get it now 👍

[devigned]

I like the inversion of control from the call site of the reconcile function to the reconciler being dependent on the data in the scope. It feels like this is more in the spirt of a reconciliation loop.

[nader-ziada]

sorry for all the conflicts :(

[CecileRobertMichon]

sorry for all the conflicts :(

No worries!! I knew what I was getting into 😄

[CecileRobertMichon]

@alexeldeib would love your thoughts on this approach when you have a second. I think this achieves what we discussed in #627:

• makes sharing for e.g. machine/machinepool and cluster/managedcluster possible
• The services shouldn't care about what type they're reconciling against; just the Azure stuff involved.
• The dependencies on scope are explicit.

[k8s-ci-robot]

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	a89eb3c	link	/test pull-cluster-api-provider-azure-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[nader-ziada]

/lgtm

[alexeldeib]

/lgtm

definitely happy with inverting control and slimming down scope per service 👍

[CecileRobertMichon]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment