Use Sign() to reject negative quantities in validateBasicResource

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

The problem

The function validateBasicResource was intended to reject negative resource quantities, but relied on quantity.Value() < 0 for this check. However, Value() does not reliably report the sign for all inputs: it can overflow and return a positive number for some negative values, or fall back to zero for others. As a result, certain negative quantities (like -9.5Gi or -1e30) were incorrectly accepted, and some large positive values were incorrectly rejected.

Why it's non-trivial

A naive fix would just check if Value() < 0, but this misses edge cases where Value() overflows or returns zero for negative values. Similarly, some large positive values can wrap around and appear negative, leading to false rejections. The correct solution must robustly determine the sign of the quantity regardless of overflow or internal representation.

The approach

The PR replaces the unreliable Value() check with a sign-aware method, and improves error reporting:

-	if quantity.Value() < 0 {
-		return field.ErrorList{field.Invalid(fldPath, quantity.Value(), "must be a valid resource quantity")}
+	if quantity.Sign() < 0 {
+		return field.ErrorList{field.Invalid(fldPath, quantity.String(), "must be a valid resource quantity")}
 	}

• What this does:
  • Uses quantity.Sign() < 0 to reliably detect negative quantities, regardless of how Value() handles overflow or conversion.
  • Uses quantity.String() in the error message so the reported value matches the user's input, not a potentially overflowed integer.

The PR also adds a focused table-driven test to cover all relevant edge cases:

func TestValidateBasicResource(t *testing.T) {
	// ...snip...
	cases := []struct {
		name     string
		quantity string
		wantErr  bool
	}{
		{name: "zero", quantity: "0"},
		{name: "positive", quantity: "1Gi"},
		{name: "positive, above MaxInt64", quantity: "1000E"},
		{name: "positive, far above MaxInt64", quantity: "1e30"},
		{name: "positive, Value() wraps negative", quantity: "9223372036854775808"},
		{name: "negative", quantity: "-1Gi", wantErr: true},
		{name: "negative, Value() overflows positive", quantity: "-9.5Gi", wantErr: true},
		{name: "negative, Value() overflows positive with more digits", quantity: "-9.5000000001Gi", wantErr: true},
		{name: "negative, Value() falls back to zero", quantity: "-1e30", wantErr: true},
		{name: "negative, suffixed, Value() falls back to zero", quantity: "-100E", wantErr: true},
		{name: "negative, at the int64 boundary", quantity: "-9223372036854775808", wantErr: true},
	}
	// ...snip...
}

• What this does:
  • Explicitly tests negative quantities that previously slipped through, as well as large positive values that could be falsely rejected.

From the review thread

You're right on all three — verified by running the validators against the overflow-negative that slips past Value():

master, capacity = -1E30 (Value()=0, Sign()=-1):
  PV  spec.capacity               REJECT  ValidatePositiveQuantityValue (Cmp<=0)
  PVC status.allocatedResources   REJECT  ValidateNonnegativeQuantity  (Cmp<0)
  PVC status.capacity             ACCEPT  validateBasicResource only

So status.capacity is the only path this flips for negatives; I've rewritten the description and release note to name it and drop the PV framing.

Your first extra is more than cosmetic, so I've folded it in: validateBasicResource was also falsely rejecting a positive whose Value() wraps negative — 9223372036854775808 (2^63) has Value() = -2^63, Sign() = +1, and on master all three paths reject it via Value() < 0; Sign() makes all three accept. So the change closes the missing negative check on status.capacity and removes a spurious rejection of large valid capacities on every path. Added a positive, Value() wraps negative case to TestValidateBasicResource.

Takeaways

• Always use APIs that explicitly convey the property you care about (e.g., sign) rather than inferring it from potentially lossy or overflow-prone conversions.
• Edge cases around numeric overflows and conversions should be covered by targeted tests, especially when dealing with user-supplied quantities.
• Error messages should reflect the user's actual input, not internal representations that may be misleading due to overflow or conversion logic.