⚠️ Add ClusterClass types #4928

fabriziopandini · 2021-07-13T10:00:03Z

What this PR does / why we need it:
This PR add new ClusterClass types/change Cluster API type as defined in the ClusterClass proposal.

Also related web hooks implementation/changes are included in this change set.

Which issue(s) this PR fixes:
Fixes #4908

api/v1alpha4/cluster_types.go

api/v1alpha4/clusterclass_webhooks.go

api/v1alpha4/cluster_types.go

vincepri · 2021-07-13T14:35:10Z

api/v1alpha4/cluster_webhook.go

+		// tolerate version strings without a "v" prefix: prepend it if it's not there
+		if !strings.HasPrefix(c.Spec.Topology.Version, "v") {
+			c.Spec.Topology.Version = "v" + c.Spec.Topology.Version
+		}


Let's remove this assumption and only accept versions with v prefix?

This will make this version field behave differently from the other version fields, I would prefer consistency vs being strict.

@CecileRobertMichon thoughts?

I'm with Fabrizio on this one, consistency is better. But we can open an issue to consider requiring the "v" prefix on all version fields across the API. Although IMO it's nice to not have to worry about it as a user, functionally it's the same.

There is similar toleration currently on Machine and MachineDeployment. We should have a single approach for all version fields. Either all or none!

I'll open an issue to not make it easier for users but rather acknowledge that it should always be prefixed with v. The reason to drop the simplified approach is that we're making an assumption which isn't clear to the user until they get/describe the object. This can also break gitops.

api/v1alpha4/clusterclass_webhooks.go

api/v1alpha4/cluster_webhook.go

api/v1alpha4/clusterclass_webhooks.go

api/v1alpha4/cluster_webhook.go

api/v1alpha4/cluster_types.go

api/v1alpha4/cluster_webhook.go

api/v1alpha4/clusterclass_webhooks.go

srm09

minor nits and questions.

api/v1alpha4/cluster_types.go

srm09 · 2021-07-13T23:23:22Z

api/v1alpha4/cluster_webhook.go

+		// tolerate version strings without a "v" prefix: prepend it if it's not there
+		if !strings.HasPrefix(c.Spec.Topology.Version, "v") {
+			c.Spec.Topology.Version = "v" + c.Spec.Topology.Version
+		}


There is similar toleration currently on Machine and MachineDeployment. We should have a single approach for all version fields. Either all or none!

api/v1alpha4/cluster_webhook.go

api/v1alpha4/clusterclass_webhooks.go

docs/proposals/202105256-cluster-class-and-managed-topologies.md

sbueringer

very nice, just a bunch of nits

api/v1alpha4/cluster_webhook.go

api/v1alpha4/clusterclass_types.go

api/v1alpha4/clusterclass_webhooks.go

api/v1alpha4/clusterclass_webhooks_test.go

fabriziopandini · 2021-07-14T11:50:01Z

All the comments should be addressed
@CecileRobertMichon @srm09 @vincepri I have kept changes to the type names in a separated commit for easier review

api/v1alpha4/cluster_types.go

api/v1alpha4/clusterclass_types.go

srm09

very minor nits, but otherwise lgtm.

docs/proposals/202105256-cluster-class-and-managed-topologies.md

api/v1alpha4/cluster_webhook.go

api/v1alpha4/cluster_webhook_test.go

api/v1alpha4/clusterclass_types.go

api/v1alpha4/clusterclass_webhook.go

feature/feature.go

api/v1alpha4/clusterclass_webhook.go

sbueringer · 2021-07-15T17:56:54Z

Nice!
/lgtm

fabriziopandini · 2021-07-16T19:08:34Z

@vincepri last set of comments are addressed, PR rebased, commit squashed

fabriziopandini · 2021-07-16T19:13:54Z

/test pull-cluster-api-test-main

vincepri · 2021-07-16T19:46:36Z

/retest

vincepri · 2021-07-16T20:31:22Z

/retest

vincepri

/lgtm
/assign @CecileRobertMichon
for approval

CecileRobertMichon

Sorry for the late review, finally got the chance to go through everything

CecileRobertMichon · 2021-07-16T20:55:04Z

PROJECT

@@ -13,6 +13,9 @@ resources:
 - group: cluster
  kind: MachineDeployment
  version: v1alpha3
+- group: cluster
+  kind: ClusterClass


