adding multi tenancy for controller using aad-pod-identity #977

nader-ziada · 2020-10-05T21:20:16Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

adding azure principal and identity provider types for multi-tenancy

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 #586 , #778

Special notes for your reviewer:

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

work in progress

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

support for controller manager multi-tenancy

k8s-ci-robot · 2020-10-05T21:20:34Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

nader-ziada · 2020-10-05T21:20:57Z

@devigned I thought I would push this while I'm working on it still so you can take a look and let me know what you think of the direction

devigned

Per our conversation in Slack: https://kubernetes.slack.com/archives/CEX9HENG7/p1602089427042600. I believe we are able to greatly simplify the design after talking with the AAD pod identity team. This is a great start down the multi-tenancy path.

The gist of the linked conversation:

We should run aad pod identity within the capz namespace in namespace constrained mode. We should allow users to create AzureIdentities within the workload cluster namespace and have the identity owned by the AzureCluster. The capz controller would be responsible for copying the cluster namespace identity into the controller namespace and creating a AzureIdentityBinding in the controller namespace to link the label of the capz controller and the capz namespaced copy of the cluster AzureIdentity. The capz controller would then be responsible for deleting the identities within the capz namespace when they are no longer needed.

cloud/scope/clients.go

api/v1alpha3/azurecluster_types.go

api/v1alpha3/azureserviceprincipal_types.go

cloud/scope/principal.go

config/manager/manager_image_patch.yaml

devigned · 2020-10-07T18:32:43Z

Probably goes without saying, but we will also need to deploy aad pod identity components with the capz components.

nader-ziada · 2020-10-07T19:43:24Z

Probably goes without saying, but we will also need to deploy aad pod identity components with the capz components.

yes, will add the deployment for it, was still trying to get it to work

Makefile

cloud/scope/identity.go

Makefile

nader-ziada · 2020-12-11T14:11:20Z

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

CecileRobertMichon · 2020-12-11T21:48:55Z

api/v1alpha3/azureclusteridentity_types.go

+}
+
+// ClusterNamespaceAllowed indicates if the cluster namespace is allowed
+func (c *AzureClusterIdentity) ClusterNamespaceAllowed(namespace string) bool {


let's add a unit test for this

api/v1alpha3/azureclusteridentity_types.go

cloud/scope/identity.go

controllers/azureidentity_controller.go

util/identity/defaults.go

CecileRobertMichon · 2020-12-11T22:10:45Z

test/e2e/azure_test.go

@@ -350,4 +351,60 @@ var _ = Describe("Workload cluster creation", func() {
 			})
 		})
 	})
