[BUG] Webhook taint conflict check incorrectly rejects dry-run rules

[tejassinghbhati]

What happened?

When the validation webhook is enabled, it blocks creation of a dryRun: true rule whenever an existing enforcement rule already manages the same taint key on overlapping nodes — even though a dry-run rule never writes any taint to any node and therefore presents no actual conflict.

The inverse is also blocked: promoting an existing dry-run rule to enforcement by creating a new enforcement rule with the same taint key is rejected if a dry-run rule for that key already exists.

Root cause

validateTaintConflicts in nodereadinessgaterule_webhook.go iterates all existing rules and flags any pair sharing the same taint.key + taint.effect on overlapping node selectors. It does not check spec.dryRun on either side:

// existing code — no dryRun guard
if existingRule.Spec.Taint.Key == rule.Spec.Taint.Key &&
    existingRule.Spec.Taint.Effect == rule.Spec.Taint.Effect {
    if w.nodeSelectorsOverlap(...) {
        allErrs = append(allErrs, field.Invalid(...))
    }
}

Steps to reproduce

1. Create an enforcement rule:

apiVersion: readiness.node.x-k8s.io/v1alpha1
kind: NodeReadinessRule
metadata:
  name: enforcement-rule
spec:
  enforcementMode: continuous
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/worker: ""
  conditions:
    - type: Ready
      requiredStatus: "True"
  taint:
    key: readiness.k8s.io/node-ready
    effect: NoSchedule

2. Attempt to create a dry-run preview rule for the same taint key (e.g. to evaluate a condition change):

apiVersion: readiness.node.x-k8s.io/v1alpha1
kind: NodeReadinessRule
metadata:
  name: preview-rule
spec:
  dryRun: true
  enforcementMode: continuous
  nodeSelector:
    matchLabels:
      node-role.kubernetes.io/worker: ""
  conditions:
    - type: DiskPressure
      requiredStatus: "False"
  taint:
    key: readiness.k8s.io/node-ready
    effect: NoSchedule

3. The webhook rejects the request:

Error: admission webhook "vnodereadinessrule.readiness.node.x-k8s.io" denied the request:
  validation failed: [spec.taint.key: Invalid value: "readiness.k8s.io/node-ready":
  conflicts with existing rule 'enforcement-rule' ...]

Expected behavior

A dryRun: true rule must never be rejected for a taint key conflict because it does not apply, remove, or modify any taint. The conflict guard exists to prevent two enforcement rules from fighting over the same taint; that concern does not apply to dry-run rules.

Impact

This makes the primary dryRun use case — previewing the impact of a rule against a taint key already in use — impossible when the webhook is enabled.

Controller version / image tag

Reproduced on main at e259c3f.

Kubernetes version

Not version-specific; this is webhook validation logic.

[ajaysundark]

it blocks creation of a dryRun: true rule whenever an existing enforcement rule already manages the same taint key

Could you clarify why would we want this and why it is safer to allow this?
The webhook blocking conflicting taint-keys are intentional as that likely a misconfiguration.

[tejassinghbhati]

Hey @ajaysundark,

The main use case is testing changes on existing rules. If you already have a rule enforcing a taint in prod and want to test adding a new condition to it, you'd want to spin up a dryRun: true rule for the same taint to check .status.dryRunResults before making any actual changes. Without this, you'd have to delete your active prod rule just to test a new one.

As for safety, dry-run rules are completely read-only for nodes, the controller just writes stats to the rule's own status and never touches node taints, so there's no risk of taint fighting or flapping.

And if someone tries to flip dryRun: false on it later while the original rule is still around, the webhook will still catch and block it then.

[ajaysundark]

If you already have a rule enforcing a taint in prod and want to test adding a new condition to it, you'd want to spin up a dryRun: true rule for the same taint

This CUJ is not supported today as conditions are the immutable. The only way to go about this is to use a different rule (and taint-key), starting in dry run and enforce it, once rule-B is adopted, old rule can be removed.

[tejassinghbhati]

A couple of reasons why using a separate rule with the same taint key is needed here:

