feat: add CEL validation rules to MemberCluster taint API types#478
feat: add CEL validation rules to MemberCluster taint API types#478Yetkin Timocin (ytimocin) wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Yetkin Timocin (ytimocin)
force-pushed
the
feat/cel-validation-membercluster
branch
12 times, most recently
from
6dfd0c7 to
90e59c0
Compare
There was a problem hiding this comment.
Pull request overview
Adds CRD-level CEL validations for MemberCluster taints (v1 + v1beta1) so invalid taints are rejected at schema validation time, aligning with existing webhook validation for defense-in-depth.
Changes:
- Added CEL rules and length constraints to
MemberClusterSpec.Taints/Taintin both API versions (format, prefix/name/value limits, and uniqueness by(key,value,effect)). - Regenerated the
MemberClusterCRD schema to include the new validations. - Expanded validation coverage via API integration tests (v1 + v1beta1), updated validator unit tests, and adjusted an e2e assertion to match CEL error output.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
apis/cluster/v1/membercluster_types.go |
Adds CEL validations + length constraints for taints in v1 API types. |
apis/cluster/v1beta1/membercluster_types.go |
Adds the same CEL validations + constraints for v1beta1 API types. |
config/crd/bases/cluster.kubernetes-fleet.io_memberclusters.yaml |
CRD schema regeneration to include taint CEL validations and max/min length constraints. |
test/apis/cluster/v1/api_validation_integration_test.go |
Adds integration coverage for valid/invalid taint CEL cases (v1). |
test/apis/cluster/v1beta1/api_validation_integration_test.go |
Adds integration coverage for valid/invalid taint CEL cases (v1beta1). |
pkg/utils/validator/membercluster_test.go |
Adds a unit test covering “same key+effect, different value” as valid (uniqueness is by key+value+effect). |
test/e2e/webhook_test.go |
Updates expected error text to match CEL/CRD validation output. |
| var statusErr *k8sErrors.StatusError | ||
| g.Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update MC call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) | ||
| g.Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character")) | ||
| g.Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("taint key name segment must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character")) |
There was a problem hiding this comment.
This assertion uses MatchRegexp with a literal string that contains regex metacharacters (notably the unescaped "."), which can make the test pass for unintended messages. Consider using ContainSubstring for this check, or wrapping the expected message with regexp.QuoteMeta() before passing it to MatchRegexp to ensure the regex matches the literal text.
Yetkin Timocin (ytimocin)
force-pushed
the
feat/cel-validation-membercluster
branch
2 times, most recently
from
d486583 to
d20fcc6
Compare
Yetkin Timocin (ytimocin)
force-pushed
the
feat/cel-validation-membercluster
branch
5 times, most recently
from
ca856e2 to
f395770
Compare
| // 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") |
There was a problem hiding this comment.
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.
| It("should allow creating MemberCluster with taints having same key and effect but different values", func() { | ||
| mc := &clusterv1.MemberCluster{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "mc-v1-taint-same-key-effect", | ||
| }, |
There was a problem hiding this comment.
This spec is a valid (allowed) case but is placed under the "invalid cases" Context, which makes the test suite harder to understand/maintain. Move this It block into the "valid cases" Context (or rename/split the Contexts to match the actual expectations).
| It("should allow creating MemberCluster with taints having same key and effect but different values", func() { | ||
| mc := &clusterv1beta1.MemberCluster{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "mc-taint-same-key-effect", | ||
| }, | ||
| Spec: clusterv1beta1.MemberClusterSpec{ | ||
| Identity: rbacv1.Subject{ | ||
| Name: "fleet-member-agent-cluster-1", | ||
| Kind: "ServiceAccount", | ||
| Namespace: "fleet-system", | ||
| APIGroup: "", | ||
| }, | ||
| Taints: []clusterv1beta1.Taint{ | ||
| {Key: "key1", Value: "value1", Effect: "NoSchedule"}, | ||
| {Key: "key1", Value: "value2", Effect: "NoSchedule"}, | ||
| }, | ||
| }, | ||
| } | ||
| By("expecting success of CREATE with same key+effect but different values") | ||
| Expect(hubClient.Create(ctx, mc)).Should(Succeed()) | ||
| Expect(hubClient.Delete(ctx, mc)).Should(Succeed()) |
There was a problem hiding this comment.
This spec is a valid (allowed) case but is placed under the "invalid cases" Context, which makes the test suite harder to understand/maintain. Move this It block into the "valid cases" Context (or rename/split the Contexts to match the actual expectations).
| It("should allow creating MemberCluster with taints having same key and effect but different values", func() { | |
| mc := &clusterv1beta1.MemberCluster{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "mc-taint-same-key-effect", | |
| }, | |
| Spec: clusterv1beta1.MemberClusterSpec{ | |
| Identity: rbacv1.Subject{ | |
| Name: "fleet-member-agent-cluster-1", | |
| Kind: "ServiceAccount", | |
| Namespace: "fleet-system", | |
| APIGroup: "", | |
| }, | |
| Taints: []clusterv1beta1.Taint{ | |
| {Key: "key1", Value: "value1", Effect: "NoSchedule"}, | |
| {Key: "key1", Value: "value2", Effect: "NoSchedule"}, | |
| }, | |
| }, | |
| } | |
| By("expecting success of CREATE with same key+effect but different values") | |
| Expect(hubClient.Create(ctx, mc)).Should(Succeed()) | |
| Expect(hubClient.Delete(ctx, mc)).Should(Succeed()) | |
| Context("Test MemberCluster API validation - valid taint cases", func() { | |
| It("should allow creating MemberCluster with taints having same key and effect but different values", func() { | |
| mc := &clusterv1beta1.MemberCluster{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: "mc-taint-same-key-effect", | |
| }, | |
| Spec: clusterv1beta1.MemberClusterSpec{ | |
| Identity: rbacv1.Subject{ | |
| Name: "fleet-member-agent-cluster-1", | |
| Kind: "ServiceAccount", | |
| Namespace: "fleet-system", | |
| APIGroup: "", | |
| }, | |
| Taints: []clusterv1beta1.Taint{ | |
| {Key: "key1", Value: "value1", Effect: "NoSchedule"}, | |
| {Key: "key1", Value: "value2", Effect: "NoSchedule"}, | |
| }, | |
| }, | |
| } | |
| By("expecting success of CREATE with same key+effect but different values") | |
| Expect(hubClient.Create(ctx, mc)).Should(Succeed()) | |
| Expect(hubClient.Delete(ctx, mc)).Should(Succeed()) | |
| }) |
Yetkin Timocin (ytimocin)
force-pushed
the
feat/cel-validation-membercluster
branch
from
f395770 to
9d79ddb
Compare
Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Yetkin Timocin (ytimocin)
force-pushed
the
feat/cel-validation-membercluster
branch
from
9d79ddb to
4229c8e
Compare
|
|
||
| // 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" |
There was a problem hiding this comment.
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)
Description of your changes
Adds CRD CEL validation for
MemberClustertaints in both API versions (v1andv1beta1), replacing the webhook-based taint validation with declarative CRD-level validation.What changed
MemberClusterSpec.TaintsandTaintin:apis/cluster/v1/membercluster_types.goapis/cluster/v1beta1/membercluster_types.go(key, value, effect)MaxLength,MinLength, andMaxItemsconstraints to bound CEL costconfig/crd/bases/cluster.kubernetes-fleet.io_memberclusters.yamlpkg/webhook/membercluster/membercluster_validating_webhook.go— CREATE/UPDATE now returnsAllowedsince CEL handles validation before the webhook is reached. DELETE path (ServiceExport check) is unchanged.pkg/utils/validator/membercluster.go— Deleted (dead code)pkg/utils/validator/membercluster_test.go— Deleted (dead code)test/apis/cluster/v1/api_validation_integration_test.gotest/apis/cluster/v1beta1/api_validation_integration_test.gotest/e2e/webhook_test.goWhy
Breaking change note
Error responses for invalid taints have changed:
403 Forbiddenwith messages fromk8s.io/apimachinery/pkg/util/validation(e.g.,"name part must consist of...")422 Unprocessable Entitywith CEL rule messages (e.g.,"taint key name segment must consist of...")If external tooling parses taint validation error messages or status codes, it will need to be updated.
How has this code been tested
make reviewablepasses cleango test ./test/apis/cluster/v1 -run TestAPIs -count=1 -- -ginkgo.focus="MemberCluster taint CEL validation" -ginkgo.vgo test ./test/apis/cluster/v1beta1 -run TestAPIs -count=1 -- -ginkgo.focus="MemberCluster taint CEL validation" -ginkgo.vSpecial notes for your reviewer
InternalServiceExportresources) that cannot be expressed in CEL.I have:
make reviewableto ensure this PR is ready for review.