what is our strategy for introducing new CRDs in terms of API versions? We should discuss, there was also #4917 on the topic

seems like it'd make more sense to introduce new APIs at alpha once we move to beta for the rest of the project.

+1 in defining policies, but I think the two usa case have a key difference.
In the case of ClusterClass we are adding a new type to the main CAPI group, so it shares the API version of the existing types (the strategy is implicit here). In the case of the operator instead we are creating a new, separated API group so we have freedom to choose a different API version.

CecileRobertMichon · 2021-07-16T20:57:57Z

api/v1alpha4/cluster_types.go

+	// NOTE: This feature is alpha; it is required to enable the ClusterTopology
+	// feature gate flag to activate managed topologies support.
+	// +optional
+	Topology *Topology `json:"topology,omitempty"`


General question about the PR: does it make master unreleasable? Should we be merging new types without corresponding implementation/controllers since we're not in a "breaking change" development phase anymore and might need to release v0.4.1 any time to fix bugs? Would it be a better strategy to start a new branch for ClusterClass like we did for the provider operator work? Apologies if this was already discussed and I missed it.

General question about the PR: does it make master unreleasable?

No. Master should always be ready for a release.

In order to make it possible while adding ClusterClass and managed topologies on the master branch we are avoiding breaking changes and we are keeping all the new features behind a feature flag, and if the flag is false (which is the default), Cluster API just works as of today with web hooks enforcing you can't set Cluster.Topology or Create/Update ClusterClass types. Also, keeping "strict" isolation between ClusterClass and existing controllers is a driving principle of the next steps of this effort, see e.g. notes on #4933.

+1, as long as webhooks are installed (which we always assume) we should be fine. While the fields show up within the CRDs, enabling the feature gate won't do much more than enabling something non functional.

One thing I'd add into the comment for now is This feature is alpha, highly experimental, and parts of it might still be not implemented

api/v1alpha4/cluster_types.go

CecileRobertMichon

/lgtm

fabriziopandini · 2021-07-19T16:14:13Z

I have updated the comment of Cluster.Spec.Topology as per @vincepri comment (reporting this feature is highly experimental, and parts of it might still be not implemented)

vincepri

/approve
/lgtm

k8s-ci-robot · 2021-07-19T16:15:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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~~ [vincepri]

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

sbueringer · 2021-07-19T16:21:57Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 13, 2021

fabriziopandini requested a review from srm09 July 13, 2021 10:00

k8s-ci-robot requested review from CecileRobertMichon and sbueringer July 13, 2021 10:00

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

schrej reviewed Jul 13, 2021

View reviewed changes

api/v1alpha4/cluster_types.go Outdated Show resolved Hide resolved

schrej reviewed Jul 13, 2021

View reviewed changes

api/v1alpha4/clusterclass_webhooks.go Outdated Show resolved Hide resolved

vincepri reviewed Jul 13, 2021

View reviewed changes

api/v1alpha4/cluster_webhook.go Show resolved Hide resolved

vincepri reviewed Jul 13, 2021

View reviewed changes

api/v1alpha4/cluster_types.go Outdated Show resolved Hide resolved

vincepri reviewed Jul 13, 2021

View reviewed changes

api/v1alpha4/cluster_types.go Outdated Show resolved Hide resolved

sbueringer reviewed Jul 13, 2021

View reviewed changes

api/v1alpha4/cluster_webhook.go Outdated Show resolved Hide resolved

api/v1alpha4/clusterclass_webhooks.go Outdated Show resolved Hide resolved

srm09 reviewed Jul 14, 2021

View reviewed changes

This was referenced Jul 14, 2021

Should we support downgrades in ManagedTopologies? #4939

Closed

Additional validation checks for ClusterClass and managed topologies #4940

Closed

sbueringer reviewed Jul 14, 2021

View reviewed changes

enxebre reviewed Jul 14, 2021

View reviewed changes

api/v1alpha4/cluster_types.go Outdated Show resolved Hide resolved

enxebre reviewed Jul 14, 2021

View reviewed changes

api/v1alpha4/cluster_types.go Show resolved Hide resolved

enxebre reviewed Jul 14, 2021

View reviewed changes

api/v1alpha4/cluster_types.go Show resolved Hide resolved

enxebre reviewed Jul 14, 2021

View reviewed changes

api/v1alpha4/clusterclass_types.go Show resolved Hide resolved

srm09 reviewed Jul 14, 2021