1. Dry-run simulation accuracy: The dry-run engine checks if nodes currently have rule.Spec.Taint to calculate taintsToRemove vs taintsToAdd. If forced to use a dummy taint key, currentlyHasTaint is always false—meaning taintsToRemove will always report 0 and you can't accurately simulate the effect on existing tainted nodes.
2. Testing around immutability: Since conditions are immutable on rule-A, creating a new rule-B (with dryRun: true and the target taint key) is the only way to preview the new condition logic against live nodes before doing the migration (delete rule-A -> enforce rule-B).
3. Fixed taint contracts: In production, taint keys are often fixed contracts configured across workload tolerations / scheduling configs, so operators usually can't just change the taint key when tuning readiness criteria.

Since rule-B is in dry run and doesn't mutate nodes, allowing it alongside rule-A gives operators accurate preview stats without risking any taint flapping.

[Ayush4958]

I think the PR #436 to fix the webhook is correct but the migration path suggested by @tejassinghbhati is brittle & may cause heavy taint flapping

Migrating path is brittle as Deleting rule-A will drops the taint & create a scheduling race condition before the rule-B applies, leading to heavy taint flapping.

[tejassinghbhati]

I wouldn't call that flapping @Ayush4958. Flapping is when two controllers fight over a taint in an endless loop, swapping rules is just a one-time handoff.

In prod, you'd usually canary the new rule across node pools using nodeSelector anyway, so you never have to do a blind cluster-wide swap.

Either way, dry-run is just for previewing stats beforehand. Blocking it doesn't prevent any race condition, it just forces operators to fly blind when testing new rules.

[Ayush4958]

I wouldn't call that flapping @Ayush4958. Flapping is when two controllers fight over a taint in an endless loop, swapping rules is just a one-time handoff.

yup it is a one time handoff not an endless one

In prod, you'd usually canary the new rule across node pools using nodeSelector anyway, so you never have to do a blind cluster-wide swap.

even if you canary it using nodeSelector , the second a node stops matching rule-A & transitions to rule-B , there is a temp gap where the node is completely untainted. During that gap, the scheduler will allow pods to sneak onto nodes that your rules dictate are unready

Since this controller heavily uses NoSchedule taints, those pods won't be evicted when rule-B finally kicks in... they will just be stuck running on nodes they were supposed to be blocked from.

Either way, dry-run is just for previewing stats beforehand. Blocking it doesn't prevent any race condition, it just forces operators to fly blind when testing new rules.

yup, I completely agree

[tejassinghbhati]

if you completely agree on whatever I am saying idts there is any need of ai clarifications and stuff

[Ayush4958]

if you completely agree on whatever I am saying idts there is any need of ai clarifications and stuff

final review of @ajaysundark still needed to merge ur raised PR

[tejassinghbhati]

@Ayush4958 sorry, that came out sharper than I meant it and it was unfair. Your point about the gap is a good one and it actually helps the case here.

You are right that during an A to B handoff the node sits untainted for a window, and with NoSchedule anything the scheduler places in that window stays put rather than getting evicted later. That is a real cost of the swap approach, and it is a reason to want an accurate preview before doing it rather than an argument against previewing.

@ajaysundark on your point that the CUJ is not supported because conditions are immutable, I think that is exactly why this matters, and there is a concrete detail worth weighing.

processDryRun computes taintsToRemove from rule.Spec.Taint:

currentlyHasTaint := r.hasTaintBySpec(&node, rule.Spec.Taint)
if shouldRemoveTaint && currentlyHasTaint {
    taintsToRemove++
} else if !shouldRemoveTaint && !currentlyHasTaint {
    taintsToAdd++
}

So a preview rule with a different taint key never sees the taint that is actually on the nodes. currentlyHasTaint is false everywhere, taintsToRemove is always 0, and taintsToAdd counts every matching node. The preview reports a full rollout rather than the delta, which is the number you would want before a migration.

Happy to close this and #436 if you would rather keep the webhook strict, no argument from me. But if dry run is meant to answer what would change, it cannot do that today for any taint key already in use.