feat: Restrict NodeReadinessRuleSpec.Taint to "readiness.k8s.io/" prefix by sats-23 · Pull Request #112 · kubernetes-sigs/node-readiness-controller

sats-23 · 2026-01-26T17:07:32Z

Fixes #106

k8s-ci-robot · 2026-01-26T17:07:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sats-23
Once this PR has been reviewed and has the lgtm label, please assign mrunalp for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2026-01-26T17:07:41Z

✅ Deploy Preview for node-readiness-controller ready!

Name	Link
🔨 Latest commit	`ec812d9`
🔍 Latest deploy log	https://app.netlify.com/projects/node-readiness-controller/deploys/698da4ba85cf8900086d7ce8
😎 Deploy Preview	https://deploy-preview-112--node-readiness-controller.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

k8s-ci-robot · 2026-01-26T17:07:42Z

Welcome @sats-23!

It looks like this is your first PR to kubernetes-sigs/node-readiness-controller 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/node-readiness-controller has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

ajaysundark · 2026-01-28T06:54:57Z

@Karthik-K-N Do you have time for this review?

Karthik-K-N · 2026-01-28T08:03:42Z

@Karthik-K-N Do you have time for this review?

Yes, I will check thanks.

api/v1alpha1/nodereadinessrule_types.go

Karthik-K-N · 2026-01-30T05:32:34Z

api/v1alpha1/nodereadinessrule_types.go

+	// +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'"


This looks good. Thanks

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.

Sure, will do

internal/controller/node_controller_test.go

Karthik-K-N · 2026-01-30T05:34:41Z

internal/webhook/nodereadinessgaterule_webhook.go

 	taintField := specField.Child("taint")
 	if spec.Taint.Key == "" {
 		allErrs = append(allErrs, field.Required(taintField.Child("key"), "taint key cannot be empty"))
+	} else {


Apologies, I think with CEL, Nowhere it could skip the validation there so webhook might not be needed. We can remove.

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

IIUC, @Karthik-K-N's suggestion was that field validations in webhook is redundant as you already added the CEL check at API. The conflict check for rules are already in place and they need not be updated for the scope of this PR.

ajaysundark · 2026-02-10T06:04:01Z

/retest

ajaysundark · 2026-02-10T19:33:58Z

/assign @sats-23

looks like the tests need your attention, ptal

Signed-off-by: Sathvik <Sathvik.S@ibm.com>

ajaysundark · 2026-02-13T07:25:35Z

/retest

ajaysundark · 2026-02-16T07:38:31Z

internal/webhook/nodereadinessgaterule_webhook.go

+			allErrs = append(allErrs, field.Invalid(taintField.Child("key"), spec.Taint.Key, "taint key must start with 'readiness.k8s.io/'"))
+		}
+		// Validate key length
+		if len(spec.Taint.Key) < 17 || len(spec.Taint.Key) > 253 {


nit: <17 length check feels redundant as you already have the prefix check above.

ajaysundark · 2026-02-16T07:40:43Z

internal/webhook/nodereadinessgaterule_webhook.go

 	taintField := specField.Child("taint")
 	if spec.Taint.Key == "" {
 		allErrs = append(allErrs, field.Required(taintField.Child("key"), "taint key cannot be empty"))
+	} else {


IIUC, @Karthik-K-N's suggestion was that field validations in webhook is redundant as you already added the CEL check at API. The conflict check for rules are already in place and they need not be updated for the scope of this PR.

k8s-ci-robot requested review from SergeyKanzhelev and tallclair January 26, 2026 17:07

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 26, 2026

ajaysundark requested review from Karthik-K-N and removed request for SergeyKanzhelev and tallclair January 26, 2026 17:11

sats-23 force-pushed the issue106 branch 2 times, most recently from 211e30a to 770db23 Compare January 26, 2026 17:44

sats-23 force-pushed the issue106 branch from 770db23 to 70026e1 Compare January 28, 2026 08:58

Karthik-K-N reviewed Jan 28, 2026

View reviewed changes

api/v1alpha1/nodereadinessrule_types.go Show resolved Hide resolved

sats-23 mentioned this pull request Jan 29, 2026

REQUEST: New membership for sats-23 kubernetes/org#6111

Closed

11 tasks

sats-23 force-pushed the issue106 branch from 472a78b to ff9998b Compare January 30, 2026 05:08

sats-23 requested a review from Karthik-K-N January 30, 2026 05:10

Karthik-K-N reviewed Jan 30, 2026

View reviewed changes

sats-23 force-pushed the issue106 branch from ff9998b to 692ff1c Compare February 2, 2026 13:22

k8s-ci-robot assigned sats-23 Feb 10, 2026

feat: Restrict NodeReadinessRuleSpec.Taint to "readiness.k8s.io/" prefix

ec812d9

Signed-off-by: Sathvik <Sathvik.S@ibm.com>

sats-23 force-pushed the issue106 branch from 692ff1c to ec812d9 Compare February 12, 2026 10:00

ajaysundark self-requested a review February 16, 2026 07:28

ajaysundark requested changes Feb 16, 2026

View reviewed changes

Conversation

sats-23 commented Jan 26, 2026

Uh oh!

k8s-ci-robot commented Jan 26, 2026

Uh oh!

netlify bot commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for node-readiness-controller ready!

Uh oh!

k8s-ci-robot commented Jan 26, 2026

Uh oh!

ajaysundark commented Jan 28, 2026

Uh oh!

Karthik-K-N commented Jan 28, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajaysundark commented Feb 10, 2026

Uh oh!

ajaysundark commented Feb 10, 2026

Uh oh!

ajaysundark commented Feb 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify bot commented Jan 26, 2026 •

edited

Loading