feat: Restrict NodeReadinessRuleSpec.Taint to "readiness.k8s.io/" prefix #112
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sats-23 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for node-readiness-controller canceled.
|
|
Welcome @sats-23! |
k8s-ci-robot
added
size/L
ajaysundark
requested review from
Karthik-K-N
and removed request for
SergeyKanzhelev and
tallclair
sats-23
force-pushed
the
issue106
branch
2 times, most recently
from
211e30a to
770db23
Compare
|
@Karthik-K-N Do you have time for this review? |
Yes, I will check thanks. |
| // +kubebuilder:validation:XValidation:rule="self.key.startsWith('readiness.k8s.io/')",message="taint key must start with 'readiness.k8s.io/'" | ||
| // +kubebuilder:validation:XValidation:rule="self.key.size() >= 17 && self.key.size() <= 253",message="taint key length must be between 17 and 253 characters" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.value) || self.value.size() <= 63",message="taint value length must be at most 63 characters" | ||
| // +kubebuilder:validation:XValidation:rule="self.effect in ['NoSchedule', 'PreferNoSchedule', 'NoExecute']",message="taint effect must be one of 'NoSchedule', 'PreferNoSchedule', 'NoExecute'" |
There was a problem hiding this comment.
This looks good. Thanks
There was a problem hiding this comment.
Also lets document it somewhere in the book or doc that the readiness.k8s.io/ should be the prefix for taints, User should be aware of it.
| Key: "readiness.k8s.io/status-test-taint", | ||
| Effect: corev1.TaintEffectNoSchedule, | ||
| Value: "pending", | ||
| Value: val, |
There was a problem hiding this comment.
Just extracted it for consistency
| taintField := specField.Child("taint") | ||
| if spec.Taint.Key == "" { | ||
| allErrs = append(allErrs, field.Required(taintField.Child("key"), "taint key cannot be empty")) | ||
| } else { |
There was a problem hiding this comment.
Apologies, I think with CEL, Nowhere it could skip the validation there so webhook might not be needed. We can remove.
There was a problem hiding this comment.
I was thinking what in case of conflict detection, like, if a rule A already exists and rule B is applied on the same set of nodes (same taint), then individually with CEL, rule B may pass but cause conflict in the controller?
In case, this is invalid, I can remove it
Signed-off-by: Sathvik <Sathvik.S@ibm.com>
4 participants
Fixes #106