🏃add draft of multi-tenancy proposal #809

devigned · 2020-07-20T21:50:22Z

What this PR does / why we need it:

We need to provide the Cluster an identity to use for provisioning besides the default identity of the controller. This will enable the CAPZ controllers to work as a single instance with multiple tenants.

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

see also: kubernetes-sigs/cluster-api-provider-aws#1674

Big thank you to @randomvariable and @andrewmyhre! I cribbed much of this document from the AWS provider proposal.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

NONE

docs/proposals/20200720-single-controller-multitenancy.md

alexeldeib · 2020-07-21T02:05:02Z

docs/proposals/20200720-single-controller-multitenancy.md

+the principal types. Upon receiving an update event, the controller will update lookup the key in 
+the cache and update the relevant `Authorizer`. This may be implemented as its own interface. 
+Mutexes will ensure in-flight updates are completed prior to SDK calls are made. This would require 
+changes to RBAC, and maintaining a watch on secrets of a specific type will require further


maintaining a watch on secrets of a specific type will require further
investigation as to feasibility

this sounds fine to me, anything you're worried about? if there's any identifying metadata you can use it to write a filter

I was not, but the CAPA authors were. Not sure if I missed something... Wanted to leave it in for someone smarter than me to spot the hole.

docs/proposals/20200720-single-controller-multitenancy.md

nader-ziada · 2020-07-21T17:02:46Z

docs/proposals/20200720-single-controller-multitenancy.md

+* The controller will set the Identity resource as one of the OwnerReferences of the AzureCluster.
+
+### Clusterctl changes
+


related issue in cluster-api kubernetes-sigs/cluster-api#3354

CecileRobertMichon

I think the proposal is sane, I like the proposed API changes. Mostly added some clarifying questions/comments.

docs/proposals/20200720-single-controller-multitenancy.md

CecileRobertMichon · 2020-07-21T20:06:39Z

docs/proposals/20200720-single-controller-multitenancy.md

+<a name="FR3">FR3.</a> CAPZ MUST prevent privilege escalation allowing users to create clusters in Azure accounts they should
+  not be able to.
+
+<a name="FR4">FR4.</a> CAPZ SHOULD support credential refreshing modified principal data.


is an example of this being able to refresh an expired service principal secret to be able to keep managing an existing cluster?

This is more about someone changing the content of the secret, which should cause the Authorizer cache to revoke the existing Authorizer and rebuild it from the changed secret.

CecileRobertMichon · 2020-07-21T20:07:12Z

docs/proposals/20200720-single-controller-multitenancy.md

+
+<a name="FR4">FR4.</a> CAPZ SHOULD support credential refreshing modified principal data.
+
+<a name="FR5">FR5.</a> CAPZ SHOULD provide validation for principal data submitted by users.


what kind of validation? Does that include validating that the credentials are valid/not expired?

It would be nice to attempt to get a token from AAD. If we are unable to get a token from AAD, perhaps we get a 401, we should reject the secret as invalid.

CecileRobertMichon · 2020-07-21T20:07:37Z

docs/proposals/20200720-single-controller-multitenancy.md

+
+### Non-Functional
+
+<a name="NFR1">NFR1.</a> Each instance of CAPZ SHOULD be able to support 200 clusters using role assumption.


Seems like a reasonable aspiration and was part of the CAPA proposal.

docs/proposals/20200720-single-controller-multitenancy.md

CecileRobertMichon · 2020-07-21T20:18:09Z

docs/proposals/20200720-single-controller-multitenancy.md

+
+<em>Cluster scoped resources</em>
+
+* `AzureServicePrincipal` represents an AAD Service Principal. Should support both 


related to #778

CecileRobertMichon · 2020-07-21T20:19:53Z

docs/proposals/20200720-single-controller-multitenancy.md

+imperfect. For example, a change to an Azure Identity resource could affect the validity of 
+corresponding AzureCluster.
+
+#### Principal Type Credential Provider Behaviour


Conversion changes? What happens if you have a v1alpha3 cluster using SystemAssigned identity and you try to convert to v1alpha2?

That's a great question. I think that fails.

We could add the identity information to the metadata of the v1alpha2 version of the AzureCluster. Thoughts?

We should at least document that as breaking. Wonder how CAPA is handling conversion... we should probably align on that.

devigned · 2020-07-24T12:58:48Z

I think I've addressed all of the feedback thus far. PTAL when you all have a min.

CecileRobertMichon · 2020-07-24T17:32:24Z

docs/proposals/20200720-single-controller-multitenancy.md

