Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provide more prescriptive guidance on multi-tenancy #4514

Closed
wants to merge 1 commit into from
Closed

Provide more prescriptive guidance on multi-tenancy #4514

wants to merge 1 commit into from

Conversation

randomvariable
Copy link
Member

What this PR does / why we need it:

As a follow up to the Cluster API meeting of 2021/04/21, this PR intends to give more prescriptive guidance around the multi-tenancy contract. In particular, this documents how cluster scoped resources should work at the present time, and also in a
future release of Cluster API that drops support for older Kubernetes versions.

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 #

/assign @sedefsavas
/assign @devigned
/assign @CecileRobertMichon

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 22, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from cecilerobertmichon after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@randomvariable randomvariable force-pushed the multi-tenancy-guidance-update branch 5 times, most recently from 90cc124 to 4caa22f Compare April 22, 2021 14:36
@randomvariable randomvariable mentioned this pull request Apr 22, 2021
Merged
@randomvariable randomvariable force-pushed the multi-tenancy-guidance-update branch 2 times, most recently from 62cc67b to 106f25a Compare April 22, 2021 14:44
@CecileRobertMichon
Copy link
Contributor

/cc @nader-ziada

@randomvariable randomvariable changed the title Provide more prescriptive guidance on multi-tenancy [WIP] Provide more prescriptive guidance on multi-tenancy Apr 22, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 22, 2021
@randomvariable
Copy link
Member Author

Marked as WIP since we want to include the clusterctl annotation pieces for cluster scoped resource move I think @fabriziopandini ?

@randomvariable
Copy link
Member Author

TODO: Add helper to expose current controller namespace.

@vincepri , reckon I should make a PR to controller-runtime to expose a public utility function that does https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/leaderelection/leader_election.go#L104 ?

@fabriziopandini fabriziopandini mentioned this pull request Apr 22, 2021
Closed
@sbueringer sbueringer mentioned this pull request Apr 22, 2021
Closed
3 tasks
devigned
devigned reviewed Apr 24, 2021
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 just added a question and a little nit.

docs/book/src/developer/architecture/controllers/multi-tenancy.md Outdated Show resolved Hide resolved
docs/book/src/developer/architecture/controllers/multi-tenancy.md Outdated Show resolved Hide resolved
@fabriziopandini fabriziopandini mentioned this pull request May 11, 2021
Merged
@randomvariable
Copy link
Member Author

Right, I have some questions to be answered. I have put them in this google doc https://docs.google.com/document/d/1hnjjPW6oEm3injHJMaVC-gbUYaSIuZB_ZGWVBh3LdMA/edit#

@sedefsavas sedefsavas mentioned this pull request May 12, 2021
Closed
@sbueringer
Copy link
Member

@randomvariable @vincepri I'm currently working towards the CAPO v0.4.0 release. Is the v0.4.0 provider contract the current state of this PR or what's documented here? https://cluster-api.sigs.k8s.io/developer/architecture/controllers/multi-tenancy.html?highlight=multi%20ten#multi-tenancy

P.S. I was on vacation the last weeks so I could have missed other discussions / PRs etc about this topic

@sbueringer sbueringer mentioned this pull request Jun 29, 2021
Closed
@randomvariable randomvariable changed the title [WIP] Provide more prescriptive guidance on multi-tenancy Provide more prescriptive guidance on multi-tenancy Jul 14, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2021
@randomvariable
Provide more prescriptive guidance on multi-tenancy
e1a76d2 
Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
@randomvariable
Copy link
Member Author

This is updated as per the comments. I've tried to balance what the providers have done with the consensus in the discussions. PTAL.

@randomvariable randomvariable mentioned this pull request Jul 14, 2021
Closed
sbueringer
sbueringer reviewed Jul 27, 2021
View reviewed changes
api/v1alpha4/identity_types.go

// Name of the infrastructure identity to be used.
// Must be either a cluster-scoped resource, or namespaced-scoped
// resource the same namespace as the resource(s) being provisioned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// resource the same namespace as the resource(s) being provisioned.
// resource with the same namespace as the resource(s) being provisioned.

