Skip to content

feat: add CEL validation rules to CRP API types#479

Open
Yetkin Timocin (ytimocin) wants to merge 1 commit intokubefleet-dev:mainfrom
ytimocin:feat/cel-validation-placement-crp
Open

feat: add CEL validation rules to CRP API types#479
Yetkin Timocin (ytimocin) wants to merge 1 commit intokubefleet-dev:mainfrom
ytimocin:feat/cel-validation-placement-crp

Conversation

@ytimocin
Copy link
Copy Markdown
Collaborator

@ytimocin Yetkin Timocin (ytimocin) commented Mar 4, 2026

Description of your changes

Adds CRD CEL validation for placement APIs (ClusterResourcePlacement in v1 and v1beta1, plus shared PlacementSpec usage in ResourcePlacement for v1beta1) to match existing webhook validation behavior (defense-in-depth).

What changed

  • API type updates:
    • Added/updated CEL rules in:
      • apis/placement/v1/clusterresourceplacement_types.go
      • apis/placement/v1beta1/clusterresourceplacement_types.go
    • Rules cover:
      • CRP/RP name length validation
      • policy transition/immutability behavior (including PickAll -> nil transition parity)
      • PickAll / PickN / PickFixed policy-shape validation
      • PickFixed cluster name format/length/uniqueness
      • toleration format constraints, uniqueness, and add-only update behavior
      • affinity term restrictions (propertySorter/propertySelector)
      • strategy consistency checks (for example serverSideApplyConfig constraints)
  • Regenerated CRD schemas under config/crd/bases/, including:
    • placement.kubernetes-fleet.io_clusterresourceplacements.yaml
    • placement.kubernetes-fleet.io_resourceplacements.yaml
    • placement.kubernetes-fleet.io_clusterschedulingpolicysnapshots.yaml
    • placement.kubernetes-fleet.io_schedulingpolicysnapshots.yaml
    • plus related generated placement CRDs affected by shared schema generation.
  • Added integration validation coverage:
    • test/apis/placement/v1/api_validation_integration_test.go
    • test/apis/placement/v1beta1/crp_cel_validation_integration_test.go
    • test/apis/placement/v1beta1/api_validation_integration_test.go
    • Added focused valid/invalid CEL transition and toleration cases.
  • Updated e2e assertions:
    • test/e2e/webhook_test.go
    • Adjusted expected messages where CEL now rejects earlier in admission.
  • Webhook logic is unchanged.

Why

  • Enforces invalid placement specs earlier at CRD admission time.
  • Keeps behavior consistent with the current webhook validator.

How has this code been tested

  • make reviewable
  • GOTOOLCHAIN=go1.24.13 make manifests
  • go test ./apis/placement/v1 ./apis/placement/v1beta1 -run '^$'
  • go test ./test/apis/placement/v1 -run '^$'
  • GOCACHE=/tmp/go-build-cache go test ./test/apis/placement/v1beta1 -run '^$'

Special notes for your reviewer

  • Requests failing CEL validation are rejected before reaching the webhook, so some error messages now come from CEL/CRD validation.
  • Full envtest execution was not possible in this environment because /usr/local/kubebuilder/bin/etcd is unavailable.

I have:

  • Run make reviewable to ensure this PR is ready for review.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ytimocin Yetkin Timocin (ytimocin) force-pushed the feat/cel-validation-placement-crp branch 2 times, most recently from 8c42485 to 8984af6 Compare March 4, 2026 18:26
@britaniar
Copy link
Copy Markdown
Member

Might want to hold off on this one. Ryan is working on updating CRP v1 as well as RP to v1.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds CRD-level CEL validation to the placement APIs (CRP v1/v1beta1 and shared PlacementSpec used by RP v1beta1) to mirror existing webhook validation behavior for defense-in-depth, then regenerates CRDs and updates tests to reflect earlier admission rejections and updated messages.

Changes:

  • Added/updated CEL validation rules on placement API types (name length, policy shape/immutability, tolerations, affinity restrictions, strategy constraints).
  • Regenerated CRD schemas under config/crd/bases/ to include the new CEL validations.
  • Added/updated integration + E2E tests to cover/align with CEL validation behavior and message sources.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/e2e/webhook_test.go Updates expected denial messages now that CEL validation rejects earlier than the webhook in some cases.
