Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions apis/cluster/v1/membercluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ type MemberClusterSpec struct {
//
// This field is beta-level and is for the taints and tolerations feature.
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:XValidation:rule="self.all(t, self.filter(u, u.key == t.key && (has(u.value) ? u.value : '') == (has(t.value) ? t.value : '') && u.effect == t.effect).size() == 1)",message="taints must be unique"
// +optional
Taints []Taint `json:"taints,omitempty"`
}
Expand Down Expand Up @@ -136,12 +137,21 @@ type MemberClusterStatus struct {

// Taint attached to MemberCluster has the "effect" on
// any ClusterResourcePlacement that does not tolerate the Taint.
// +kubebuilder:validation:XValidation:rule="(self.key.contains('/') ? self.key.substring(self.key.indexOf('/') + 1) : self.key).matches('^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$')",message="taint key name segment must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character"

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.

The CEL validation for name and label seem too complex ? Currently we depend on validation functions provided by k8s which guarantee that we are following established conventions once this CEL validation is merged we would also need to maintain consistency between our validation and k8s (very unlikely but still something to keep in mind)

// +kubebuilder:validation:XValidation:rule="!self.key.contains('/') || self.key.substring(0, self.key.indexOf('/')).matches('^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(\\\\.[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)*$')",message="taint key prefix must be a lowercase DNS subdomain"
// +kubebuilder:validation:XValidation:rule="(self.key.contains('/') ? self.key.size() - self.key.indexOf('/') - 1 : self.key.size()) <= 63",message="taint key name segment must be 63 characters or less"
// +kubebuilder:validation:XValidation:rule="!self.key.contains('/') || self.key.indexOf('/') <= 253",message="taint key prefix must be 253 characters or less"
// +kubebuilder:validation:XValidation:rule="!has(self.value) || size(self.value) == 0 || self.value.matches('^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$')",message="taint value must be a valid label value"
type Taint struct {
// The taint key to be applied to a MemberCluster.
// MaxLength is 253 (prefix) + 1 (slash) + 63 (name segment) = 317.
// +kubebuilder:validation:MaxLength=317
// +kubebuilder:validation:MinLength=1
// +required
Key string `json:"key"`

// The taint value corresponding to the taint key.
// +kubebuilder:validation:MaxLength=63
// +optional
Value string `json:"value,omitempty"`

Expand Down
10 changes: 10 additions & 0 deletions apis/cluster/v1beta1/membercluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ type MemberClusterSpec struct {
//
// This field is beta-level and is for the taints and tolerations feature.
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:XValidation:rule="self.all(t, self.filter(u, u.key == t.key && (has(u.value) ? u.value : '') == (has(t.value) ? t.value : '') && u.effect == t.effect).size() == 1)",message="taints must be unique"
// +optional
Taints []Taint `json:"taints,omitempty"`

Expand Down Expand Up @@ -156,12 +157,21 @@ type MemberClusterStatus struct {

// Taint attached to MemberCluster has the "effect" on
// any ClusterResourcePlacement that does not tolerate the Taint.
// +kubebuilder:validation:XValidation:rule="(self.key.contains('/') ? self.key.substring(self.key.indexOf('/') + 1) : self.key).matches('^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$')",message="taint key name segment must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character"
// +kubebuilder:validation:XValidation:rule="!self.key.contains('/') || self.key.substring(0, self.key.indexOf('/')).matches('^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(\\\\.[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)*$')",message="taint key prefix must be a lowercase DNS subdomain"
// +kubebuilder:validation:XValidation:rule="(self.key.contains('/') ? self.key.size() - self.key.indexOf('/') - 1 : self.key.size()) <= 63",message="taint key name segment must be 63 characters or less"
// +kubebuilder:validation:XValidation:rule="!self.key.contains('/') || self.key.indexOf('/') <= 253",message="taint key prefix must be 253 characters or less"
// +kubebuilder:validation:XValidation:rule="!has(self.value) || size(self.value) == 0 || self.value.matches('^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$')",message="taint value must be a valid label value"
type Taint struct {
// The taint key to be applied to a MemberCluster.
// MaxLength is 253 (prefix) + 1 (slash) + 63 (name segment) = 317.
// +kubebuilder:validation:MaxLength=317
// +kubebuilder:validation:MinLength=1
// +required
Key string `json:"key"`

// The taint value corresponding to the taint key.
// +kubebuilder:validation:MaxLength=63
// +optional
Value string `json:"value,omitempty"`

Expand Down
56 changes: 54 additions & 2 deletions config/crd/bases/cluster.kubernetes-fleet.io_memberclusters.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -128,17 +128,43 @@ spec:
- NoSchedule
type: string
key:
description: The taint key to be applied to a MemberCluster.
description: |-
The taint key to be applied to a MemberCluster.
MaxLength is 253 (prefix) + 1 (slash) + 63 (name segment) = 317.
maxLength: 317
minLength: 1
type: string
value:
description: The taint value corresponding to the taint key.
maxLength: 63
type: string
required:
- effect
- key
type: object
x-kubernetes-validations:
- message: taint key name segment must consist of alphanumeric characters,
'-', '_' or '.', and must start and end with an alphanumeric
character
rule: '(self.key.contains(''/'') ? self.key.substring(self.key.indexOf(''/'')
+ 1) : self.key).matches(''^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$'')'
- message: taint key prefix must be a lowercase DNS subdomain
rule: '!self.key.contains(''/'') || self.key.substring(0, self.key.indexOf(''/'')).matches(''^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)*$'')'
- message: taint key name segment must be 63 characters or less
rule: '(self.key.contains(''/'') ? self.key.size() - self.key.indexOf(''/'')
- 1 : self.key.size()) <= 63'
- message: taint key prefix must be 253 characters or less
rule: '!self.key.contains(''/'') || self.key.indexOf(''/'') <=
253'
- message: taint value must be a valid label value
rule: '!has(self.value) || size(self.value) == 0 || self.value.matches(''^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$'')'
maxItems: 100
type: array
x-kubernetes-validations:
- message: taints must be unique
rule: 'self.all(t, self.filter(u, u.key == t.key && (has(u.value)
? u.value : '''') == (has(t.value) ? t.value : '''') && u.effect
== t.effect).size() == 1)'
required:
- identity
type: object
Expand Down Expand Up @@ -500,17 +526,43 @@ spec:
- NoSchedule
type: string
key:
description: The taint key to be applied to a MemberCluster.
description: |-
The taint key to be applied to a MemberCluster.
MaxLength is 253 (prefix) + 1 (slash) + 63 (name segment) = 317.
maxLength: 317
minLength: 1
type: string
value:
description: The taint value corresponding to the taint key.
maxLength: 63
type: string
required:
- effect
- key
type: object
x-kubernetes-validations:
- message: taint key name segment must consist of alphanumeric characters,
'-', '_' or '.', and must start and end with an alphanumeric
character
rule: '(self.key.contains(''/'') ? self.key.substring(self.key.indexOf(''/'')
+ 1) : self.key).matches(''^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$'')'
- message: taint key prefix must be a lowercase DNS subdomain
rule: '!self.key.contains(''/'') || self.key.substring(0, self.key.indexOf(''/'')).matches(''^[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?)*$'')'
- message: taint key name segment must be 63 characters or less
rule: '(self.key.contains(''/'') ? self.key.size() - self.key.indexOf(''/'')
- 1 : self.key.size()) <= 63'
- message: taint key prefix must be 253 characters or less
rule: '!self.key.contains(''/'') || self.key.indexOf(''/'') <=
253'
- message: taint value must be a valid label value
rule: '!has(self.value) || size(self.value) == 0 || self.value.matches(''^[a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])?$'')'
maxItems: 100
type: array
x-kubernetes-validations:
- message: taints must be unique
rule: 'self.all(t, self.filter(u, u.key == t.key && (has(u.value)
? u.value : '''') == (has(t.value) ? t.value : '''') && u.effect
== t.effect).size() == 1)'
required:
- identity
type: object
Expand Down
41 changes: 0 additions & 41 deletions pkg/utils/validator/membercluster.go

