✨ Introduce CEL for ClusterClass Variables #9239

chaunceyjiang · 2023-08-18T07:00:31Z

What this PR does / why we need it:

Introduce CEL for ClusterClass Variables.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Part of : #8565

/area clusterclass

k8s-ci-robot · 2023-08-18T07:00:40Z

Hi @chaunceyjiang. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

killianmuldoon

/ok-to-test

killianmuldoon · 2023-08-18T08:47:19Z

Looks like you'll need to regenerate the associated files

chaunceyjiang · 2023-08-18T09:58:33Z

@killianmuldoon Hello, may I ask if we want to separate the API from its implementation into two PRs or keep them in one PR?

killianmuldoon · 2023-08-18T12:45:32Z

Hello, may I ask if we want to separate the API from its implementation into two PRs or keep them in one PR?

Best to keep them in the one PR IMO - but maybe keep them to seperate commits. You might want to wait for thorough reviews of the API initially to ensure the implementation doesn't need to be changed regularly.

k8s-ci-robot · 2023-08-20T14:53:22Z

@chaunceyjiang: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-test-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-dualstack-and-ipv6-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-scale-main-experimental
/test pull-cluster-api-e2e-workload-upgrade-1-28-latest-main
/test pull-cluster-api-test-mink8s-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-informing-main
pull-cluster-api-e2e-main
pull-cluster-api-test-main
pull-cluster-api-verify-main

In response to this:

/test pull-cluster-api-verify-mai

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.

chaunceyjiang · 2023-08-21T10:47:37Z

@killianmuldoon @sbueringer Hi, The API part is ready for review.

richardcase · 2023-08-24T12:19:15Z

I'd also favour the same PR but 2 commits.

@chaunceyjiang - it might be worth marking this as WIP if there will be a second commit with the implementation.

richardcase

This will be really useful 🎉

api/v1beta1/clusterclass_types.go

sbueringer · 2023-08-25T07:03:56Z

+1 to just one PR. Just that we don't end up with an API without implementation (assuming the PR doesn't become huge)

Will try to review soon, but my PR backlog is just too big right now

jimmidyson · 2023-09-07T08:40:42Z

Excited to see this PR land (with implementation)! @chaunceyjiang Anything I can do to help out?

chaunceyjiang · 2023-09-07T08:45:19Z

@jimmidyson Hi, thank you very much. I am waiting for the API parts review.

Once there is an LGTM, I will start implementing it. Can you help review the API part?

jimmidyson

A few comments, but overall looks good to me, nice work. I think there could be some changes once we get to integrating the CEL validation so I also like the idea of implementation as another commit in this PR rather than as a follow-up.

api/v1beta1/clusterclass_types.go

sbueringer

Did a first review, while we probably want to refine the godoc a bit later I think the current state (+ fixes for the findings) is easily good enough to start the implementation. I would focus on implementing the validation functionality for variables.

In a later step / PR we can introduce additional validation in the ClusterClass webhook for the new fields in the ClusterClass

api/v1beta1/clusterclass_types.go

api/v1beta1/common_types.go

api/v1beta1/clusterclass_types.go

jimmidyson · 2023-10-11T11:49:06Z

@chaunceyjiang Any news on this PR? Great work so far btw!

chaunceyjiang · 2023-10-11T14:01:57Z

@jimmidyson I am very sorry for delaying this PR. In the following time, I will continue to complete this PR.

sbueringer · 2024-06-24T14:42:50Z

/lgtm

k8s-ci-robot · 2024-06-24T14:42:58Z

LGTM label has been added.

Git tree hash: 0b5403bd2eea4ce494489fc5cff3315eed8663da

sbueringer · 2024-06-24T14:51:00Z

/test pull-cluster-api-e2e-conformance-ci-latest-main
/test pull-cluster-api-e2e-conformance-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-30-1-31-main

sbueringer · 2024-06-24T14:51:09Z

Thx everyone so far, really nice work :)

/assign @chrischdi @fabriziopandini

k8s-ci-robot · 2024-06-24T16:36:20Z

@chaunceyjiang: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	`8834caa`	link	false	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

sbueringer · 2024-06-24T16:43:06Z

/test pull-cluster-api-e2e-mink8s-main

fabriziopandini · 2024-06-24T19:13:03Z

@chaunceyjiang @jimmidyson @sbueringer awesome work!
I'm impressed by the quality of the work, detailed comments. top class test coverage, and by the attention to user experience/validation and error messages

/lgtm

chrischdi · 2024-06-27T08:22:09Z

internal/topology/variables/clusterclass_variable_validation.go

+	if len(validationErrors) > 0 {
+		for _, validationError := range validationErrors {
+			// Add context to the error message.
+			validationError.Detail = fmt.Sprintf("failed to convert schema definition for variable %q; ClusterClass should be checked: %s", clusterClassVariable.Name, validationError.Detail) // TODO: consider if to add ClusterClass name


Should we resolve this todo or remove it before merging? (Same below)

Nope, it's a copy & paste from a lot of other places (has been there since forever). Let's solve this independently

chrischdi · 2024-06-27T08:52:46Z

internal/topology/variables/clusterclass_variable_validation.go

 	}

 	return allErrs
 }
+
+// The following funcs are all duplicated from upstream CRD validation at
+// https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.0/pkg/apis/apiextensions/validation/validation.go#L1317.


nit: line number for the link does not seem to be correct :-)

Suggested change

// https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.0/pkg/apis/apiextensions/validation/validation.go#L1317.

// https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.0/pkg/apis/apiextensions/validation/validation.go#L107.

Let's get rid of the line number. These are copied from all over this file

