✨ Enable ClusterClass rebase operation #5644

killianmuldoon · 2021-11-11T16:00:30Z

This change removes a check in the Cluster Webhook which ensure a cluster can't be moved from one ClusterClass to another. Additional compatibility checks have been added to ensure that the resulting update to the Cluster is protected from some unreconcilable changes as outlined in the clusterClass proposal #4985

Fixes #5318

webhooks/cluster.go

sbueringer

I like the direction in which this is going :)

A few nits for now. I'll take a closer look later on, but would be good to get the underlying PR merged.

controllers/topology/blueprint_test.go

internal/topology/check/compatibility.go

webhooks/cluster.go

sbueringer

@killianmuldoon Can you please rebase the PR on top of main?

I'll do another review then.

controllers/topology/cluster_controller_test.go

internal/topology/check/compatibility_test.go

webhooks/cluster.go

killianmuldoon · 2021-11-17T09:21:14Z

@sbueringer rebased now.

controllers/topology/blueprint_test.go

internal/topology/check/compatibility.go

webhooks/cluster.go

controllers/topology/cluster_controller_test.go

sbueringer

Looks great. Mostly nits from my side. I skipped compatibility_test.go and cluster_test.go. I'll do that on the next round, but there will be at most a few nits in those files.

controllers/topology/blueprint_test.go

controllers/topology/cluster_controller_test.go

internal/topology/check/compatibility.go

webhooks/cluster.go

internal/topology/check/compatibility_test.go

webhooks/cluster_test.go

killianmuldoon · 2021-11-17T16:46:41Z

webhooks/cluster.go

