Skip to content

Commit e9da2db

Browse files
IrvingMgvsoch
IrvingMg
authored and
vsoch
committed
Add CEL rules to ResourceFlavor (kubernetes-sigs#1958)
* Add CEL rules to ResourceFlavor * Update comments * Add issue reference to TODO comment * Group similar test cases with DescribeTable * Update validation rule
1 parent d29d456 commit e9da2db

File tree

6 files changed

+131
-28
lines changed

6 files changed

+131
-28
lines changed

‎apis/kueue/v1beta1/resourceflavor_types.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ type ResourceFlavorSpec struct {
6565
// +optional
6666
// +listType=atomic
6767
// +kubebuilder:validation:MaxItems=8
68+
// +kubebuilder:validation:XValidation:rule="self.all(x, x.effect in ['NoSchedule', 'PreferNoSchedule', 'NoExecute'])", message="supported taint effect values: 'NoSchedule', 'PreferNoSchedule', 'NoExecute'"
6869
NodeTaints []corev1.Taint `json:"nodeTaints,omitempty"`
6970

7071
// tolerations are extra tolerations that will be added to the pods admitted in
@@ -78,6 +79,11 @@ type ResourceFlavorSpec struct {
7879
// +optional
7980
// +listType=atomic
8081
// +kubebuilder:validation:MaxItems=8
82+
// +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'"
83+
// +kubebuilder:validation:XValidation:rule="self.all(x, has(x.tolerationSeconds) ? x.effect == 'NoExecute' : true)", message="effect must be 'NoExecute' when 'tolerationSeconds' is set"
84+
// +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.operator) || x.operator in ['Equal', 'Exists'])", message="supported toleration values: 'Equal'(default), 'Exists'"
85+
// +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'"
86+
// +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.effect) || x.effect in ['NoSchedule', 'PreferNoSchedule', 'NoExecute'])", message="supported taint effect values: 'NoSchedule', 'PreferNoSchedule', 'NoExecute'"
8187
Tolerations []corev1.Toleration `json:"tolerations,omitempty"`
8288
}
8389

‎charts/kueue/templates/crd/kueue.x-k8s.io_resourceflavors.yaml‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,11 @@ spec:
118118
maxItems: 8
119119
type: array
120120
x-kubernetes-list-type: atomic
121+
x-kubernetes-validations:
122+
- message: 'supported taint effect values: ''NoSchedule'', ''PreferNoSchedule'',
123+
''NoExecute'''
124+
rule: self.all(x, x.effect in ['NoSchedule', 'PreferNoSchedule',
125+
'NoExecute'])
121126
tolerations:
122127
description: |-
123128
tolerations are extra tolerations that will be added to the pods admitted in
@@ -168,6 +173,23 @@ spec:
168173
maxItems: 8
169174
type: array
170175
x-kubernetes-list-type: atomic
176+
x-kubernetes-validations:
177+
- message: operator must be Exists when 'key' is empty, which means
178+
'match all values and all keys'
179+
rule: 'self.all(x, !has(x.key) ? x.operator == ''Exists'' : true)'
180+
- message: effect must be 'NoExecute' when 'tolerationSeconds' is
181+
set
182+
rule: 'self.all(x, has(x.tolerationSeconds) ? x.effect == ''NoExecute''
183+
: true)'
184+
- message: 'supported toleration values: ''Equal''(default), ''Exists'''
185+
rule: self.all(x, !has(x.operator) || x.operator in ['Equal', 'Exists'])
186+
- message: a value must be empty when 'operator' is 'Exists'
187+
rule: 'self.all(x, has(x.operator) && x.operator == ''Exists'' ?
188+
!has(x.value) : true)'
189+
- message: 'supported taint effect values: ''NoSchedule'', ''PreferNoSchedule'',
190+
''NoExecute'''
191+
rule: self.all(x, !has(x.effect) || x.effect in ['NoSchedule', 'PreferNoSchedule',
192+
'NoExecute'])
171193
type: object
172194
type: object
173195
served: true

‎config/components/crd/bases/kueue.x-k8s.io_resourceflavors.yaml‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,11 @@ spec:
103103
maxItems: 8
104104
type: array
105105
x-kubernetes-list-type: atomic
106+
x-kubernetes-validations:
107+
- message: 'supported taint effect values: ''NoSchedule'', ''PreferNoSchedule'',
108+
''NoExecute'''
109+
rule: self.all(x, x.effect in ['NoSchedule', 'PreferNoSchedule',
110+
'NoExecute'])
106111
tolerations:
107112
description: |-
108113
tolerations are extra tolerations that will be added to the pods admitted in
@@ -153,6 +158,23 @@ spec:
153158
maxItems: 8
154159
type: array
155160
x-kubernetes-list-type: atomic
161+
x-kubernetes-validations:
162+
- message: operator must be Exists when 'key' is empty, which means
163+
'match all values and all keys'
164+
rule: 'self.all(x, !has(x.key) ? x.operator == ''Exists'' : true)'
165+
- message: effect must be 'NoExecute' when 'tolerationSeconds' is
166+
set
167+
rule: 'self.all(x, has(x.tolerationSeconds) ? x.effect == ''NoExecute''
168+
: true)'
169+
- message: 'supported toleration values: ''Equal''(default), ''Exists'''
170+
rule: self.all(x, !has(x.operator) || x.operator in ['Equal', 'Exists'])
171+
- message: a value must be empty when 'operator' is 'Exists'
172+
rule: 'self.all(x, has(x.operator) && x.operator == ''Exists'' ?
173+
!has(x.value) : true)'
174+
- message: 'supported taint effect values: ''NoSchedule'', ''PreferNoSchedule'',
175+
''NoExecute'''
176+
rule: self.all(x, !has(x.effect) || x.effect in ['NoSchedule', 'PreferNoSchedule',
177+
'NoExecute'])
156178
type: object
157179
type: object
158180
served: true

‎pkg/webhooks/resourceflavor_webhook.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,6 @@ func validateNodeTaints(taints []corev1.Taint, fldPath *field.Path) field.ErrorL
111111
if errs := validation.IsValidLabelValue(currTaint.Value); len(errs) != 0 {
112112
allErrors = append(allErrors, field.Invalid(idxPath.Child("value"), currTaint.Value, strings.Join(errs, ";")))
113113
}
114-
// validate the taint effect
115-
allErrors = append(allErrors, validateTaintEffect(&currTaint.Effect, false, idxPath.Child("effect"))...)
116114

117115
// validate if taint is unique by <key, effect>
118116
if len(uniqueTaints[currTaint.Effect]) > 0 && uniqueTaints[currTaint.Effect].Has(currTaint.Key) {
@@ -131,6 +129,7 @@ func validateNodeTaints(taints []corev1.Taint, fldPath *field.Path) field.ErrorL
131129
return allErrors
132130
}
133131

132+
// TODO(#463): Remove this function when CEL validations are added to workload type
134133
// validateTaintEffect is extracted from git.k8s.io/kubernetes/pkg/apis/core/validation/validation.go
135134
func validateTaintEffect(effect *corev1.TaintEffect, allowEmpty bool, fldPath *field.Path) field.ErrorList {
136135
if !allowEmpty && len(*effect) == 0 {

‎pkg/webhooks/resourceflavor_webhook_test.go‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,6 @@ func TestValidateResourceFlavor(t *testing.T) {
4949
Effect: corev1.TaintEffectNoSchedule,
5050
}).Obj(),
5151
},
52-
{
53-
// Taint validation is not exhaustively tested, because the code was copied from upstream k8s.
54-
name: "invalid taint",
55-
rf: utiltesting.MakeResourceFlavor("resource-flavor").Taint(corev1.Taint{
56-
Key: "skdajf",
57-
}).Obj(),
58-
wantErr: field.ErrorList{
59-
field.Required(field.NewPath("spec", "nodeTaints").Index(0).Child("effect"), ""),
60-
},
61-
},
6252
{
6353
name: "invalid label name",
6454
rf: utiltesting.MakeResourceFlavor("resource-flavor").Label("@abc", "foo").Obj(),

‎test/integration/webhook/resourceflavor_test.go‎

Lines changed: 80 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,24 @@ var _ = ginkgo.Describe("ResourceFlavor Webhook", func() {
5151
})
5252

5353
ginkgo.When("Creating a ResourceFlavor", func() {
54+
ginkgo.It("Should be valid", func() {
55+
resourceFlavor := testing.MakeResourceFlavor("resource-flavor").Label("foo", "bar").
56+
Taint(corev1.Taint{
57+
Key: "spot",
58+
Value: "true",
59+
Effect: corev1.TaintEffectNoSchedule,
60+
}).Obj()
61+
gomega.Expect(k8sClient.Create(ctx, resourceFlavor)).Should(gomega.Succeed())
62+
defer func() {
63+
var rf kueue.ResourceFlavor
64+
gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(resourceFlavor), &rf)).Should(gomega.Succeed())
65+
controllerutil.RemoveFinalizer(&rf, kueue.ResourceInUseFinalizerName)
66+
gomega.Expect(k8sClient.Update(ctx, &rf)).Should(gomega.Succeed())
67+
util.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, resourceFlavor, true)
68+
}()
69+
})
5470
ginkgo.It("Should have a finalizer", func() {
55-
ginkgo.By("Creating a new resourceFlavor")
71+
ginkgo.By("Creating a new empty resourceFlavor")
5672
resourceFlavor := testing.MakeResourceFlavor("resource-flavor").Obj()
5773
gomega.Expect(k8sClient.Create(ctx, resourceFlavor)).Should(gomega.Succeed())
5874
defer func() {
@@ -69,20 +85,6 @@ var _ = ginkgo.Describe("ResourceFlavor Webhook", func() {
6985
})
7086
})
7187

72-
ginkgo.When("Creating a ResourceFlavor with invalid taints", func() {
73-
ginkgo.It("Should fail to create", func() {
74-
ginkgo.By("Creating a new resourceFlavor")
75-
resourceFlavor := testing.MakeResourceFlavor("resource-flavor").Taint(corev1.Taint{
76-
Key: "@foo",
77-
Value: "bar",
78-
Effect: corev1.TaintEffectNoSchedule,
79-
}).Obj()
80-
err := k8sClient.Create(ctx, resourceFlavor)
81-
gomega.Expect(err).To(gomega.HaveOccurred())
82-
gomega.Expect(errors.IsForbidden(err)).To(gomega.BeTrue(), "error: %v", err)
83-
})
84-
})
85-
8688
ginkgo.DescribeTable("invalid number of properties", func(taintsCount int, nodeSelectorCount int, isInvalid bool) {
8789
rf := testing.MakeResourceFlavor("resource-flavor")
8890
for i := 0; i < taintsCount; i++ {
@@ -139,7 +141,69 @@ var _ = ginkgo.Describe("ResourceFlavor Webhook", func() {
139141
ginkgo.By("Updating the resourceFlavor with invalid labels")
140142
err := k8sClient.Update(ctx, &created)
141143
gomega.Expect(err).To(gomega.HaveOccurred())
142-
gomega.Expect(errors.IsForbidden(err)).To(gomega.BeTrue(), "error: %v", err)
144+
gomega.Expect(err).Should(testing.BeAPIError(testing.InvalidError))
143145
})
144146
})
147+
148+
ginkgo.DescribeTable("Validate resourceFlavor on creation", func(rf *kueue.ResourceFlavor, errorType int) {
149+
err := k8sClient.Create(ctx, rf)
150+
if err == nil {
151+
defer func() {
152+
util.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, rf, true)
153+
}()
154+
}
155+
switch errorType {
156+
case isForbidden:
157+
gomega.Expect(err).Should(gomega.HaveOccurred())
158+
gomega.Expect(err).Should(testing.BeAPIError(testing.ForbiddenError), "error: %v", err)
159+
case isInvalid:
160+
gomega.Expect(err).Should(gomega.HaveOccurred())
161+
gomega.Expect(err).Should(testing.BeAPIError(testing.InvalidError), "error: %v", err)
162+
default:
163+
gomega.Expect(err).ShouldNot(gomega.HaveOccurred())
164+
}
165+
},
166+
ginkgo.Entry("Should fail to create with invalid taints",
167+
testing.MakeResourceFlavor("resource-flavor").
168+
Taint(corev1.Taint{
169+
Key: "skdajf",
170+
}).
171+
Taint(corev1.Taint{
172+
Key: "@foo",
173+
Value: "bar",
174+
Effect: corev1.TaintEffectNoSchedule,
175+
}).Obj(),
176+
isInvalid),
177+
ginkgo.Entry("Should fail to create with invalid label name",
178+
testing.MakeResourceFlavor("resource-flavor").Label("@abc", "foo").Obj(),
179+
isForbidden),
180+
ginkgo.Entry("Should fail to create with invalid tolerations",
181+
testing.MakeResourceFlavor("resource-flavor").
182+
Toleration(corev1.Toleration{
183+
Key: "@abc",
184+
Operator: corev1.TolerationOpEqual,
185+
Value: "v",
186+
Effect: corev1.TaintEffectNoSchedule,
187+
}).
188+
Toleration(corev1.Toleration{
189+
Key: "abc",
190+
Operator: corev1.TolerationOpExists,
191+
Value: "v",
192+
Effect: corev1.TaintEffectNoSchedule,
193+
}).
194+
Toleration(corev1.Toleration{
195+
Key: "abc",
196+
Operator: corev1.TolerationOpEqual,
197+
Value: "v",
198+
Effect: corev1.TaintEffect("not-valid"),
199+
}).
200+
Toleration(corev1.Toleration{
201+
Key: "abc",
202+
Operator: corev1.TolerationOpEqual,
203+
Value: "v",
204+
Effect: corev1.TaintEffectNoSchedule,
205+
}).
206+
Obj(),
207+
isInvalid),
208+
)
145209
})

0 commit comments

Comments
 (0)