+
+	Context("Creating a cluster using a different SP identity", func() {
+		BeforeEach(func() {


Same comment I wrote in the Windows PR - we need to audit all the clusters that get created in pr e2e tests and see if any of them can be safely combined to avoid growing the cost and duration of each PR run too much

I think it makes sense for this test to have its own cluster since it using a different identity which we would want to test separately, wdyt?

CecileRobertMichon · 2020-12-11T22:13:04Z

docs/book/src/topics/multitenancy.md

+type: Opaque
+data:
+  certificate: CERTIFICATE
+  password: PASSWORD


this is unclear to me? is it one or the other?

both kinds are supported by aad-pod-identity https://github.com/Azure/aad-pod-identity/tree/master/deploy/demo

docs/book/src/topics/multitenancy.md

CecileRobertMichon · 2020-12-11T22:14:14Z

controllers/azureidentity_controller.go

+		return ctrl.Result{}, err
+	}
+
+	bindingsToDelete := []aadpodv1.AzureIdentityBinding{}


where are the bindings created?

nevermind found it in scope. Seems a bit weird to me to not create the bindings/azureidentity in here too. Also why are we expecting unused bindings to be left around needing cleanup? Why not delete them as part of AzureCluster deletion?

The thinking behind this is that this object lives in a different namespace and is only for the purpose of the controller with the nmi pod, technically the cluster/user should not care about it. Also the reason we needed the cleanup controller is that if the user changes the reference to a different identity, the controller will generate a new azureIdentity/Binding and the old one will be left behind

CecileRobertMichon · 2020-12-11T22:17:31Z

cloud/scope/identity.go

+			APIVersion: "aadpodidentity.k8s.io/v1",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      identity.GetAzureIdentityName(p.AzureCluster.Name, p.AzureCluster.Namespace, p.Identity.Name),


if this has the cluster name in it, its lifecycle is owned by the life of the cluster, no? Why not create/delete it when the an AzureCluster is created/deleted, similarly to how we create/delete the azure.json secret?

I just added the cluster name to make it easier to find, but it wasn't there before, its not part of the cluster lifecycle it lives in a different namespace and is for internal use by the controller

nader-ziada · 2020-12-13T21:30:49Z

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

nader-ziada · 2020-12-16T14:28:31Z

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

- support SP identity only - add flavor + topic doc - add identity reconciler

add values to azurecluster instead of expecting an azureidentity

k8s-ci-robot · 2020-12-16T16:28:31Z

@nader-ziada: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-e2e-full	`516cbd2`	link	`/test pull-cluster-api-provider-azure-e2e-full`
pull-cluster-api-provider-azure-apidiff	`d52af1a`	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.

CecileRobertMichon · 2020-12-17T20:26:14Z

lgtm

Are you tracking all follow-ups (clusterctl move, pre-existing aad pod identity deploy, etc) in issues in there respective repos?

devigned

Solid work, @nader-ziada!
lgtm

devigned · 2020-12-17T21:08:49Z

🚀
/lgtm
/approve

k8s-ci-robot · 2020-12-17T21:09:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

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~~ [devigned]

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

CecileRobertMichon · 2020-12-17T21:48:53Z

Just remembered still waiting on the proposal to match the implementation

That's why there's a /hold

devigned · 2020-12-17T22:56:34Z

@CecileRobertMichon I promise, promise, promise to update the proposal by eod tomorrow.

CecileRobertMichon · 2020-12-17T23:00:13Z

/hold cancel

@devigned thanks, github is witness 😉

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 5, 2020

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Oct 5, 2020

k8s-ci-robot requested review from juan-lee and justaugustus October 5, 2020 21:20

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 5, 2020

nader-ziada changed the title ~~adding cluster principal and credentails prvider~~ [WIP] adding cluster principal and credentials provider Oct 6, 2020

devigned suggested changes Oct 7, 2020

View reviewed changes

nader-ziada force-pushed the multitenancy branch from 1b31c9f to 045fad3 Compare October 7, 2020 19:47

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2020

nader-ziada force-pushed the multitenancy branch from 0734ec9 to ee07557 Compare October 9, 2020 22:18

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2020

CecileRobertMichon reviewed Oct 9, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

justaugustus removed their request for review October 10, 2020 19:03

nader-ziada force-pushed the multitenancy branch from ee07557 to aa6ac11 Compare October 15, 2020 14:16

nader-ziada commented Oct 15, 2020

View reviewed changes

cloud/scope/identity.go Outdated Show resolved Hide resolved

nader-ziada commented Oct 15, 2020

View reviewed changes

cloud/scope/identity.go Outdated Show resolved Hide resolved

nader-ziada force-pushed the multitenancy branch 2 times, most recently from e92831d to d55c592 Compare October 16, 2020 15:49

CecileRobertMichon reviewed Oct 16, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

nader-ziada force-pushed the multitenancy branch from d55c592 to ef65d99 Compare October 16, 2020 21:20

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 16, 2020

nader-ziada changed the title ~~[WIP] adding cluster principal and credentials provider~~ [WIP] adding multi tenancy for controller using aad-pod-identity Oct 16, 2020

nader-ziada force-pushed the multitenancy branch from 51aa667 to c22ddf6 Compare December 10, 2020 22:24

CecileRobertMichon reviewed Dec 11, 2020

View reviewed changes

nader-ziada force-pushed the multitenancy branch from c22ddf6 to 516cbd2 Compare December 13, 2020 21:28

nader-ziada added 3 commits December 16, 2020 11:25

using aad-pod-identity for multi-tenancy

df6bd31

- support SP identity only - add flavor + topic doc - add identity reconciler

add e2e test

ea3c578

switch to using getting identity values from internal type

d52af1a

add values to azurecluster instead of expecting an azureidentity

nader-ziada force-pushed the multitenancy branch from 516cbd2 to d52af1a Compare December 16, 2020 16:26

devigned approved these changes Dec 17, 2020

View reviewed changes

k8s-ci-robot assigned devigned Dec 17, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 17, 2020

k8s-ci-robot merged commit 591631e into kubernetes-sigs:master Dec 18, 2020

k8s-ci-robot added this to the v0.4.11 milestone Dec 18, 2020

This was referenced Jan 4, 2021

Support Client Certificate authentication for CAPZ #778

Closed

Multi-tenancy within one manager instance using Azure User Assigned Identities #1104

Closed

CecileRobertMichon mentioned this pull request Apr 13, 2021

Image mcr.microsoft.com/oss/azure/aad-pod-identity/nmi ImagePullPolicy: Always #1305

Closed

shysank mentioned this pull request May 25, 2021

Multitenancy for managed clusters #1386

Merged

3 tasks

CecileRobertMichon mentioned this pull request Jun 14, 2021

Make azure credentials optional in manager deployment #1445

Merged

3 tasks

nojnhuh mentioned this pull request Feb 22, 2023

Add proposal for Azure Service Operator #3113

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding multi tenancy for controller using aad-pod-identity #977

adding multi tenancy for controller using aad-pod-identity #977

nader-ziada commented Oct 5, 2020 •

edited

Loading

k8s-ci-robot commented Oct 5, 2020

nader-ziada commented Oct 5, 2020

devigned left a comment

devigned commented Oct 7, 2020 •

edited

Loading

nader-ziada commented Oct 7, 2020

nader-ziada commented Dec 11, 2020

CecileRobertMichon Dec 11, 2020

CecileRobertMichon Dec 11, 2020

nader-ziada Dec 13, 2020

CecileRobertMichon Dec 11, 2020

nader-ziada Dec 13, 2020

CecileRobertMichon Dec 11, 2020

CecileRobertMichon Dec 11, 2020

nader-ziada Dec 13, 2020

CecileRobertMichon Dec 11, 2020

nader-ziada Dec 13, 2020

nader-ziada commented Dec 13, 2020

nader-ziada commented Dec 16, 2020

k8s-ci-robot commented Dec 16, 2020

CecileRobertMichon commented Dec 17, 2020

devigned left a comment

devigned commented Dec 17, 2020

k8s-ci-robot commented Dec 17, 2020

CecileRobertMichon commented Dec 17, 2020

devigned commented Dec 17, 2020

CecileRobertMichon commented Dec 17, 2020

adding multi tenancy for controller using aad-pod-identity #977

adding multi tenancy for controller using aad-pod-identity #977

Conversation

nader-ziada commented Oct 5, 2020 • edited Loading

k8s-ci-robot commented Oct 5, 2020

nader-ziada commented Oct 5, 2020

devigned left a comment

Choose a reason for hiding this comment

devigned commented Oct 7, 2020 • edited Loading

nader-ziada commented Oct 7, 2020

nader-ziada commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada commented Dec 13, 2020

nader-ziada commented Dec 16, 2020

k8s-ci-robot commented Dec 16, 2020

CecileRobertMichon commented Dec 17, 2020

devigned left a comment

Choose a reason for hiding this comment

devigned commented Dec 17, 2020

k8s-ci-robot commented Dec 17, 2020

CecileRobertMichon commented Dec 17, 2020

devigned commented Dec 17, 2020

CecileRobertMichon commented Dec 17, 2020

nader-ziada commented Oct 5, 2020 •

edited

Loading

devigned commented Oct 7, 2020 •

edited

Loading