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

📖 CAEP: Flexible Managed Kubernetes Endpoints #8500

Merged
merged 5 commits into from
Nov 13, 2023
Merged

📖 CAEP: Flexible Managed Kubernetes Endpoints #8500

merged 5 commits into from
Nov 13, 2023

Conversation

jackfrancis
Copy link
Contributor

@jackfrancis jackfrancis commented Apr 10, 2023 •
edited
Loading

What this PR does / why we need it:

This PR introduces a proposal to make Cluster Infra Resource Optional in Cluster API to aid Managed Kubernetes provider scenarios.

After this PR we will want to create an issue that describes the implementation tasks

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 #7494
References #5295

@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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 10, 2023
fabriziopandini
fabriziopandini reviewed Apr 14, 2023
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

made a quick pass. it looks great so far 🥳

docs/proposals/20230407-optional-cluster-infra-resource.md Outdated Show resolved Hide resolved
@CecileRobertMichon CecileRobertMichon mentioned this pull request May 10, 2023
Merged
@jackfrancis jackfrancis mentioned this pull request May 17, 2023
Merged
@jackfrancis jackfrancis force-pushed the caep-optional-cluster-infra-resource branch from 2723963 to c904d5f Compare May 22, 2023 22:07
@jackfrancis jackfrancis force-pushed the caep-optional-cluster-infra-resource branch from c904d5f to 165d4d4 Compare May 22, 2023 23:54
@jackfrancis jackfrancis changed the title [WIP] 📖 CAEP: Make Cluster Infra Resource Optional 📖 CAEP: Contract Changes to Support Managed Kubernetes May 22, 2023
@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 May 22, 2023
@jackfrancis jackfrancis force-pushed the caep-optional-cluster-infra-resource branch 2 times, most recently from 0a7661d to f5a81be Compare May 23, 2023 00:00
@alexander-demicev
Copy link
Contributor

Simplifying the support for managed Kubernetes is truly a great effort. However, I believe that using a control plane/bootstrap "provider" to handle infrastructure specific options such as regions might deviate from the original concept of a control plane/bootstrap provider where the primary objective of such a provider is to offer an approach for configuring Kubernetes components that operate on the machine.
While I understand that using a "managed" control plane provider is a well-established practice for this use case in CAPI, there is still something that doesn't quite look like this is a truly native solution for CAPI.

fabriziopandini
fabriziopandini reviewed May 30, 2023
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@jackfrancis
thanks for this new iteration of this proposal, IMO It is striking a good balance between surfacing all the context and the discussions in the working group, and focus and clarity on the proposed solution

/lgtm

docs/proposals/20220725-managed-kubernetes.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 1768f6112c6ffbffbdec42b610379d9c4c3a7641

sbueringer
sbueringer reviewed May 30, 2023
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Overall lgtm, just some nits

docs/proposals/20230407-managed-k8s-capi-contract-changes.md Outdated Show resolved Hide resolved
docs/proposals/20230407-managed-k8s-capi-contract-changes.md Outdated Show resolved Hide resolved
docs/proposals/20230407-managed-k8s-capi-contract-changes.md Outdated Show resolved Hide resolved
docs/proposals/20230407-managed-k8s-capi-contract-changes.md Outdated Show resolved Hide resolved
docs/proposals/20230407-managed-k8s-capi-contract-changes.md Outdated Show resolved Hide resolved
@jackfrancis jackfrancis force-pushed the caep-optional-cluster-infra-resource branch from f5a81be to 548b3dd Compare May 30, 2023 16:42
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2023
@jackfrancis jackfrancis force-pushed the caep-optional-cluster-infra-resource branch 2 times, most recently from 1807fe7 to 57c45f1 Compare October 27, 2023 17:02
@jackfrancis
initial draft of modified proposal including new CRD
5c8d5a3 
Signed-off-by: Jack Francis <jackfrancis@gmail.com>
@jackfrancis jackfrancis force-pushed the caep-optional-cluster-infra-resource branch from 57c45f1 to 5c8d5a3 Compare October 31, 2023 16:16
@jackfrancis jackfrancis force-pushed the caep-optional-cluster-infra-resource branch 2 times, most recently from 113dc2e to ded981a Compare November 1, 2023 16:21
@jackfrancis
remove background history
9547470 
Signed-off-by: Jack Francis <jackfrancis@gmail.com>
@jackfrancis jackfrancis force-pushed the caep-optional-cluster-infra-resource branch from ded981a to 9547470 Compare November 1, 2023 16:34
fabriziopandini
fabriziopandini reviewed Nov 2, 2023
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

Great work @jackfrancis
/lgtm

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

LGTM label has been added.

Git tree hash: 1e5da141d911a983061809ab775121311ccd3f99

@jackfrancis
Copy link
Contributor Author

/retest

CecileRobertMichon
CecileRobertMichon reviewed Nov 3, 2023
View reviewed changes
docs/proposals/20230407-flexible-managed-k8s-endpoints.md
// The port on which the endpoint is serving.
Port int32 `json:"port"`

// Cluster is a reference to the cluster name that this endpoint is reachable on.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: ident is off

CecileRobertMichon
CecileRobertMichon reviewed Nov 3, 2023
View reviewed changes
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.

/lgtm

@enxebre
Copy link
Member

enxebre commented Nov 3, 2023

thanks @jackfrancis
/lgtm

@vincepri vincepri mentioned this pull request Nov 8, 2023
Closed
@fabriziopandini
Copy link
Member

lazy consensus deadline passed (Friday before KubeCon)
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 Nov 13, 2023
@fabriziopandini
Copy link
Member

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2023
@k8s-ci-robot k8s-ci-robot merged commit 853f2e1 into kubernetes-sigs:main Nov 13, 2023
18 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Nov 13, 2023
@jackfrancis jackfrancis deleted the caep-optional-cluster-infra-resource branch November 14, 2023 17:49
@MPV MPV mentioned this pull request Nov 15, 2023
Open
@g-gaston
Copy link
Contributor

/area documentation

@k8s-ci-robot k8s-ci-robot added the area/documentation Issues or PRs related to documentation label Nov 21, 2023
@mdbooth mdbooth mentioned this pull request Feb 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardcase richardcase richardcase left review comments

@nrb nrb nrb left review comments

@enxebre enxebre enxebre left review comments

@vincepri vincepri vincepri left review comments

@sbueringer sbueringer sbueringer left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@CecileRobertMichon CecileRobertMichon CecileRobertMichon left review comments

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

@g-gaston g-gaston Awaiting requested review from g-gaston

Assignees

@enxebre enxebre

@sbueringer sbueringer

@fabriziopandini fabriziopandini

@g-gaston g-gaston

@CecileRobertMichon CecileRobertMichon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation Issues or PRs related to documentation 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.

Managed Kubernetes in CAPI