Multitenancy for managed clusters

[shysank]

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR implements multitenancy for azure managed clusters largely building upon the work done in #977

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1153

Special notes for your reviewer:
A couple of things to note:

• NetworkInfrastructureReadyCondition not implemented. There are no conditions right now for managed clusters. So, will create a followup issue for this
• There is a separate AzureIdentityReconciler for reconciling AzureIdentity and AzureIdentityBinding in exp/controllers. which does exactly the same thing as the one already there in controllers with one difference being it only deletes identities created by managed clusters. This should eventually merged into a single controller once managed clusters are promoted from experimental.

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:

Multitenancy for managed clusters.

[shysank]

/retest

[nader-ziada]

is it possible to have an e2e test for this?

[shysank]

is it possible to have an e2e test for this?

Yes, I'll work on adding it.

[nader-ziada]

I think this is good to go
@devigned do you have any thoughts?

[devigned]

lgtm

The issue with the repeated identity controller should either be refactored here or in #1389.

[CecileRobertMichon]

The issue with the repeated identity controller should either be refactored here or in #1389

I don't intend to refactor exp/ controllers or dir structure in #1389, IMO we should tackle it here. The identity feature is not experimental so it shouldn't live in exp/.

[shysank]

The issue with the repeated identity controller should either be refactored here or in #1389

I don't intend to refactor exp/ controllers or dir structure in #1389, IMO we should tackle it here. The identity feature is not experimental so it shouldn't leave in exp/.

Two solutions I can think of:

1. Reconcile identities created by amcp along with azure cluster. This is what Nader and I discussed; we didn't want to import exp api types inside the main controllers package.
2. Ignore identities created by amcp, and just make sure they don't get deleted. A hacky solution until we remove it from exp, lots of dangling identities will be there. There may also be issues when we create a new amcp with the same in future.

Both of these are not ideal solutions. The main problem I see is, although the identity feature is not experimental, in this case it's tightly coupled with the exp api types which makes it difficult to move it out of exp. wdyt?

[nader-ziada]

The issue with the repeated identity controller should either be refactored here or in #1389

I don't intend to refactor exp/ controllers or dir structure in #1389, IMO we should tackle it here. The identity feature is not experimental so it shouldn't leave in exp/.

Two solutions I can think of:

1. Reconcile identities created by amcp along with azure cluster. This is what Nader and I discussed; we didn't want to import exp api types inside the main controllers package.
2. Ignore identities created by amcp, and just make sure they don't get deleted. A hacky solution until we remove it from exp, lots of dangling identities will be there. There may also be issues when we create a new amcp with the same in future.

Both of these are not ideal solutions. The main problem I see is, although the identity feature is not experimental, in this case it's tightly coupled with the exp api types which makes it difficult to move it out of exp. wdyt?

The identity controller in the exp folder is just to deal with identities related to managedcontrolplane clusters, I don't understand why it is an issue to keep it there. If the issue is about duplication of code we can try to refactor some of the code in a util package and use it in both, but I still think they should be two controllers

[CecileRobertMichon]

Reconcile identities created by amcp along with azure cluster. This is what Nader and I discussed; we didn't want to import exp api types inside the main controllers package.

Why is importing exp types in the main controller a bad thing? As long as it's all behind feature gates that should be fine and, IMO, cleaner than duplicating code that might diverge in the future. We already do this in CAPI, eg:
https://github.com/kubernetes-sigs/cluster-api/blob/master/controllers/cluster_controller.go#L365-L373

[nader-ziada]

Reconcile identities created by amcp along with azure cluster. This is what Nader and I discussed; we didn't want to import exp api types inside the main controllers package.

Why is importing exp types in the main controller a bad thing? As long as it's all behind feature gates that should be fine and, IMO, cleaner than duplicating code that might diverge in the future. We already do this in CAPI, eg:
https://github.com/kubernetes-sigs/cluster-api/blob/master/controllers/cluster_controller.go#L365-L373

I didn't think this was allowed, in that case, my bad for asking Shyam to not do that and have a 2nd controller, we can just combine them then

[shysank]

Thanks @CecileRobertMichon and @nader-ziada for the feedback. I'll update the existing identity_controller to handle aks identities, and remove the new one.

[alexeldeib]

this looks fairly good to go? anything you're still working on?

[shysank]

this looks fairly good to go? anything you're still working on?

Nope, I think we can this one in and create a follow up issue for more e2e scenarios.

[alexeldeib]

/lgtm

nice work!

[CecileRobertMichon]

/lgtm
/assign @nader-ziada

[nader-ziada]

👏👏

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nader-ziada

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 [nader-ziada]

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

[nader-ziada]

@shysank you are going to have to rebase so we can get this in

[shysank]

/test pull-cluster-api-provider-azure-e2e

[k8s-ci-robot]

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

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	5a6a864	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]

/test pull-cluster-api-provider-azure-e2e

[nader-ziada]

/lgtm