+	// On update.
+	if old != nil {
+		// Error if the update moves the cluster from Managed to Unmanaged i.e. the managed topology is removed on update.
+		if old.Spec.Topology != nil && new.Spec.Topology == nil {


Check for managed -> unmanaged

webhooks/cluster.go

controllers/topology/cluster_controller_test.go

internal/topology/check/compatibility.go

sbueringer · 2021-11-18T08:22:33Z

internal/topology/check/compatibility.go

+	}
+
+	for i, mdc := range clusterClass.Spec.Workers.MachineDeployments {
+		allErrs = append(allErrs, LocalObjectTemplateIsValid(&mdc.Template.Bootstrap, clusterClass.Namespace, field.NewPath("spec", "workers", "machineDeployments").Index(i).Child("template", "bootstrap"))...)


(I wouldn't change anything in this PR, just realized it now)
Shouldn't the bootstrap template be optional? (based on that it's optional in MDs)

// ConfigRef is a reference to a bootstrap provider-specific resource
// that holds configuration details. The reference is optional to
// allow users/operators to specify Bootstrap.DataSecretName without
// the need of a controller.

Or is that something not supported by ClusterClass?

cc @fabriziopandini Is this something we have to think about / create an issue for?

I don't think so.
it is kind of impossible automatically creating clusters without a bootstrap provider, given that it is impractical or impossible to provide bootstrap data secrets at ClusterClass level

Okay sounds good. Pretty relieved that we don't have to support optional BootstrapTemplates

webhooks/cluster.go

webhooks/cluster_test.go

fabriziopandini · 2021-11-18T16:00:18Z

/lgtm
@vincepri PTAL

sbueringer · 2021-11-18T16:44:43Z

Looks great, thx!

lgtm pending squash

sbueringer · 2021-11-18T16:50:27Z

/lgtm

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>

sbueringer · 2021-11-18T16:55:46Z

/lgtm

killianmuldoon · 2021-11-18T16:57:12Z

@sbueringer sorry for the ping pong over and back! 😄

sbueringer · 2021-11-18T17:04:41Z

@sbueringer sorry for the ping pong over and back! 😄

No problem. I was just starting to write a comment about the state you had in between when I saw the new version :)

fabriziopandini · 2021-11-19T09:58:30Z

/lgtm

fabriziopandini · 2021-11-22T13:05:43Z

/lgtm
/approve
this blocking some work in this area and this week most of the folks in the US are out, so I'm approving to keep the work moving (BTW this already has two lgtm), and then we should probably have a code walkthrough of al cluster/clusterclass validation

k8s-ci-robot · 2021-11-22T13:06:03Z

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

~~OWNERS~~ [fabriziopandini]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 11, 2021

k8s-ci-robot requested review from CecileRobertMichon and sbueringer November 11, 2021 16:00

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 11, 2021

killianmuldoon commented Nov 11, 2021

View reviewed changes

webhooks/cluster.go Show resolved Hide resolved

killianmuldoon force-pushed the webhook/add-cluster-webhook-checks branch 4 times, most recently from 58116fe to 8f9f3a4 Compare November 11, 2021 18:12

sbueringer reviewed Nov 12, 2021

View reviewed changes

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

killianmuldoon force-pushed the webhook/add-cluster-webhook-checks branch from 8f9f3a4 to 04f4be8 Compare November 16, 2021 15:13

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

killianmuldoon changed the title ~~✨ [WIP] Enable ClusterClass rebase operation~~ ✨ Enable ClusterClass rebase operation Nov 16, 2021

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

sbueringer mentioned this pull request Nov 17, 2021

✨ Add ClusterClass changes e2e test #5664

Merged

sbueringer reviewed Nov 17, 2021

View reviewed changes

controllers/topology/cluster_controller_test.go Outdated Show resolved Hide resolved

internal/topology/check/compatibility_test.go Outdated Show resolved Hide resolved

webhooks/cluster.go Outdated Show resolved Hide resolved

killianmuldoon force-pushed the webhook/add-cluster-webhook-checks branch from 04f4be8 to 4ee61d4 Compare November 17, 2021 09:20

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

killianmuldoon force-pushed the webhook/add-cluster-webhook-checks branch 2 times, most recently from 622a83d to 53d3c26 Compare November 17, 2021 11:33

fabriziopandini reviewed Nov 17, 2021

View reviewed changes

sbueringer reviewed Nov 17, 2021

View reviewed changes

killianmuldoon commented Nov 17, 2021

View reviewed changes

sbueringer reviewed Nov 17, 2021

View reviewed changes

sbueringer reviewed Nov 18, 2021

View reviewed changes

killianmuldoon mentioned this pull request Nov 18, 2021

Add spec comparison to topology integration tests #5699

Closed

killianmuldoon force-pushed the webhook/add-cluster-webhook-checks branch from 05b5077 to 7937ad9 Compare November 18, 2021 13:52

k8s-ci-robot assigned fabriziopandini Nov 18, 2021

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

killianmuldoon force-pushed the webhook/add-cluster-webhook-checks branch from 7937ad9 to 09b577f Compare November 18, 2021 16:49

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2021

k8s-ci-robot assigned sbueringer Nov 18, 2021

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

ClusterClass rebase

2ffa35b

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>

killianmuldoon force-pushed the webhook/add-cluster-webhook-checks branch from 09b577f to 2ffa35b Compare November 18, 2021 16:52

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2021

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

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

k8s-ci-robot merged commit 9b13261 into kubernetes-sigs:main Nov 22, 2021

k8s-ci-robot added this to the v1.1 milestone Nov 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Enable ClusterClass rebase operation #5644

✨ Enable ClusterClass rebase operation #5644

killianmuldoon commented Nov 11, 2021 •

edited

sbueringer left a comment

sbueringer left a comment

killianmuldoon commented Nov 17, 2021

sbueringer left a comment •

edited

killianmuldoon Nov 17, 2021

sbueringer Nov 18, 2021

sbueringer Nov 18, 2021

fabriziopandini Nov 18, 2021

sbueringer Nov 18, 2021

fabriziopandini commented Nov 18, 2021

sbueringer commented Nov 18, 2021

sbueringer commented Nov 18, 2021

sbueringer commented Nov 18, 2021

killianmuldoon commented Nov 18, 2021

sbueringer commented Nov 18, 2021

fabriziopandini commented Nov 19, 2021

fabriziopandini commented Nov 22, 2021

k8s-ci-robot commented Nov 22, 2021

✨ Enable ClusterClass rebase operation #5644

✨ Enable ClusterClass rebase operation #5644

Conversation

killianmuldoon commented Nov 11, 2021 • edited

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer left a comment

Choose a reason for hiding this comment

killianmuldoon commented Nov 17, 2021

sbueringer left a comment • edited

Choose a reason for hiding this comment

killianmuldoon Nov 17, 2021

Choose a reason for hiding this comment

sbueringer Nov 18, 2021

Choose a reason for hiding this comment

sbueringer Nov 18, 2021

Choose a reason for hiding this comment

fabriziopandini Nov 18, 2021

Choose a reason for hiding this comment

sbueringer Nov 18, 2021

Choose a reason for hiding this comment

fabriziopandini commented Nov 18, 2021

sbueringer commented Nov 18, 2021

sbueringer commented Nov 18, 2021

sbueringer commented Nov 18, 2021

killianmuldoon commented Nov 18, 2021

sbueringer commented Nov 18, 2021

fabriziopandini commented Nov 19, 2021

fabriziopandini commented Nov 22, 2021

k8s-ci-robot commented Nov 22, 2021

killianmuldoon commented Nov 11, 2021 •

edited

sbueringer left a comment •

edited