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.

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

📖 Document multi-tenancy contract #4074

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
As per discussion on January 13th office hour meeting, this PR updates the cluster API book by:

  1. Removing outdated definitions/related paragraphs in the clusterctl documentation
  2. Documenting the new definition of multi tenancy, as per Adapt clusterctl move to the new multi-tenancy model #3042, and the impact of such definition on the contract (including e.g. and Run webhooks as part of the main manager #3822)
  3. Documenting the lesson learned in running multiple instances of the same providers in v1alpha3, and the intent to preserve the possibility to go down this path if someone wants (make it possible)

Please note that the last point most probably does not represent yet the final state and it should be intended as a hook for the follow-up work for #4004.

/area clusterctl

/cc @wfernandes @vincepri @CecileRobertMichon @detiber @MarcelMue @yastij @randomvariable

@k8s-ci-robot k8s-ci-robot added the area/clusterctl Issues or PRs related to clusterctl label Jan 14, 2021
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 14, 2021
@k8s-ci-robot
Copy link
Contributor

@fabriziopandini: GitHub didn't allow me to request PR reviews from the following users: MarcelMue.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it:
As per discussion on January 13th office hour meeting, this PR updates the cluster API book by:

  1. Removing outdated definitions/related paragraphs in the clusterctl documentation
  2. Documenting the new definition of multi tenancy, as per Adapt clusterctl move to the new multi-tenancy model #3042, and the impact of such definition on the contract (including e.g. and Run webhooks as part of the main manager #3822)
  3. Documenting the lesson learned in running multiple instances of the same providers in v1alpha3, and the intent to preserve the possibility to go down this path if someone wants (make it possible)

Please note that the last point most probably does not represent yet the final state and it should be intended as a hook for the follow-up work for #4004.

/area clusterctl

/cc @wfernandes @vincepri @CecileRobertMichon @detiber @MarcelMue @yastij @randomvariable

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.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 14, 2021
@@ -283,8 +283,6 @@ Provider authors should be aware of the following transformations that `clusterc
* Enforcement of target namespace:
* The name of the namespace object is set;
* The namespace field of all the objects is set (with exception of cluster wide objects like e.g. ClusterRoles);
* ClusterRole and ClusterRoleBinding are renamed by adding a “${namespace}-“ prefix to the name; this change reduces the risks
Copy link
Member Author

Choose a reason for hiding this comment

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

@wfernandes this is a clusterctl internal; we should probably re-consider this during the implementation phase of the CAPI provider operator

- All the infrastructure provider are required to manage different credentials.
- All the provider should deploy and run web-hook and with managers;
please note this is the default in controllers generated with kubebuilder.
- All the provider are required to create a `component.yaml` accordingly.
Copy link
Member Author

Choose a reason for hiding this comment

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

My intent is to link documentation from #3985 here, depending on which PR is going to merge first


- In the Cluster API project, we are not going to include release a `component.yaml` file supporting this scenario;
however it will be possible to create such file starting from the content of the `/config` folder.
- clusterctl and the CAPI provider operator are not going to support deploying/managing multiple instances of the same provider.
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
- clusterctl and the CAPI provider operator are not going to support deploying/managing multiple instances of the same provider.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should state this in order to set clear expectations, unless you think this is clear from the previous statement

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

looks great

@@ -0,0 +1,41 @@
# Support running multiple instances of the same provider

Up until v1alpha3, the need of supporting multiple credentials was addressed by running multiple
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, PTAL

@MarcelMue
Copy link
Contributor

LGTM after reading through it closely.

@fabriziopandini
Copy link
Member Author

/milestone v0.4.0

@k8s-ci-robot k8s-ci-robot added this to the v0.4.0 milestone Jan 19, 2021
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2021
Copy link
Contributor

@wfernandes wfernandes left a comment

Choose a reason for hiding this comment

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

No blockers. LGTM.

@fabriziopandini fabriziopandini force-pushed the document-multi-tenancy-contract branch from 7e6ed1f to bb54134 Compare January 20, 2021 12:54
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2021
@fabriziopandini
Copy link
Member Author

Latest comments addressed + squash

@wfernandes
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2021
Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2021
@k8s-ci-robot k8s-ci-robot merged commit c615219 into kubernetes-sigs:master Jan 20, 2021
@fabriziopandini fabriziopandini deleted the document-multi-tenancy-contract branch January 21, 2021 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants