Refactor: Services should not return interfaces

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

Detailed Description

Follow up on #482 (comment)

We should be returning SDK types for Azure resources in services/ instead of interface{}.

Also, we should remove unused service mocks (we mock the Azure Clients and those are the ones used in tests):

• remove unused https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/cloud/fake_interfaces.go
• remove unused https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/cloud/mocks/service_mock.go#L31

related to #110

Contract changes [optional]

[Describe contract changes between Cluster API Azure Provider controllers, if applicable.]

Data model changes [optional]

[Describe contract changes between Cluster API Azure Provider controllers, if applicable.]

/kind proposal

[CecileRobertMichon]

@juan-lee are you still interested in helping with this?

[CecileRobertMichon]

cc @alexeldeib @justaugustus @devigned

[devigned]

I'd caution from returning SDK types from the services. Having dependencies on AzSDK for Go types beyond the service layer implies a tighter coupling to the SDK.

Perhaps, we should consider the services layer returns a spec + status type (maybe not a runtime.Object, but something with all the info needed by callers).

[CecileRobertMichon]

What if the Get() functions were never called from outside services? The way I see this working is you would only have Ensure()/Reconcile() and Delete() be public and be called from the controllers (see #110). Reconcile and Delete should both only return error, the SDK type return would only be for intermediate Gets.

[devigned]

What do you think about Get() being get()?

[CecileRobertMichon]

@devigned I almost wrote get() but I realized that might not work if you need to get another resource type since each service is in its own package right now, eg. you need to get the subnet ID from the network interface package... https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/cloud/services/networkinterfaces/networkinterfaces.go#L71

Ways to go around this off the top of my head:

• put all the Azure resource types in one services package (or maybe azure package)
• have public functions just to get the ID or whatever info we need, so GetSubnetID() would return a string and call get() which would return the network.subnet.

[CecileRobertMichon]

/assign

[CecileRobertMichon]

/close

in favor of #757

[k8s-ci-robot]

@CecileRobertMichon: Closing this issue.

In response to this:

/close

in favor of #757

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.