I'll take this as a follow-up. I have a few follow-up PRs queued up anyway

chrischdi · 2024-06-27T09:37:16Z

Really awesome work! just two nits from my side, but also okay for me to handle them in a follow-up :-)

chrischdi · 2024-06-27T10:53:35Z

/approve

/hold
in case of you want to wait for other reviews

k8s-ci-robot · 2024-06-27T10:53:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrischdi

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

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

sbueringer · 2024-06-27T10:54:34Z

Thx!

Again, really great work everyone!!

/hold cancel

jimmidyson · 2024-06-27T11:21:06Z

Thank you for all the help getting this in, especially to @sbueringer who has been a superstar in reviewing, guiding, and getting improvements/fixes in 🙏

sbueringer · 2024-06-27T12:20:19Z

Thx, appreciate it 😃

k8s-ci-robot added area/clusterclass Issues or PRs related to clusterclass cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 18, 2023

k8s-ci-robot requested review from stmcginnis and ykakarap August 18, 2023 07:00

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 18, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 18, 2023

killianmuldoon reviewed Aug 18, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 18, 2023

chaunceyjiang force-pushed the crd branch from aee549b to 6daa29e Compare August 18, 2023 09:54

chaunceyjiang force-pushed the crd branch 3 times, most recently from 3c0f205 to f3ef3b7 Compare August 21, 2023 10:27

chaunceyjiang changed the title ~~✨ [WIP] Introduce CEL for ClusterClass Variables~~ ✨ Introduce CEL for ClusterClass Variables Aug 21, 2023

richardcase reviewed Aug 24, 2023

View reviewed changes

api/v1beta1/clusterclass_types.go Show resolved Hide resolved

api/v1beta1/clusterclass_types.go Show resolved Hide resolved

chaunceyjiang force-pushed the crd branch from f3ef3b7 to ef1a7e4 Compare August 25, 2023 06:08

jimmidyson reviewed Sep 7, 2023

View reviewed changes

api/v1beta1/clusterclass_types.go Outdated Show resolved Hide resolved

api/v1beta1/clusterclass_types.go Show resolved Hide resolved

api/v1beta1/clusterclass_types.go Outdated Show resolved Hide resolved

sbueringer reviewed Sep 7, 2023

View reviewed changes

resolve compile issue after rebase

c47d989

jimmidyson force-pushed the crd branch from 785cc5b to c47d989 Compare June 24, 2024 12:40

Some more improvements (#4)

8834caa

k8s-ci-robot assigned sbueringer Jun 24, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 24, 2024

k8s-ci-robot assigned chrischdi and fabriziopandini Jun 24, 2024

sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jun 25, 2024

chrischdi reviewed Jun 27, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 27, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2024

k8s-ci-robot merged commit ada2764 into kubernetes-sigs:main Jun 27, 2024
26 of 27 checks passed

k8s-ci-robot added this to the v1.8 milestone Jun 27, 2024

sbueringer mentioned this pull request Jun 28, 2024

✨ Add variable validation to ClusterClass controller, block Cluster reconcile if variables not reconciled #10812

Merged

sbueringer mentioned this pull request Nov 11, 2024

Explore CEL validation for ClusterClass variables #8565

Closed

	// https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.0/pkg/apis/apiextensions/validation/validation.go#L1317.
	// https://github.com/kubernetes/apiextensions-apiserver/blob/v0.30.0/pkg/apis/apiextensions/validation/validation.go#L107.

✨ Introduce CEL for ClusterClass Variables #9239

✨ Introduce CEL for ClusterClass Variables #9239

Conversation

chaunceyjiang commented Aug 18, 2023

k8s-ci-robot commented Aug 18, 2023

killianmuldoon left a comment

Choose a reason for hiding this comment

killianmuldoon commented Aug 18, 2023

chaunceyjiang commented Aug 18, 2023

killianmuldoon commented Aug 18, 2023

k8s-ci-robot commented Aug 20, 2023

chaunceyjiang commented Aug 21, 2023

richardcase commented Aug 24, 2023

richardcase left a comment

Choose a reason for hiding this comment

sbueringer commented Aug 25, 2023 • edited Loading

jimmidyson commented Sep 7, 2023

chaunceyjiang commented Sep 7, 2023

jimmidyson left a comment

Choose a reason for hiding this comment

sbueringer left a comment • edited Loading

Choose a reason for hiding this comment

jimmidyson commented Oct 11, 2023

chaunceyjiang commented Oct 11, 2023

sbueringer commented Jun 24, 2024

k8s-ci-robot commented Jun 24, 2024

sbueringer commented Jun 24, 2024

sbueringer commented Jun 24, 2024

k8s-ci-robot commented Jun 24, 2024 • edited Loading

sbueringer commented Jun 24, 2024

fabriziopandini commented Jun 24, 2024

chrischdi Jun 27, 2024

Choose a reason for hiding this comment

sbueringer Jun 27, 2024

Choose a reason for hiding this comment

chrischdi Jun 27, 2024

Choose a reason for hiding this comment

chrischdi Jun 27, 2024

Choose a reason for hiding this comment

sbueringer Jun 27, 2024

Choose a reason for hiding this comment

sbueringer Jun 27, 2024

Choose a reason for hiding this comment

chrischdi commented Jun 27, 2024

chrischdi commented Jun 27, 2024

k8s-ci-robot commented Jun 27, 2024

sbueringer commented Jun 27, 2024

jimmidyson commented Jun 27, 2024

sbueringer commented Jun 27, 2024

sbueringer commented Aug 25, 2023 •

edited

Loading

sbueringer left a comment •

edited

Loading

k8s-ci-robot commented Jun 24, 2024 •

edited

Loading