Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add CEL rules to ResourceFlavor #1958

Merged
merged 5 commits into from
Apr 11, 2024
Merged

Add CEL rules to ResourceFlavor #1958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6516dc6
Add CEL rules to ResourceFlavor
IrvingMg Apr 9, 2024
139e9f3
Update comments
IrvingMg Apr 10, 2024
06daa97
Add issue reference to TODO comment
IrvingMg Apr 10, 2024
aa9b099
Group similar test cases with DescribeTable
IrvingMg Apr 11, 2024
a73a0c0
Update validation rule
IrvingMg Apr 11, 2024
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
6 changes: 6 additions & 0 deletions apis/kueue/v1beta1/resourceflavor_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ type ResourceFlavorSpec struct {
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=8
// +kubebuilder:validation:XValidation:rule="self.all(x, x.effect in ['NoSchedule', 'PreferNoSchedule', 'NoExecute'])", message="supported taint effect values: 'NoSchedule', 'PreferNoSchedule', 'NoExecute'"
NodeTaints []corev1.Taint `json:"nodeTaints,omitempty"`
alculquicondor marked this conversation as resolved.
Show resolved Hide resolved

// tolerations are extra tolerations that will be added to the pods admitted in
Expand All @@ -78,6 +79,11 @@ type ResourceFlavorSpec struct {
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=8
// +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.key) ? x.operator == 'Exists' : true)", message="operator must be Exists when 'key' is empty, which means 'match all values and all keys'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for discussion, from maintenance, do you guys think this is more simple comparing to the codes.

Copy link
Contributor

Choose a reason for hiding this comment

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

From maintenance POV it depends on how the rules look, From a runtime POV if we are able to fully drop the webhook for a type (like it was the case for local queue) then it's a win.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense, apply the same policy through the whole project is important to avoid confusion, unless we can't fully remove the validation.

// +kubebuilder:validation:XValidation:rule="self.all(x, has(x.tolerationSeconds) ? x.effect == 'NoExecute' : true)", message="effect must be 'NoExecute' when 'tolerationSeconds' is set"
// +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.operator) || x.operator in ['Equal', 'Exists'])", message="supported toleration values: 'Equal'(default), 'Exists'"
// +kubebuilder:validation:XValidation:rule="self.all(x, has(x.operator) && x.operator == 'Exists' ? !has(x.value) : true)", message="a value must be empty when 'operator' is 'Exists'"
// +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.effect) || x.effect in ['NoSchedule', 'PreferNoSchedule', 'NoExecute'])", message="supported taint effect values: 'NoSchedule', 'PreferNoSchedule', 'NoExecute'"
Tolerations []corev1.Toleration `json:"tolerations,omitempty"`
}

Expand Down
22 changes: 22 additions & 0 deletions charts/kueue/templates/crd/kueue.x-k8s.io_resourceflavors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ spec:
maxItems: 8
type: array
x-kubernetes-list-type: atomic
x-kubernetes-validations:
- message: 'supported taint effect values: ''NoSchedule'', ''PreferNoSchedule'',
''NoExecute'''
rule: self.all(x, x.effect in ['NoSchedule', 'PreferNoSchedule',
'NoExecute'])
tolerations:
description: |-
tolerations are extra tolerations that will be added to the pods admitted in
Expand Down Expand Up @@ -168,6 +173,23 @@ spec:
maxItems: 8
type: array
x-kubernetes-list-type: atomic
x-kubernetes-validations:
- message: operator must be Exists when 'key' is empty, which means
'match all values and all keys'
rule: 'self.all(x, !has(x.key) ? x.operator == ''Exists'' : true)'
- message: effect must be 'NoExecute' when 'tolerationSeconds' is
set
rule: 'self.all(x, has(x.tolerationSeconds) ? x.effect == ''NoExecute''
: true)'
- message: 'supported toleration values: ''Equal''(default), ''Exists'''
rule: self.all(x, !has(x.operator) || x.operator in ['Equal', 'Exists'])
- message: a value must be empty when 'operator' is 'Exists'
rule: 'self.all(x, has(x.operator) && x.operator == ''Exists'' ?
!has(x.value) : true)'
- message: 'supported taint effect values: ''NoSchedule'', ''PreferNoSchedule'',
''NoExecute'''
rule: self.all(x, !has(x.effect) || x.effect in ['NoSchedule', 'PreferNoSchedule',
'NoExecute'])
type: object
type: object
served: true
Expand Down
22 changes: 22 additions & 0 deletions config/components/crd/bases/kueue.x-k8s.io_resourceflavors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ spec:
maxItems: 8
type: array
x-kubernetes-list-type: atomic
x-kubernetes-validations:
- message: 'supported taint effect values: ''NoSchedule'', ''PreferNoSchedule'',
''NoExecute'''
rule: self.all(x, x.effect in ['NoSchedule', 'PreferNoSchedule',
'NoExecute'])
tolerations:
description: |-
tolerations are extra tolerations that will be added to the pods admitted in
Expand Down Expand Up @@ -153,6 +158,23 @@ spec:
maxItems: 8
type: array
x-kubernetes-list-type: atomic
x-kubernetes-validations:
- message: operator must be Exists when 'key' is empty, which means
'match all values and all keys'
rule: 'self.all(x, !has(x.key) ? x.operator == ''Exists'' : true)'
- message: effect must be 'NoExecute' when 'tolerationSeconds' is
set
rule: 'self.all(x, has(x.tolerationSeconds) ? x.effect == ''NoExecute''
: true)'
- message: 'supported toleration values: ''Equal''(default), ''Exists'''
rule: self.all(x, !has(x.operator) || x.operator in ['Equal', 'Exists'])
- message: a value must be empty when 'operator' is 'Exists'
rule: 'self.all(x, has(x.operator) && x.operator == ''Exists'' ?
!has(x.value) : true)'
- message: 'supported taint effect values: ''NoSchedule'', ''PreferNoSchedule'',
''NoExecute'''
rule: self.all(x, !has(x.effect) || x.effect in ['NoSchedule', 'PreferNoSchedule',
'NoExecute'])
type: object
type: object
served: true
Expand Down
3 changes: 1 addition & 2 deletions pkg/webhooks/resourceflavor_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,6 @@ func validateNodeTaints(taints []corev1.Taint, fldPath *field.Path) field.ErrorL
if errs := validation.IsValidLabelValue(currTaint.Value); len(errs) != 0 {
allErrors = append(allErrors, field.Invalid(idxPath.Child("value"), currTaint.Value, strings.Join(errs, ";")))
}
// validate the taint effect
allErrors = append(allErrors, validateTaintEffect(&currTaint.Effect, false, idxPath.Child("effect"))...)

// validate if taint is unique by <key, effect>
if len(uniqueTaints[currTaint.Effect]) > 0 && uniqueTaints[currTaint.Effect].Has(currTaint.Key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can replace uniqueness checks by using +listType=map and adding the appropriate keys to use.

But we can leave that for a follow up PR.

Expand All @@ -131,6 +129,7 @@ func validateNodeTaints(taints []corev1.Taint, fldPath *field.Path) field.ErrorL
return allErrors
}

// TODO(#463): Remove this function when CEL validations are added to workload type
// validateTaintEffect is extracted from git.k8s.io/kubernetes/pkg/apis/core/validation/validation.go
func validateTaintEffect(effect *corev1.TaintEffect, allowEmpty bool, fldPath *field.Path) field.ErrorList {
if !allowEmpty && len(*effect) == 0 {
Expand Down
10 changes: 0 additions & 10 deletions pkg/webhooks/resourceflavor_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,6 @@ func TestValidateResourceFlavor(t *testing.T) {
Effect: corev1.TaintEffectNoSchedule,
}).Obj(),
},
{
// Taint validation is not exhaustively tested, because the code was copied from upstream k8s.
name: "invalid taint",
rf: utiltesting.MakeResourceFlavor("resource-flavor").Taint(corev1.Taint{
Key: "skdajf",
}).Obj(),
wantErr: field.ErrorList{
field.Required(field.NewPath("spec", "nodeTaints").Index(0).Child("effect"), ""),
},
},
{
name: "invalid label name",
rf: utiltesting.MakeResourceFlavor("resource-flavor").Label("@abc", "foo").Obj(),
Expand Down
96 changes: 80 additions & 16 deletions test/integration/webhook/resourceflavor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,24 @@ var _ = ginkgo.Describe("ResourceFlavor Webhook", func() {
})

ginkgo.When("Creating a ResourceFlavor", func() {
ginkgo.It("Should be valid", func() {
resourceFlavor := testing.MakeResourceFlavor("resource-flavor").Label("foo", "bar").
Taint(corev1.Taint{
Key: "spot",
Value: "true",
Effect: corev1.TaintEffectNoSchedule,
}).Obj()
gomega.Expect(k8sClient.Create(ctx, resourceFlavor)).Should(gomega.Succeed())
defer func() {
var rf kueue.ResourceFlavor
gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(resourceFlavor), &rf)).Should(gomega.Succeed())
controllerutil.RemoveFinalizer(&rf, kueue.ResourceInUseFinalizerName)
gomega.Expect(k8sClient.Update(ctx, &rf)).Should(gomega.Succeed())
util.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, resourceFlavor, true)
}()
})
ginkgo.It("Should have a finalizer", func() {
ginkgo.By("Creating a new resourceFlavor")
ginkgo.By("Creating a new empty resourceFlavor")
resourceFlavor := testing.MakeResourceFlavor("resource-flavor").Obj()
gomega.Expect(k8sClient.Create(ctx, resourceFlavor)).Should(gomega.Succeed())
defer func() {
Expand All @@ -69,20 +85,6 @@ var _ = ginkgo.Describe("ResourceFlavor Webhook", func() {
})
})

ginkgo.When("Creating a ResourceFlavor with invalid taints", func() {
ginkgo.It("Should fail to create", func() {
ginkgo.By("Creating a new resourceFlavor")
resourceFlavor := testing.MakeResourceFlavor("resource-flavor").Taint(corev1.Taint{
Key: "@foo",
Value: "bar",
Effect: corev1.TaintEffectNoSchedule,
}).Obj()
err := k8sClient.Create(ctx, resourceFlavor)
gomega.Expect(err).To(gomega.HaveOccurred())
gomega.Expect(errors.IsForbidden(err)).To(gomega.BeTrue(), "error: %v", err)
})
})

ginkgo.DescribeTable("invalid number of properties", func(taintsCount int, nodeSelectorCount int, isInvalid bool) {
rf := testing.MakeResourceFlavor("resource-flavor")
for i := 0; i < taintsCount; i++ {
Expand Down Expand Up @@ -139,7 +141,69 @@ var _ = ginkgo.Describe("ResourceFlavor Webhook", func() {
ginkgo.By("Updating the resourceFlavor with invalid labels")
err := k8sClient.Update(ctx, &created)
gomega.Expect(err).To(gomega.HaveOccurred())
gomega.Expect(errors.IsForbidden(err)).To(gomega.BeTrue(), "error: %v", err)
gomega.Expect(err).Should(testing.BeAPIError(testing.InvalidError))
})
})

ginkgo.DescribeTable("Validate resourceFlavor on creation", func(rf *kueue.ResourceFlavor, errorType int) {
err := k8sClient.Create(ctx, rf)
if err == nil {
defer func() {
util.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, rf, true)
}()
}
switch errorType {
case isForbidden:
gomega.Expect(err).Should(gomega.HaveOccurred())
gomega.Expect(err).Should(testing.BeAPIError(testing.ForbiddenError), "error: %v", err)
case isInvalid:
gomega.Expect(err).Should(gomega.HaveOccurred())
gomega.Expect(err).Should(testing.BeAPIError(testing.InvalidError), "error: %v", err)
default:
gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
}
},
ginkgo.Entry("Should fail to create with invalid taints",
testing.MakeResourceFlavor("resource-flavor").
Taint(corev1.Taint{
Key: "skdajf",
}).
Taint(corev1.Taint{
Key: "@foo",
Value: "bar",
Effect: corev1.TaintEffectNoSchedule,
}).Obj(),
isInvalid),
ginkgo.Entry("Should fail to create with invalid label name",
testing.MakeResourceFlavor("resource-flavor").Label("@abc", "foo").Obj(),
isForbidden),
ginkgo.Entry("Should fail to create with invalid tolerations",
testing.MakeResourceFlavor("resource-flavor").
Toleration(corev1.Toleration{
Key: "@abc",
Operator: corev1.TolerationOpEqual,
Value: "v",
Effect: corev1.TaintEffectNoSchedule,
}).
Toleration(corev1.Toleration{
Key: "abc",
Operator: corev1.TolerationOpExists,
Value: "v",
Effect: corev1.TaintEffectNoSchedule,
}).
Toleration(corev1.Toleration{
Key: "abc",
Operator: corev1.TolerationOpEqual,
Value: "v",
Effect: corev1.TaintEffect("not-valid"),
}).
Toleration(corev1.Toleration{
Key: "abc",
Operator: corev1.TolerationOpEqual,
Value: "v",
Effect: corev1.TaintEffectNoSchedule,
}).
Obj(),
isInvalid),
)
})