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

NestedControlPlane & NestedCluster Proposal #29

Merged
merged 1 commit into from
Mar 10, 2021
Merged

NestedControlPlane & NestedCluster Proposal #29

merged 1 commit into from
Mar 10, 2021

Conversation

christopherhein
Copy link
Member

@christopherhein christopherhein commented Jan 26, 2021 •
edited

Adding new proposal fromm hackmd with regards to how the NCP and NC types are made. This probably still needs a bit of work related to diagrams of interactions but for the most part these types aren't doing the actual orchestration of the control plane.

Closes #7

Signed-off-by: Chris Hein me@chrishein.com

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 26, 2021
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 26, 2021
@christopherhein christopherhein marked this pull request as draft January 26, 2021 18:39
@christopherhein
Copy link
Member Author

/cc @Fei-Guo @charleszheng44

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 29, 2021
@christopherhein
Copy link
Member Author

Per #30 Sounds like I need to add some functions in here about how we generate Certs if they aren't supplied.

@kfox1111
Copy link

kfox1111 commented Feb 2, 2021

Using cert-manager please? cluster-api already depends on it.

@christopherhein
Copy link
Member Author

Yeah, @kfox1111 I this is an option, similar to how the kubeadm bootstrap provider will create certs for you if you don't have them already created and stored in secrets. Regarding the cert-manager integration, do you think it should default to creating Certificate resources from the NestedControlPlane controller?

@kfox1111
Copy link

kfox1111 commented Feb 2, 2021

Yeah. I think that would work well.

@christopherhein
Copy link
Member Author

More discussion about this doc happened on today's CAPN call - recording in the notes as well as a quick summary in writing - https://docs.google.com/document/d/10aTeq2lhXW_3aFQAd_MdGjY8PtZPslKhZCCcXxFp3_Q/edit#

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 11, 2021
@christopherhein
Copy link
Member Author

/cc @charleszheng44 @Fei-Guo @weiling61

@k8s-ci-robot
Copy link
Contributor

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

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

In response to this:

/cc @charleszheng44 @Fei-Guo @weiling61

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.

@christopherhein christopherhein changed the title [WIP] NestedControlPlane & NestedCluster Proposal NestedControlPlane & NestedCluster Proposal Feb 11, 2021
@christopherhein christopherhein marked this pull request as ready for review February 11, 2021 02:32
@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 Feb 11, 2021
Fei-Guo
Fei-Guo reviewed Feb 11, 2021
View reviewed changes
Copy link

@Fei-Guo Fei-Guo left a comment

Choose a reason for hiding this comment

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

Should we update "creating-controll-plane-components" proposal? since the doc still mentions that the CA will be created by each component controller.

docs/proposals/20210126-nc-and-ncp.md Outdated Show resolved Hide resolved
@christopherhein
Copy link
Member Author

Should we update "creating-controll-plane-components" proposal? since the doc still mentions that the CA will be created by each component controller.

I think so, but lets make sure we're all in agreement and I can do a follow-up PR with this change.

@christopherhein christopherhein mentioned this pull request Feb 16, 2021
Merged
This was referenced Feb 23, 2021
Closed
Closed
Closed
Closed
weiling61
weiling61 approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@weiling61 weiling61 left a comment

Choose a reason for hiding this comment

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

Looks good

docs/proposals/20201026-creating-control-plane-components.md Show resolved Hide resolved
vincepri
vincepri approved these changes Feb 24, 2021
View reviewed changes
docs/Makefile Outdated Show resolved Hide resolved
miamipanther
miamipanther approved these changes Mar 1, 2021
View reviewed changes
Copy link

@miamipanther miamipanther left a comment •
edited

Choose a reason for hiding this comment

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

Looking good to me

@k8s-ci-robot
Copy link
Contributor

@miamipanther: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@christopherhein
adding initial NCP + NC proposal
9ed258c 
Signed-off-by: Chris Hein <me@chrishein.com>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christopherhein, miamipanther, vincepri, weiling61

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 [christopherhein,vincepri]

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

@charleszheng44
Copy link
Contributor

charleszheng44 commented Mar 10, 2021 •
edited

/lgtm

@Fei-Guo Fei-Guo merged commit 91fb2b5 into kubernetes-sigs:master Mar 10, 2021
@christopherhein christopherhein deleted the proposal/ncp-document branch March 10, 2021 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charleszheng44 charleszheng44 charleszheng44 left review comments

@vincepri vincepri vincepri approved these changes

@miamipanther miamipanther miamipanther approved these changes

@weiling61 weiling61 weiling61 approved these changes

@detiber detiber Awaiting requested review from detiber

@neolit123 neolit123 Awaiting requested review from neolit123

@Fei-Guo Fei-Guo Awaiting requested review from Fei-Guo

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal for ControlPlane Custom Resources
8 participants
@christopherhein @kfox1111 @k8s-ci-robot @charleszheng44 @vincepri @miamipanther @Fei-Guo @weiling61