fix: make nil DeviceTaintSelector match no devices (tracker)

Project: kubernetes/kubernetes | Language: Go | Concepts: error-handling, api-design, testing-strategy, refactoring | Difficulty: mid
Link: https://github.com/kubernetes/kubernetes/pull/141648

The problem

If a DeviceTaintRule was created with spec.deviceSelector omitted (i.e., set to nil), the system incorrectly treated this as matching every device in the cluster. According to the API documentation, omitting the selector should match no devices. This bug caused a single rule with an omitted selector to block all DRA scheduling cluster-wide, which was especially problematic for admin claims.

Why it's non-trivial

A naive fix might just check for nil in one place, but the bug was spread across multiple code paths and helper functions. The code previously defaulted a nil selector to an empty struct, which matched all devices, and there were subtle differences between how NoSchedule and NoExecute taints were handled. Fixing this required careful changes to both the patch key logic and the main patch application logic, as well as updates to tests and benchmarks to match the documented behavior.

The approach

1. Fixing the patch key logic to handle nil selectors:

Previously, the function defaulted a nil selector to an empty struct, which matched all devices:

-func driverPoolDeviceIndexPatchKey(patch *resourceapi.DeviceTaintRule) string {
-	deviceSelector := ptr.Deref(patch.Spec.DeviceSelector, resourceapi.DeviceTaintSelector{})
+func driverPoolDeviceIndexPatchKey(patch *resourceapi.DeviceTaintRule) string {
+	deviceSelector := patch.Spec.DeviceSelector
+	if deviceSelector == nil {
+		// No selector matches no devices, so it matches no slices.
+		return ""
+	}

Annotation:
The new code checks if deviceSelector is nil and, if so, returns an empty string. This ensures that a rule with no selector will not match any device slice, which is the correct behavior per the API documentation.

2. Skipping rules with nil selectors in the patch application logic:

The patch application previously only checked for matching driver/pool if the selector was present, but did not skip rules with a nil selector:

-		deviceSelector := taintRule.Spec.DeviceSelector
-		var deviceName *string
-		if deviceSelector != nil {
-			if deviceSelector.Driver != nil && *deviceSelector.Driver != slice.Spec.Driver {
-				logger.V(7).Info("DeviceTaintRule does not apply, mismatched driver", "sliceDriver", slice.Spec.Driver, "taintDriver", *deviceSelector.Driver)
-				continue
-			}
-			if deviceSelector.Pool != nil && *deviceSelector.Pool != slice.Spec.Pool.Name {
-				logger.V(7).Info("DeviceTaintRule does not apply, mismatched pool", "slicePool", slice.Spec.Pool.Name, "taintPool", *deviceSelector.Pool)
-				continue
-			}
-			deviceName = deviceSelector.Device
-		}
+		deviceSelector := taintRule.Spec.DeviceSelector
+		if deviceSelector == nil {
+			// No selector matches no devices.
+			logger.V(7).Info("DeviceTaintRule does not apply, no selector")
+			continue
+		}
+		if deviceSelector.Driver != nil && *deviceSelector.Driver != slice.Spec.Driver {
+			logger.V(7).Info("DeviceTaintRule does not apply, mismatched driver", "sliceDriver", slice.Spec.Driver, "taintDriver", *deviceSelector.Driver)
+			continue
+		}
+		if deviceSelector.Pool != nil && *deviceSelector.Pool != slice.Spec.Pool.Name {
+			logger.V(7).Info("DeviceTaintRule does not apply, mismatched pool", "slicePool", slice.Spec.Pool.Name, "taintPool", *deviceSelector.Pool)
+			continue
+		}
+		deviceName := deviceSelector.Device

Annotation:
Now, if the selector is nil, the rule is skipped entirely, and a debug log is emitted. This enforces the correct "nil matches none" semantics.

3. Updating tests and benchmarks to reflect the correct semantics:

Test fixtures and benchmarks previously relied on the buggy "nil matches all" behavior. They are updated to use an explicit empty selector (&DeviceTaintSelector{}) to match all devices, and new tests are added to ensure nil matches none:

-var (
-	taintAllDevicesRule = &resourceapi.DeviceTaintRule{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "rule",
-		},
-		Spec: resourceapi.DeviceTaintRuleSpec{
-			Taint: deviceTaint(deviceTaint1),
-		},
-	}
+var (
+	taintAllDevicesRule = &resourceapi.DeviceTaintRule{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "rule",
+		},
+		Spec: resourceapi.DeviceTaintRuleSpec{
+			DeviceSelector: &resourceapi.DeviceTaintSelector{},
+			Taint:          deviceTaint(deviceTaint1),
+		},
+	}
+	taintNoSelectorRule = &resourceapi.DeviceTaintRule{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "rule-no-selector",
+		},
+		Spec: resourceapi.DeviceTaintRuleSpec{
+			Taint: deviceTaint(deviceTaint1),
+		},
+	}

Annotation:
taintAllDevicesRule now explicitly sets an empty selector to match all devices. taintNoSelectorRule omits the selector to test the "matches none" case.

A new regression test ensures nil selectors do not match any devices:

+		"nil-selector-matches-none": {
+			events: []any{
+				add(slice1),
+				add(slice2),
+				add(taintNoSelectorRule),
+			},
+			expectedPatchedSlices: []*resourceapi.ResourceSlice{
+				slice1,
+				slice2,
+			},
+			expectedHandlerEvents: []handlerEvent{
+				{event: handlerEventAdd, newObj: slice1},
+				{event: handlerEventAdd, newObj: slice2},
+			},
+		},

Annotation:
This test confirms that adding a rule with a nil selector does not taint any devices.

Benchmarks are also updated to use the explicit empty selector:

-						DeviceSelector: nil, // all slices
+						DeviceSelector: &resourceapi.DeviceTaintSelector{}, // all slices

From the review thread

Reviewer nit:

Nit: let's use deviceSelector := patch.Spec.DeviceSelector (i.e. keep it as a pointer) and do the nil check after that assignment.

Author response:

Done — keeping it as a pointer now and checking after the assignment. Thanks for the nit!

Reviewer nit:

Nit: let's move this after deviceSelector := taintRule.Spec.DeviceSelector and then check deviceSelector for nil.

Author response:

Done — moved the assignment before the nil check as suggested. Thanks!

Takeaways

• Always align code behavior with documented API semantics, especially for edge cases like nil or omitted fields.
• When fixing subtle bugs, update both the implementation and all related tests/benchmarks to reflect the intended behavior.
• Use explicit values (e.g., empty structs) to represent "match all" or "match none" cases, rather than relying on implicit or defaulted behaviors.