View reviewed changes

docs/proposals/202105256-cluster-class-and-managed-topologies.md Outdated Show resolved Hide resolved

docs/proposals/202105256-cluster-class-and-managed-topologies.md Outdated Show resolved Hide resolved

fabriziopandini force-pushed the clusterclass-api-types branch 2 times, most recently from 118809d to 1d9c337 Compare July 15, 2021 09:40

sbueringer reviewed Jul 15, 2021

View reviewed changes

api/v1alpha4/clusterclass_webhook.go Outdated Show resolved Hide resolved

fabriziopandini force-pushed the clusterclass-api-types branch from 15d095a to d46babc Compare July 15, 2021 17:55

k8s-ci-robot assigned sbueringer Jul 15, 2021

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

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

fabriziopandini force-pushed the clusterclass-api-types branch from f021a62 to 38fc771 Compare July 16, 2021 19:08

k8s-ci-robot assigned CecileRobertMichon Jul 16, 2021

vincepri approved these changes Jul 16, 2021

View reviewed changes

k8s-ci-robot assigned vincepri Jul 16, 2021

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

CecileRobertMichon reviewed Jul 16, 2021

View reviewed changes

fabriziopandini mentioned this pull request Jul 19, 2021

Investigate if we should enable additional linters (through golangci-lint) #4622

Closed

9 tasks

fabriziopandini force-pushed the clusterclass-api-types branch from 38fc771 to d027c3c Compare July 19, 2021 12:00

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

fabriziopandini force-pushed the clusterclass-api-types branch from d027c3c to 20c3c7e Compare July 19, 2021 15:11

CecileRobertMichon reviewed Jul 19, 2021

View reviewed changes

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

Add ClusterClass types

fb67f57

fabriziopandini force-pushed the clusterclass-api-types branch from 20c3c7e to fb67f57 Compare July 19, 2021 16:13

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

vincepri approved these changes Jul 19, 2021

View reviewed changes

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

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

k8s-ci-robot merged commit d9a16c9 into kubernetes-sigs:master Jul 19, 2021

k8s-ci-robot added this to the v0.4 milestone Jul 19, 2021

fabriziopandini deleted the clusterclass-api-types branch July 19, 2021 18:24

vincepri mentioned this pull request Jul 22, 2021

🌱 Align ClusterClass ControlPlane with the rest of the fields #5000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Add ClusterClass types #4928

⚠️ Add ClusterClass types #4928

fabriziopandini commented Jul 13, 2021

vincepri Jul 13, 2021

fabriziopandini Jul 13, 2021

vincepri Jul 13, 2021

CecileRobertMichon Jul 13, 2021

srm09 Jul 13, 2021

vincepri Jul 16, 2021

srm09 left a comment

srm09 Jul 13, 2021

sbueringer left a comment

fabriziopandini commented Jul 14, 2021

srm09 left a comment

sbueringer commented Jul 15, 2021

fabriziopandini commented Jul 16, 2021

fabriziopandini commented Jul 16, 2021

vincepri commented Jul 16, 2021

vincepri commented Jul 16, 2021

vincepri left a comment

CecileRobertMichon left a comment

CecileRobertMichon Jul 16, 2021

fabriziopandini Jul 19, 2021 •

edited

Loading

CecileRobertMichon Jul 16, 2021

fabriziopandini Jul 19, 2021 •

edited

Loading

vincepri Jul 19, 2021

CecileRobertMichon left a comment

fabriziopandini commented Jul 19, 2021

vincepri left a comment

k8s-ci-robot commented Jul 19, 2021

sbueringer commented Jul 19, 2021

⚠️ Add ClusterClass types #4928

⚠️ Add ClusterClass types #4928

Conversation

fabriziopandini commented Jul 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srm09 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer left a comment

Choose a reason for hiding this comment

fabriziopandini commented Jul 14, 2021

srm09 left a comment

Choose a reason for hiding this comment

sbueringer commented Jul 15, 2021

fabriziopandini commented Jul 16, 2021

fabriziopandini commented Jul 16, 2021

vincepri commented Jul 16, 2021

vincepri commented Jul 16, 2021

vincepri left a comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

fabriziopandini commented Jul 19, 2021

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 19, 2021

sbueringer commented Jul 19, 2021

fabriziopandini Jul 19, 2021 •

edited

Loading

fabriziopandini Jul 19, 2021 •

edited

Loading