This file was deleted.

100 changes: 0 additions & 100 deletions pkg/utils/validator/membercluster_test.go

This file was deleted.

13 changes: 3 additions & 10 deletions pkg/webhook/membercluster/membercluster_validating_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (

clusterv1beta1 "github.com/kubefleet-dev/kubefleet/apis/cluster/v1beta1"
"github.com/kubefleet-dev/kubefleet/pkg/utils"
"github.com/kubefleet-dev/kubefleet/pkg/utils/validator"

fleetnetworkingv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1"
)
Expand Down Expand Up @@ -78,14 +77,8 @@ func (v *memberClusterValidator) Handle(ctx context.Context, req admission.Reque
return admission.Allowed("Member cluster is ready to leave")
}

if err := v.decoder.Decode(req, &mc); err != nil {
klog.ErrorS(err, "Failed to decode member cluster object for validating fields", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups)
return admission.Errored(http.StatusBadRequest, err)
}

if err := validator.ValidateMemberCluster(mc); err != nil {
klog.V(2).ErrorS(err, "Member cluster has invalid fields, request is denied", "operation", req.Operation, "memberCluster", mcObjectName)
return admission.Denied(err.Error())
}
// Taint validation is now handled by CRD-level CEL validation rules.
// CREATE/UPDATE requests with invalid taints are rejected at schema
// validation time before reaching this webhook.
return admission.Allowed("Member cluster has valid fields")
Comment on lines +80 to 83

Copilot AI Apr 1, 2026

Copy link

Choose a reason for hiding this comment

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

The webhook now short-circuits CREATE/UPDATE, but the validating webhook is still configured (in pkg/webhook/webhook.go) with FailurePolicy=Fail and Operations={Create,Update,Delete} for MemberCluster. That means webhook unavailability will still block MemberCluster CREATE/UPDATE even though taint validation moved to CEL, which undermines the stated goal of removing the webhook dependency for taint validation. Consider updating the webhook configuration to only target DELETE (or set FailurePolicy=Ignore for CREATE/UPDATE) now that only DELETE performs cross-object validation.

Copilot uses AI. Check for mistakes.
}
Loading
Loading