+  Name string `json:"name"`
+}
+
+type  AzureClusterSpec  struct {


I just thought about this, what's the impact on the subscriptionID field in AzureClusterSpec? None right?

The changes should have no impact on subscriptionID.

An AAD identity has nothing to do with a subscription. Auth only needs the tenant, client ID, a secret, and resource or scopes.

If one day in the future we decide to build multi-subscription clusters, then we'd have to make a change to subscription ID.

Got it. Thanks for the clarification.

CecileRobertMichon · 2020-07-24T17:42:38Z

/hold

Starting lazy consensus for a week from now (7/31)

CecileRobertMichon · 2020-07-27T17:18:01Z

another quick question: I assume this has no impact on #481, correct? Since this is for identity for infra provisioning

devigned · 2020-07-28T14:37:47Z

another quick question: I assume this has no impact on #481, correct? Since this is for identity for infra provisioning

@CecileRobertMichon, nope. I think this is more for user auth to the cluster (kubectl auth).

docs/proposals/20200720-single-controller-multitenancy.md

CecileRobertMichon · 2020-07-31T21:01:43Z

Last chance for comments, will remove the hold Monday morning

CecileRobertMichon

/lgtm

nader-ziada · 2020-08-01T15:17:01Z

lgtm

CecileRobertMichon · 2020-08-03T16:47:08Z

/hold cancel
/approve

k8s-ci-robot · 2020-08-03T16:47:17Z

[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

devigned · 2020-08-03T17:52:52Z

/retest

devigned requested review from alexeldeib, nader-ziada and CecileRobertMichon July 20, 2020 21:50

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 20, 2020

k8s-ci-robot requested a review from juan-lee July 20, 2020 21:50

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 20, 2020

CecileRobertMichon reviewed Jul 20, 2020

View reviewed changes

docs/proposals/20200720-single-controller-multitenancy.md Outdated Show resolved Hide resolved

alexeldeib reviewed Jul 21, 2020

View reviewed changes

docs/proposals/20200720-single-controller-multitenancy.md Outdated Show resolved Hide resolved

alexeldeib reviewed Jul 21, 2020

View reviewed changes

docs/proposals/20200720-single-controller-multitenancy.md Show resolved Hide resolved

alexeldeib reviewed Jul 21, 2020

View reviewed changes

docs/proposals/20200720-single-controller-multitenancy.md Outdated Show resolved Hide resolved

alexeldeib reviewed Jul 21, 2020

View reviewed changes

docs/proposals/20200720-single-controller-multitenancy.md Outdated Show resolved Hide resolved

alexeldeib reviewed Jul 21, 2020

View reviewed changes

docs/proposals/20200720-single-controller-multitenancy.md Outdated Show resolved Hide resolved

alexeldeib reviewed Jul 21, 2020

View reviewed changes

docs/proposals/20200720-single-controller-multitenancy.md Outdated Show resolved Hide resolved

nader-ziada reviewed Jul 21, 2020

View reviewed changes

CecileRobertMichon reviewed Jul 21, 2020

View reviewed changes

alexeldeib mentioned this pull request Jul 22, 2020

[multitenancy/managedcluster] enable reading Azure credentials from secret #614

Closed

alexeldeib mentioned this pull request Jul 23, 2020

[WIP]✨ integrate AzureMachine with AzureManagedControlPlane #822

Closed

3 tasks

devigned force-pushed the multitenant branch 6 times, most recently from eae3e0a to ed06a34 Compare July 24, 2020 12:55

devigned requested a review from alexeldeib July 24, 2020 12:58

devigned requested review from CecileRobertMichon and nader-ziada July 24, 2020 12:58

CecileRobertMichon reviewed Jul 24, 2020

View reviewed changes

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

alexeldeib reviewed Jul 28, 2020

View reviewed changes

docs/proposals/20200720-single-controller-multitenancy.md Outdated Show resolved Hide resolved

devigned force-pushed the multitenant branch from ed06a34 to 59189b7 Compare July 28, 2020 17:04

add first draft of multitenancy proposal

0deb03d

devigned force-pushed the multitenant branch from 59189b7 to 0deb03d Compare July 28, 2020 17:05

devigned changed the title ~~add first draft of multitenancy proposal~~ 🏃add draft of multi-tenancy proposal Jul 30, 2020

CecileRobertMichon approved these changes Jul 31, 2020

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Jul 31, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 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 Aug 3, 2020

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

k8s-ci-robot merged commit c28cdfc into kubernetes-sigs:master Aug 3, 2020

k8s-ci-robot added this to the v0.4.7 milestone Aug 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏃add draft of multi-tenancy proposal #809

🏃add draft of multi-tenancy proposal #809

devigned commented Jul 20, 2020

alexeldeib Jul 21, 2020

devigned Jul 24, 2020

nader-ziada Jul 21, 2020

CecileRobertMichon left a comment

CecileRobertMichon Jul 21, 2020

devigned Jul 23, 2020

CecileRobertMichon Jul 21, 2020

devigned Jul 23, 2020

CecileRobertMichon Jul 21, 2020

devigned Jul 23, 2020

CecileRobertMichon Jul 21, 2020

CecileRobertMichon Jul 21, 2020

devigned Jul 23, 2020

CecileRobertMichon Jul 27, 2020

devigned commented Jul 24, 2020

CecileRobertMichon Jul 24, 2020

devigned Jul 24, 2020

CecileRobertMichon Jul 24, 2020

CecileRobertMichon commented Jul 24, 2020

CecileRobertMichon commented Jul 27, 2020

devigned commented Jul 28, 2020

CecileRobertMichon commented Jul 31, 2020

CecileRobertMichon left a comment

nader-ziada commented Aug 1, 2020

CecileRobertMichon commented Aug 3, 2020

k8s-ci-robot commented Aug 3, 2020

devigned commented Aug 3, 2020

		* The controller will set the Identity resource as one of the OwnerReferences of the AzureCluster.

		### Clusterctl changes


		<a name="FR4">FR4.</a> CAPZ SHOULD support credential refreshing modified principal data.

		<a name="FR5">FR5.</a> CAPZ SHOULD provide validation for principal data submitted by users.


		### Non-Functional

		<a name="NFR1">NFR1.</a> Each instance of CAPZ SHOULD be able to support 200 clusters using role assumption.


		<em>Cluster scoped resources</em>

		* `AzureServicePrincipal` represents an AAD Service Principal. Should support both

🏃add draft of multi-tenancy proposal #809

🏃add draft of multi-tenancy proposal #809

Conversation

devigned commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devigned commented Jul 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Jul 24, 2020

CecileRobertMichon commented Jul 27, 2020

devigned commented Jul 28, 2020

CecileRobertMichon commented Jul 31, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

nader-ziada commented Aug 1, 2020

CecileRobertMichon commented Aug 3, 2020

k8s-ci-robot commented Aug 3, 2020

devigned commented Aug 3, 2020