docs/book/src/developer/architecture/controllers/multi-tenancy.md
}
```

- A Namespace field MUST never be added to the ProviderIdentityReference type to avoid crossing namespace boundaries.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the ProviderIdentityReference type still exist?

docs/book/src/developer/architecture/controllers/multi-tenancy.md
// ADDITIONAL INFRASTRUCTURE SPECIFIC IDENTITY FIELDS//


clusterv1.InfraClusterScopedResourceIdentityCommonSpec `json:",inline"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not exist in the code, renamed?

docs/book/src/developer/architecture/controllers/multi-tenancy.md
is resolved -->
These behaviours will be encoded in utility functions in the Cluster API repository at a later date.

### Namespaced scoped resources
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Namespaced scoped resources
### Namespace-scoped resources

docs/book/src/developer/architecture/controllers/multi-tenancy.md

## Supported RBAC Models

Providers MAY support any combination of cluster-scoped or namespace-scoped resources as follows:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the secret variant should also be added here

api/v1alpha4/identity_types.go
//
// This field is mutually exclusive with list.
// +optional
Selector metav1.LabelSelector `json:"selector"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably stupid question: Can we differentiate between null and empty when the selector is not a pointer?

@sbueringer
Copy link
Member

@randomvariable Thx for clarifying the part with the secret!

@vincepri
Copy link
Member

@randomvariable Do you have some time to fix the comments above and make the bot happy? We can do another round of review thereafter

@k8s-ci-robot
Copy link
Contributor

@randomvariable: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-test-main e1a76d2 link /test pull-cluster-api-test-main
pull-cluster-api-verify e1a76d2 link /test pull-cluster-api-verify
pull-cluster-api-verify-main e1a76d2 link true /test pull-cluster-api-verify-main
pull-cluster-api-e2e-main e1a76d2 link true /test pull-cluster-api-e2e-main

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.

@enxebre
Copy link
Member

enxebre commented Dec 13, 2021

@randomvariable should we close this one?

@enxebre
Copy link
Member

enxebre commented Mar 1, 2022

hey @randomvariable just doing some housekeeping here, I'm closing this since does not seem to apply anymore, plase let me know or just reopen if I'm mistaken.
/close

@k8s-ci-robot
Copy link
Contributor

@enxebre: Closed this PR.

In response to this:

hey @randomvariable just doing some housekeeping here, I'm closing this since does not seem to apply anymore, plase let me know or just reopen if I'm mistaken.
/close

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.

@sbueringer
Copy link
Member

sbueringer commented Mar 2, 2022 •
edited
Loading

@enxebre I assume this means that the current contract regarding multi tenancy remains what we currently have in the book? https://cluster-api.sigs.k8s.io/developer/architecture/controllers/multi-tenancy.html

If I remember correctly some providers already aligned to what is this described in this PR. But I think given that this PR here never merged, we can't really treat it as an active contract. (Or maybe it's documented elsewhere and I'm just not aware)

I can't really assess what this means for us, why this contract addition was important, ...

P.S. I agree that this PR itself doesn't really make sense if it's just sitting there. I just wonder if the topic is still relevant.

@sbueringer
Copy link
Member

/cc @CecileRobertMichon @fabriziopandini @vincepri

@fabriziopandini
Copy link
Member

@yastij this could be relevant for the overarching goal to ensure consistency across providers...

@sbueringer sbueringer mentioned this pull request Nov 28, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devigned devigned devigned left review comments

@gab-satchi gab-satchi gab-satchi left review comments

@sbueringer sbueringer sbueringer left review comments

@yastij yastij yastij left review comments

@nader-ziada nader-ziada nader-ziada left review comments

@tobiasgiese tobiasgiese tobiasgiese left review comments

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@moshloop moshloop Awaiting requested review from moshloop

@CecileRobertMichon CecileRobertMichon Awaiting requested review from CecileRobertMichon

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

@vincepri vincepri Awaiting requested review from vincepri

Assignees

@devigned devigned

@CecileRobertMichon CecileRobertMichon

@sedefsavas sedefsavas

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.