✨ feature: migrate structural validation to kubebuilder CEL markers (Phase 1) - #444
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Vaishnav88sk The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughThe PR adds an Alpha CEL validation feature gate and CRD constraints for non-empty manifests, HTTPS client URLs, ManagedCluster names, client-config list size, and matching ManagedClusterSetBinding names. An integration test covers rejection of a hostless HTTPS URL. ChangesCRD validation updates
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yamlTraceback (most recent call last): addon/v1beta1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yamlTraceback (most recent call last): cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yamlTraceback (most recent call last):
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cluster/v1/types.go`:
- Around line 86-87: Replace the URL validation marker on the URL field in
cluster/v1/types.go:86-87 with a CEL expression that performs real URL
validation and requires an HTTPS scheme, rejecting malformed values such as
https://. Regenerate
cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml:111-113
so its validation rule and message match the updated source marker.
In `@work/v1/types.go`:
- Around line 69-71: Make Manifests required in work/v1/types.go by removing its
optional marker while retaining the non-empty validation. Regenerate the CRDs so
the required-field schema is reflected in
work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml, both
listed addon/v1alpha1 and addon/v1beta1 AddonTemplates locations, the second
addon/v1beta1 location, and
work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml;
no other schema changes are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faef0d51-045c-4ee7-8c02-f0ba16262ca3
📒 Files selected for processing (9)
addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yamladdon/v1beta1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yamlcluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yamlcluster/v1/types.gocluster/v1beta2/0000_01_clusters.open-cluster-management.io_managedclustersetbindings.crd.yamlcluster/v1beta2/types_managedclustersetbinding.gowork/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yamlwork/v1/types.gowork/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml
|
please add integration tests to test all the validations. |
f9ddf0f to
2153948
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
work/v1/types.go (1)
69-71: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake
manifestsrequired.The
+optionalmarker andomitemptystill allow the field to be omitted, sosize(self) > 0does not enforce the intended invariant for absent values. Remove the optional marker and regenerate the affected CRDs. (kubernetes.io)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@work/v1/types.go` around lines 69 - 71, Make Manifests required by removing its +optional marker and omitempty JSON tag, while retaining the non-empty validation rule. Regenerate the affected CRD manifests so the generated schemas reflect the required field and size constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cluster/v1/types.go`:
- Around line 87-88: Update the XValidation rule on the URL field to require
url(self).getHostname() != '' instead of checking getHost(), preserving the
existing HTTPS and valid-URL requirements. Add a regression test covering
https://:6443 and verify it is rejected.
In `@feature/feature.go`:
- Around line 108-110: Use the existing featuregate.Feature symbol CELValidation
to gate the CRD-side CEL rollout: conditionally install CEL validation rules and
ValidatingAdmissionPolicy resources, or retain the legacy validating webhook
when the gate is disabled. Update the relevant CRD/policy installation and
webhook-removal paths so CELValidation=false preserves legacy behavior.
In `@test/integration/api/managedcluster_test.go`:
- Around line 151-168: The ManagedCluster integration coverage is missing
validation for the new name and client-config count constraints. Extend the
managed-cluster creation tests alongside the existing HTTPS host validation case
to cover an invalid metadata.name, acceptance of exactly 32
ManagedClusterClientConfigs, and rejection of 33 configs, asserting the expected
create success or validation error for each case.
---
Duplicate comments:
In `@work/v1/types.go`:
- Around line 69-71: Make Manifests required by removing its +optional marker
and omitempty JSON tag, while retaining the non-empty validation rule.
Regenerate the affected CRD manifests so the generated schemas reflect the
required field and size constraint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b5635bd-7728-413a-81ba-b4e022dc2c14
📒 Files selected for processing (9)
addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yamladdon/v1beta1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yamlcluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yamlcluster/v1/types.gofeature/feature.gotest/integration/api/managedcluster_test.gowork/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yamlwork/v1/types.gowork/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml
💤 Files with no reviewable changes (4)
- work/v1/0000_00_work.open-cluster-management.io_manifestworks.crd.yaml
- work/v1alpha1/0000_00_work.open-cluster-management.io_manifestworkreplicasets.crd.yaml
- addon/v1alpha1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yaml
- addon/v1beta1/0000_03_addon.open-cluster-management.io_addontemplates.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- cluster/v1/0000_00_clusters.open-cluster-management.io_managedclusters.crd.yaml
| // +kubebuilder:validation:MaxLength=2048 | ||
| // +kubebuilder:validation:XValidation:rule="isURL(self) && url(self).getScheme() == 'https' && url(self).getHost() != ''",message="url must be a valid https URL" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the hostname, not only the host string.
url(self).getHost() != '' can accept https://:6443: the host string contains a port, but the hostname is empty. Use getHostname() != '' and add a regression test for this case. Kubernetes distinguishes getHost() from getHostname(). (kubernetes.io)
[details]
- // +kubebuilder:validation:XValidation:rule="isURL(self) && url(self).getScheme() == 'https' && url(self).getHost() != ''",message="url must be a valid https URL"
+ // +kubebuilder:validation:XValidation:rule="isURL(self) && url(self).getScheme() == 'https' && url(self).getHostname() != ''",message="url must be a valid https URL"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cluster/v1/types.go` around lines 87 - 88, Update the XValidation rule on the
URL field to require url(self).getHostname() != '' instead of checking
getHost(), preserving the existing HTTPS and valid-URL requirements. Add a
regression test covering https://:6443 and verify it is rejected.
| // CELValidation replaces the legacy Go Validating Webhooks with native Kubernetes | ||
| // CEL validation rules and ValidatingAdmissionPolicy bindings. | ||
| CELValidation featuregate.Feature = "CELValidation" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '(^|/)feature/feature\.go$|CELValidation|ValidatingAdmissionPolicy|XValidation|CEL validation|legacy Go Validating Webhooks|webhook'Repository: open-cluster-management-io/api
Length of output: 3097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- feature/feature.go ---'
sed -n '1,220p' feature/feature.go
echo
echo '--- CELValidation references ---'
rg -n 'CELValidation' .Repository: open-cluster-management-io/api
Length of output: 10322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -i 'CELValidation|ValidatingAdmissionPolicy|x-kubernetes-validations|xValidation|validating webhook|legacy webhook|admission policy' . --glob '!vendor/**'Repository: open-cluster-management-io/api
Length of output: 3459
Gate the CRD-side CEL rollout on CELValidation
feature/feature.go:108-110 only registers the flag; the repo already ships x-kubernetes-validations in generated CRDs, so CELValidation=false does not preserve the legacy webhook path by itself. Tie CRD/policy installation or webhook removal to this gate if old behavior still needs to be supported.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@feature/feature.go` around lines 108 - 110, Use the existing
featuregate.Feature symbol CELValidation to gate the CRD-side CEL rollout:
conditionally install CEL validation rules and ValidatingAdmissionPolicy
resources, or retain the legacy validating webhook when the gate is disabled.
Update the relevant CRD/policy installation and webhook-removal paths so
CELValidation=false preserves legacy behavior.
| ginkgo.It("should reject HTTPS URL without host", func() { | ||
| managedCluster := &clusterv1.ManagedCluster{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: clusterName, | ||
| }, | ||
| Spec: clusterv1.ManagedClusterSpec{ | ||
| HubAcceptsClient: true, | ||
| ManagedClusterClientConfigs: []clusterv1.ClientConfig{ | ||
| { | ||
| URL: "https://", | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| _, err := hubClusterClient.ClusterV1().ManagedClusters().Create(context.TODO(), managedCluster, metav1.CreateOptions{}) | ||
| gomega.Expect(err).To(gomega.HaveOccurred()) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover all new ManagedCluster constraints.
This test covers only the hostless HTTPS case. Add integration cases for invalid metadata.name, exactly 32 client configs, and rejection of 33 configs so the generated schema constraints are exercised end to end.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/integration/api/managedcluster_test.go` around lines 151 - 168, The
ManagedCluster integration coverage is missing validation for the new name and
client-config count constraints. Extend the managed-cluster creation tests
alongside the existing HTTPS host validation case to cover an invalid
metadata.name, acceptance of exactly 32 ManagedClusterClientConfigs, and
rejection of 33 configs, asserting the expected create success or validation
error for each case.
- migrate structural validation to kubebuilder CEL markers - address CodeRabbit feedback on required fields and URL validation - add CELValidation FeatureGate - address PR feedback for CEL rules and Manifests Signed-off-by: Vaishnav88sk <vaishnavsk8804@gmail.com>
2153948 to
5fc06ed
Compare
3 participants
Summary
This PR implements Phase 1 of the CEL Validation Migration as outlined and approved in Enhancement Proposal #181.
It replaces legacy Go-based structural webhook validation with native Kubernetes
+kubebuilder:validation:XValidationCEL markers directly on the Custom Resource structs.Validations Migrated:
ManagedCluster: Enforced HTTPS format forSpec.ManagedClusterClientConfigsURLs and regex format formetadata.name.ManagedClusterSetBinding: Enforced cross-field parity (metadata.name == spec.clusterSet).ManifestWork: Enforcedmanifestsarray is not empty.Note: The corresponding
ValidatingAdmissionPolicyadditions and FeatureGate operator updates (Phase 2) will be submitted in a follow-up PR to theocmrepository once this API change is merged.Related issue(s)
Fixes phase 1 of open-cluster-management-io/ocm#1566
Summary by CodeRabbit
Summary by CodeRabbit
New Features
CELValidationfeature gate to enable native CEL-based API validation.Bug Fixes
manifestsfor add-on templates, manifest works, and manifest replica sets.ManagedClustervalidation: DNS-compatible name (≤63 chars),managedClusterClientConfigsmax 32, andclientConfigs[].urlmust be a validhttpsURL with non-empty host (≤2048 chars).ManagedClusterSetBindingnames matchspec.clusterSet.Tests
ManagedClusterclient URLs.