First draft of multi-tenancy proposal #1149

gab-satchi · 2021-03-18T16:07:33Z

/kind design
What this PR does / why we need it:
Proposing a way to allow CAPV to accommodate multiple tenants within a single instance

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

Thanks to the AWS and Azure folks for pioneering the multi-tenancy efforts in their respective providers. @devigned @randomvariable @nader-ziada @sedefsavas

sedefsavas · 2021-03-18T20:40:24Z

docs/proposal/20210311-single-controller-multitenancy.md

+    // account from any namespace. This field is intentionally not a
+    // pointer because the nil behavior (no namespaces) is undesirable here.
+    // +optional
+    AllowedNamespaces []string `json:"allowedNamespaces"`


This is how CAPZ defines AllowedNamespaces, in CAPA we have 2 main differences:

We modified AllowedNamespaces to also support labelSelectors:

type AllowedNamespaces struct { // An nil or empty list indicates that AWSClusters cannot use the principal from any namespace. // // +optional // +nullable NamespaceList []string `json:"list"` // AllowedNamespaces is a selector of namespaces that AWSClusters can // use this ClusterPrincipal from. This is a standard Kubernetes LabelSelector, // a label query over a set of resources. The result of matchLabels and // matchExpressions are ANDed. // // An empty selector indicates that AWSClusters cannot use this // AWSClusterPrincipal from any namespace. // +optional Selector metav1.LabelSelector `json:"selector"` }

Also the default behaviour is "allow none" whenAllowedNamespaces is nil to enforce users to make explicit choices.

It may be useful to have a common way of defining this across providers, we can discuss what it may look like. cc @nader-ziada @devigned

@sedefsavas we had some issues with implementing the label selector and that's why we only did the []string, but I do agree its good a consistent behaviour for the default across providers

I like both the differences with CAPA. Was originally thinking label selector was overkill but I can see how that's useful with targeting multiple namespaces without having to list each of them out. Will update, thanks Sedef

+1 to consistent behavior across providers.

What is the behavior when NamespaceList is nil and a selector is set? Is the result always the union of NamespaceList + Selector?

yeah in CAPA's implementation, it is a union of the two. That'll offer the most flexibility

empty allowedNamespaces --> allows all namespaces
nil allowedNamespaces --> blocks all namespaces
Note: This is similar to how networkPolicy label matching is done.

nil NamespaceList --> blocks all
nil Selector --> blocks all

What is the behavior when NamespaceList is nil and a selector is set? Is the result always the union of NamespaceList + Selector?

Yes, if at least one of them matches a namespace, it is allowed.

I drafted some examples for how we use allowedNamespaces in CAPA:
https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/2319/files

I'm tempted to say we pull out the list option for v1alpha4. I will take the conversation to CAPI, but kubernetes/kubernetes#101342 is going to move the NamespaceDefaultLabelName feature gate to GA and has been defaulted to on from 1.21 .

PTAL kubernetes-sigs/cluster-api#4514

sedefsavas · 2021-03-18T20:46:24Z

docs/proposal/20210311-single-controller-multitenancy.md

+## Summary
+The CAPV controller is capable of managing infrastructure resources on a vCenter using the credentials it was provided during initialization. The credentials are provided via environment variables that get saved onto a secret that's used by the CAPV deployment. 
+
+Credentials provided are used for the entire lifetime of the CAPV deployment which means a CAPV cluster can become broken if the provisioning CAPV deployment were to be reconfigred for another set of credentials.


How multitenancy can fix CAPV deployment credential issue?
CAPA will start with credentials retrieved locally, then other roles could be used for creating workload clusters.
When the local credentials (that CAPV uses) change, is the plan to update the secret?

I don't think MT can fix any existing clusters from breaking when the deployment credentials are changed. This was more highlighting a cluster's dependance on the deployment credentials and if MT is fully utilized, it can move that dependency to a VSphereAccount

we can't do much if the creds are no longer valid. the user would have to go an edit them, but CAPV should still be on the hook of reading creds at each reconciliation and avoid caching them, to support cred rotation

k8s-ci-robot · 2021-03-19T15:46:42Z

@gab-satchi: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-vsphere-verify-lint	`4489ea6`	link	`/test pull-cluster-api-provider-vsphere-verify-lint`
pull-cluster-api-provider-vsphere-e2e	`4489ea6`	link	`/test pull-cluster-api-provider-vsphere-e2e`
pull-cluster-api-provider-vsphere-test	`4489ea6`	link	`/test pull-cluster-api-provider-vsphere-test`
pull-cluster-api-provider-vsphere-verify-crds	`4489ea6`	link	`/test pull-cluster-api-provider-vsphere-verify-crds`
pull-cluster-api-provider-vsphere-verify-shell	`4489ea6`	link	`/test pull-cluster-api-provider-vsphere-verify-shell`

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.

yastij

did a first pass on the API

docs/proposal/20210311-single-controller-multitenancy.md

yastij · 2021-03-25T15:03:51Z

docs/proposal/20210311-single-controller-multitenancy.md

+## Summary
+The CAPV controller is capable of managing infrastructure resources on a vCenter using the credentials it was provided during initialization. The credentials are provided via environment variables that get saved onto a secret that's used by the CAPV deployment. 
+
+Credentials provided are used for the entire lifetime of the CAPV deployment which means a CAPV cluster can become broken if the provisioning CAPV deployment were to be reconfigred for another set of credentials.


we can't do much if the creds are no longer valid. the user would have to go an edit them, but CAPV should still be on the hook of reading creds at each reconciliation and avoid caching them, to support cred rotation

docs/proposal/20210311-single-controller-multitenancy.md

yastij · 2021-04-22T13:14:46Z

docs/proposal/20210311-single-controller-multitenancy.md

+
+#### Controller Changes
+
+- If IdentityRef is specified, and it references a `VsphereClusterAccount`, the CRD is fetched and used to create a session


@randomvariable @fabriziopandini - it might worthwhile to inject this reference if nothing is set and there are VsphereClusterAccount that are allowed for that namespace. This avoids having to communicate the kind and Name to folks that want to provision clusters and that don't care much about creds ?

I find the idea interesting, because it makes it easier to decouple roles and responsibilities
What there are more than one Account eligible for a cluster?

We have a mechanism in CAPA for that called AWSClusterControllerIdentity that respects the existing behaviour as well as EC2 instance profiles for cross-account role assumption which is a key requirement for AWS. See https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/docs/proposal/20200506-single-controller-multitenancy.md#upgrade-strategy

Azure doesn't have cross-account role assumption so this wasn't an important use case for them, so was not implemented there.

docs/proposal/20210311-single-controller-multitenancy.md

fabriziopandini · 2021-04-22T13:28:14Z

docs/proposal/20210311-single-controller-multitenancy.md

+
+#### Clusterctl Changes
+
+Today, clusterctl move operates by tracking objectreferences within the same namespace, since we are now proposing to use cluster-scoped resources, we will need to add requisite support to clusterctl's object graph to track cluster-scoped resources that are used by the source cluster, and ensure they are moved. We will naively not delete cluster-scoped resources during a move, as they maybe referenced across namespaces. If a cluster uses a `Secret` for account credentials, the OwnerReference will get set by the controller and the `Secret` will be moved to the target cluster. Any `VsphereClusterAccount` that's required by the resources being moved, will need to be manually created on the target cluster prior to the move operation.


clusterctl's object graph to track cluster-scoped resources that are used by the source cluster,

This is kind of complex, given that the only relation between cluster and VsphereClusterAccount is defined in the identityRef field, and thus unknown to the clusterctl move discovery algorithm (that should be generic/provider agnostic).

There are two possible workarounds:

Move all the VsphereClusterAccount (by adding the force move label on the VsphereClusterAccount CRD)

Move only specific VsphereClusterAccount (by adding the force move label only on those objects)

docs/proposal/20210311-single-controller-multitenancy.md

yastij

/approve

this should be ready to go, I'll leave lgtm to @fabriziopandini re: move choices

k8s-ci-robot · 2021-04-30T15:51:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yastij

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

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

fabriziopandini · 2021-05-02T18:42:04Z

docs/proposal/20210311-single-controller-multitenancy.md

+    AllowedNamespaces *AllowedNamespaces `json:"allowedNamespaces"`
+}
+
+type AllowedNamespaces struct {


according the general guidelines being proposed in CAPI, NamespaceList is missing

The PR calls out to deprecate NamespaceList for v1alpha4

Providers SHOULD mark NamespaceList as deprecated from v1alpha4

docs/proposal/20210311-single-controller-multitenancy.md

- adds Secret as a credential option - adds VSphereIdentityRef - adds VSphereClusterIdentity as a credential option - utilizes AllowedNamespaces to restrict access to credentials - follows CAPI guidelines for multi-tenancy

yastij · 2021-05-05T16:29:41Z

/lgtm

k8s-ci-robot added kind/design Categorizes issue or PR as related to design. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 18, 2021

k8s-ci-robot requested review from detiber and vincepri March 18, 2021 16:07

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 18, 2021

sedefsavas reviewed Mar 18, 2021

View reviewed changes

yastij self-assigned this Mar 19, 2021

gab-satchi force-pushed the multitenancy-proposal branch from 98cbd19 to 4489ea6 Compare March 19, 2021 15:46

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

gab-satchi changed the base branch from release-0.7 to master March 19, 2021 15:46

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

yastij reviewed Mar 25, 2021

View reviewed changes

fabriziopandini reviewed Mar 30, 2021

View reviewed changes

jzhoucliqr reviewed Apr 1, 2021

View reviewed changes

docs/proposal/20210311-single-controller-multitenancy.md Outdated Show resolved Hide resolved

randomvariable reviewed Apr 1, 2021

View reviewed changes

docs/proposal/20210311-single-controller-multitenancy.md Outdated Show resolved Hide resolved

randomvariable reviewed Apr 1, 2021

View reviewed changes

docs/proposal/20210311-single-controller-multitenancy.md Outdated Show resolved Hide resolved

nader-ziada mentioned this pull request Apr 1, 2021

Change multi-tenancy allowed namespaces to be more restrictive kubernetes-sigs/cluster-api-provider-azure#1288

Closed

yastij reviewed Apr 22, 2021

View reviewed changes

fabriziopandini reviewed Apr 22, 2021

View reviewed changes

randomvariable reviewed Apr 22, 2021

View reviewed changes

yastij approved these changes Apr 30, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2021

fabriziopandini reviewed May 2, 2021

View reviewed changes

Initial proposal

2380712

- adds Secret as a credential option - adds VSphereIdentityRef - adds VSphereClusterIdentity as a credential option - utilizes AllowedNamespaces to restrict access to credentials - follows CAPI guidelines for multi-tenancy

gab-satchi force-pushed the multitenancy-proposal branch from 288a4b1 to 2380712 Compare May 5, 2021 16:20

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2021

k8s-ci-robot merged commit b1e3f2f into kubernetes-sigs:master May 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First draft of multi-tenancy proposal #1149

First draft of multi-tenancy proposal #1149

gab-satchi commented Mar 18, 2021

sedefsavas Mar 18, 2021

nader-ziada Mar 18, 2021

gab-satchi Mar 19, 2021

devigned Mar 19, 2021

gab-satchi Mar 19, 2021

sedefsavas Mar 19, 2021 •

edited

sedefsavas Mar 22, 2021

randomvariable Apr 22, 2021 •

edited

randomvariable Apr 22, 2021

sedefsavas Mar 18, 2021

gab-satchi Mar 19, 2021

yastij Mar 25, 2021

k8s-ci-robot commented Mar 19, 2021 •

edited

yastij left a comment

yastij Mar 25, 2021

yastij Apr 22, 2021

fabriziopandini Apr 22, 2021

randomvariable Apr 22, 2021

fabriziopandini Apr 22, 2021

yastij left a comment

k8s-ci-robot commented Apr 30, 2021

fabriziopandini May 2, 2021

yastij May 3, 2021

gab-satchi May 3, 2021

yastij commented May 5, 2021


		#### Controller Changes

		- If IdentityRef is specified, and it references a `VsphereClusterAccount`, the CRD is fetched and used to create a session


		#### Clusterctl Changes

		Today, clusterctl move operates by tracking objectreferences within the same namespace, since we are now proposing to use cluster-scoped resources, we will need to add requisite support to clusterctl's object graph to track cluster-scoped resources that are used by the source cluster, and ensure they are moved. We will naively not delete cluster-scoped resources during a move, as they maybe referenced across namespaces. If a cluster uses a `Secret` for account credentials, the OwnerReference will get set by the controller and the `Secret` will be moved to the target cluster. Any `VsphereClusterAccount` that's required by the resources being moved, will need to be manually created on the target cluster prior to the move operation.

First draft of multi-tenancy proposal #1149

First draft of multi-tenancy proposal #1149

Conversation

gab-satchi commented Mar 18, 2021

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

sedefsavas Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randomvariable Apr 22, 2021 • edited

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

k8s-ci-robot commented Mar 19, 2021 • edited

yastij 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

yastij left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yastij commented May 5, 2021

sedefsavas Mar 19, 2021 •

edited

randomvariable Apr 22, 2021 •

edited

k8s-ci-robot commented Mar 19, 2021 •

edited