test/apis/placement/v1beta1/crp_cel_validation_integration_test.go New envtest integration suite covering v1beta1 CRP/RP CEL rule allow/deny and update transition cases.
test/apis/placement/v1beta1/api_validation_integration_test.go Adds an RP allow-case for PickAll→nil policy transition to match updated CEL rules.
test/apis/placement/v1/suite_test.go New envtest suite bootstrap for placement v1 validation integration tests.
test/apis/placement/v1/api_validation_integration_test.go New envtest integration tests for placement v1 CEL validation rules and update transitions.
test/apis/placement/testutils/validation.go Adds a shared helper for asserting validation StatusError messages contain an expected substring.
config/crd/bases/placement.kubernetes-fleet.io_works.yaml CRD regen: adds CEL validation for apply strategy schema where shared types are reused.
config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml CRD regen: propagates shared schema CEL validation into staged update run types.
config/crd/bases/placement.kubernetes-fleet.io_schedulingpolicysnapshots.yaml CRD regen: adds CEL validations/enums/length constraints for shared scheduling policy schema.
config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml CRD regen: adds CEL validations and selector/name constraints for RP schema.
config/crd/bases/placement.kubernetes-fleet.io_resourceoverridesnapshots.yaml CRD regen: propagates shared schema validations into override snapshot CRDs.
config/crd/bases/placement.kubernetes-fleet.io_resourceoverrides.yaml CRD regen: propagates shared schema validations into override CRDs.
config/crd/bases/placement.kubernetes-fleet.io_resourcebindings.yaml CRD regen: propagates shared schema validations into binding CRDs.
config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml CRD regen: propagates shared schema validations into cluster staged update run CRDs.
config/crd/bases/placement.kubernetes-fleet.io_clusterschedulingpolicysnapshots.yaml CRD regen: propagates scheduling policy schema validations into cluster snapshot CRDs.
config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml CRD regen: adds CEL validations for CRP v1/v1beta1 schemas (plus shared schema updates).
config/crd/bases/placement.kubernetes-fleet.io_clusterresourceoverridesnapshots.yaml CRD regen: propagates selector/name mutual exclusivity validation into cluster override snapshots.
config/crd/bases/placement.kubernetes-fleet.io_clusterresourceoverrides.yaml CRD regen: propagates selector/name mutual exclusivity validation into cluster overrides.
config/crd/bases/placement.kubernetes-fleet.io_clusterresourcebindings.yaml CRD regen: propagates shared schema validations into cluster resource binding CRD.
apis/placement/v1beta1/clusterresourceplacement_types.go Adds CEL validations to v1beta1 CRP/RP + shared PlacementSpec/PlacementPolicy structures.
apis/placement/v1/clusterresourceplacement_types.go Adds CEL validations to v1 CRP API types and shared structs used by CRP v1.

@ytimocin Yetkin Timocin (ytimocin) force-pushed the feat/cel-validation-placement-crp branch 2 times, most recently from 4b41506 to 0ce6378 Compare March 6, 2026 21:14
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder why there are no remove logic on the webhook part?

Comment on lines +117 to +118
// +kubebuilder:validation:XValidation:rule="!(has(oldSelf.policy) && !has(self.policy) && oldSelf.policy.placementType != 'PickAll')",message="policy cannot be removed once set"
// +kubebuilder:validation:XValidation:rule="has(oldSelf.policy) || !has(self.policy) || self.policy.placementType == 'PickAll'",message="placement type is immutable"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why is 'PlayAll" singled out here? The error messages don't mention PickAll?

the more readable way for "placement type is immutable"
!has(oldSelf.policy) || !has(self.policy) || oldSelf.policy.placementType == self.policy.placementType

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"placement type is immutable" is also checked by the CEL in the spec , why check it twice?

// If unspecified, all the joined member clusters are selected.
// +kubebuilder:validation:Optional
// +kubebuilder:validation:XValidation:rule="!(self.placementType != oldSelf.placementType)",message="placement type is immutable"
// +kubebuilder:validation:XValidation:rule="self.placementType == oldSelf.placementType",message="placement type is immutable"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

placementType is optional, so you need might need to add !self.placementType

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Comment on lines 1575 to 1579
// The desired state of ResourcePlacement.
// +kubebuilder:validation:Required
// +kubebuilder:validation:XValidation:rule="!(has(oldSelf.policy) && !has(self.policy))",message="policy cannot be removed once set"
// +kubebuilder:validation:XValidation:rule="(has(oldSelf.policy) ? oldSelf.policy.placementType : 'PickAll') == (has(self.policy) ? self.policy.placementType : 'PickAll')",message="placement type is immutable"
Spec PlacementSpec `json:"spec"`
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

The v1 ResourcePlacement type adds spec-level immutability rules, but it still lacks the same metadata.name length CEL rule that was added for v1 ClusterResourcePlacement (and for v1beta1 RP). Because v1 is a served version, clients can bypass the 63-char name constraint by creating/updating via the v1 endpoint unless the v1 RP schema enforces it too. Add the size(self.metadata.name) <= 63 XValidation to the v1 ResourcePlacement type (and regenerate CRDs) to keep validation consistent across versions.

Copilot uses AI. Check for mistakes.
Comment on lines 2737 to 2741
For other types of resources, we consider them as available after `UnavailablePeriodSeconds` seconds
have passed since they were successfully applied to the target cluster.
Default is 60.
minimum: 0
type: integer
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

minimum: 0 was added for unavailablePeriodSeconds in one schema section, but the v1 served version of this CRD still allows negative values (it has no minimum constraint). Since v1 is served, a client can submit an object with negative unavailablePeriodSeconds via v1 and bypass this validation. Ensure the v1 schema also enforces unavailablePeriodSeconds >= 0 (ideally by adding +kubebuilder:validation:Minimum=0 to the v1 API type and regenerating CRDs) so all served versions validate consistently.

Copilot uses AI. Check for mistakes.
@ytimocin Yetkin Timocin (ytimocin) force-pushed the feat/cel-validation-placement-crp branch 2 times, most recently from 181b754 to 6eec521 Compare April 1, 2026 00:41
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@ytimocin Yetkin Timocin (ytimocin) force-pushed the feat/cel-validation-placement-crp branch from 6eec521 to 98e4fb9 Compare April 9, 2026 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: improve reliability by moving away from using webhook

4 participants