NumericBounds Linter #176

krishagarwal278 · 2025-10-31T22:12:09Z

Numericbounds Linter

Adds a new analyzer that ensures int32, int64, float32, float64 fields have both minimum and maximum bounds validation markers, following Kubernetes API conventions.

What it checks

Both Minimum and Maximum markers are present on numeric fields
Uses items:Minimum and items:Maximum for slice elements
Unwraps pointers and slices

Fixes #18

krishagarwal278 · 2025-11-03T00:56:52Z

/label tide/merge-method-squash

JoelSpeed · 2025-11-03T11:10:03Z

docs/linters.md


+## NumericBounds
+
+The `numericbounds` linter checks that numeric fields (`int32` and `int64`) have appropriate bounds validation markers.


What about floats? We discourage them, but people can/do still use them

JoelSpeed · 2025-11-03T11:10:35Z

docs/linters.md

+According to Kubernetes API conventions, numeric fields should have bounds checking to prevent values that are too small, negative (when not intended), or too large.
+
+This linter ensures that:
+- `int32` and `int64` fields have both `+kubebuilder:validation:Minimum` and `+kubebuilder:validation:Maximum` markers


We should check for declarative validation markers too if they are in beta at this point

i didn't add it earlier as i only found +k8s:maximum in the k8s documentation. On exploring further i saw that the markers package does mention it. So i incorporated it 👍🏻

JoelSpeed · 2025-11-03T11:10:55Z

docs/linters.md

+
+This linter ensures that:
+- `int32` and `int64` fields have both `+kubebuilder:validation:Minimum` and `+kubebuilder:validation:Maximum` markers
+- `int64` fields with bounds outside the JavaScript safe integer range are flagged


We should also check if an int32 bound is beyond the limits of an int32

JoelSpeed · 2025-11-03T11:11:41Z

docs/linters.md

+### Examples
+
+**Valid:** Numeric field with proper bounds markers
+```go
+type Example struct {
+    // +kubebuilder:validation:Minimum=0
+    // +kubebuilder:validation:Maximum=100
+    Count int32
+}
+```
+
+**Valid:** Int64 field with JavaScript-safe bounds
+```go
+type Example struct {
+    // +kubebuilder:validation:Minimum=-9007199254740991
+    // +kubebuilder:validation:Maximum=9007199254740991
+    Timestamp int64
+}
+```
+
+**Invalid:** Missing bounds markers
+```go
+type Example struct {
+    Count int32 // want: should have minimum and maximum bounds validation markers
+}
+```
+
+**Invalid:** Only one bound specified
+```go
+type Example struct {
+    // +kubebuilder:validation:Minimum=0
+    Count int32 // want: has minimum but is missing maximum bounds validation marker
+}
+```
+
+**Invalid:** Int64 with bounds exceeding JavaScript safe range
+```go
+type Example struct {
+    // +kubebuilder:validation:Minimum=-10000000000000000
+    // +kubebuilder:validation:Maximum=10000000000000000
+    LargeNumber int64 // want: bounds exceed JavaScript safe integer range
+}
+```
+


This is probably too much detail for the docs here, we can have this in the package level docs I think

agreed 👍🏻

JoelSpeed · 2025-11-03T11:12:22Z

pkg/analysis/numericbounds/doc.go

+		// +kubebuilder:validation:Maximum=100
+		Count int32
+	}
+


Maybe copy the more complex examples you added in the docs to here

moved it to docs in numericbounds package 👍🏻

JoelSpeed · 2025-11-03T11:16:40Z

pkg/analysis/numericbounds/analyzer.go

+	maxSafeInt = 9007199254740991  // 2^53 - 1
+	minSafeInt = -9007199254740991 // -(2^53 - 1)


Rename these to Int64 and also add versions for Int32 to check?

pkg/analysis/numericbounds/analyzer.go

JoelSpeed · 2025-11-03T11:18:52Z

pkg/analysis/numericbounds/analyzer.go

+	// Check if it's a slice and unwrap (e.g., []int32)
+	if arrayType, ok := expr.(*ast.ArrayType); ok {
+		isSlice = true
+		expr = arrayType.Elt
+
+		// Handle pointer inside slice (e.g., []*int32)
+		if starExpr, ok := expr.(*ast.StarExpr); ok {
+			expr = starExpr.X
+		}
+	}
+
+	return expr, isSlice


When we unwrap array element types, we should explain that it is the array element type we are reporting the issue on

JoelSpeed · 2025-11-03T11:19:43Z

pkg/analysis/numericbounds/initializer.go

 		name,
-		newAnalyzer(),
+		Analyzer,
 		true,


At the moment, this is only for CRDs (since it's only looking at kubebuilder markers). So cannot be on by default

JoelSpeed · 2025-11-03T11:20:54Z

pkg/analysis/numericbounds/analyzer.go

+// getMarkerNumericValue extracts the numeric value from the first instance of the marker with the given name.
+func getMarkerNumericValue(markerSet markershelper.MarkerSet, markerName string) (float64, error) {
+	markerList := markerSet.Get(markerName)
+	if len(markerList) == 0 {
+		return 0, errMarkerMissingValue
+	}
+
+	marker := markerList[0]
+	rawValue, ok := marker.Expressions[""]
+	if !ok {
+		return 0, errMarkerMissingValue
+	}
+
+	// Parse as float64 using strconv for better error handling
+	value, err := strconv.ParseFloat(rawValue, 64)
+	if err != nil {
+		return 0, fmt.Errorf("error converting value to number: %w", err)
+	}
+
+	return value, nil
+}


Check the utils library, I'm sure we have some existing helpers for this

everettraven · 2025-11-05T20:03:42Z

pkg/analysis/numericbounds/analyzer.go

+	// Unwrap pointers and slices to get the underlying type
+	fieldType, isSlice := unwrapType(field.Type)
+
+	// Get the underlying numeric type identifier (int32, int64, float32, float64)
+	ident := getNumericTypeIdent(pass, fieldType)
+	if ident == nil {
+		return
+	}


I wonder if we can reuse

kube-api-linter/pkg/analysis/utils/type_check.go

Lines 33 to 38 in 5fca1e8

// NewTypeChecker returns a new TypeChecker with the provided checkFunc.

func NewTypeChecker(checkFunc func(pass *analysis.Pass, ident *ast.Ident, node ast.Node, prefix string)) TypeChecker {

return &typeChecker{

checkFunc: checkFunc,

}

}

here to drill down to the built-in type of the field we are checking without having to write this additional logic to do the unwrapping?

If we need to determine if the field is also a slice, we could also use:

kube-api-linter/pkg/analysis/utils/utils.go

Lines 216 to 232 in 5fca1e8

// IsArrayTypeOrAlias checks if the field type is an array type or an alias to an array type.

func IsArrayTypeOrAlias(pass *analysis.Pass, field *ast.Field) bool {

if _, ok := field.Type.(*ast.ArrayType); ok {

return true

}

if ident, ok := field.Type.(*ast.Ident); ok {

typeOf := pass.TypesInfo.TypeOf(ident)

if typeOf == nil {

return false

}

return isArrayType(typeOf)

}

return false

}

Using TypeChecker sounds like a good idea

pkg/analysis/numericbounds/analyzer.go

everettraven · 2025-11-05T20:16:59Z

pkg/analysis/numericbounds/analyzer.go

+	// Extract the actual type name from typeDesc (e.g., "array element type of int32" -> "int32")
+	typeName := extractTypeName(typeDesc)
+
+	switch typeName {


missing int64 and float64?

everettraven · 2025-11-05T20:19:16Z

pkg/analysis/numericbounds/analyzer.go

+		if minimum < minInt32 || minimum > maxInt32 {
+			pass.Reportf(field.Pos(), "field %s %s has minimum bound %v that is outside the valid int32 range [%d, %d]", fieldName, typeDesc, minimum, minInt32, maxInt32)
+		}
+		if maximum < minInt32 || maximum > maxInt32 {
+			pass.Reportf(field.Pos(), "field %s %s has maximum bound %v that is outside the valid int32 range [%d, %d]", fieldName, typeDesc, maximum, minInt32, maxInt32)
+		}


This looks like you might be comparing float64 values to int64 values here? Is that what we want?

adding cases for int64 and float64 would prevent comparing values of float64 values to int64 right?

everettraven · 2025-11-05T20:21:15Z

pkg/analysis/numericbounds/analyzer.go

+}
+
+// checkJavaScriptSafeBounds checks if int64 bounds are within JavaScript safe integer range.
+func checkJavaScriptSafeBounds(pass *analysis.Pass, field *ast.Field, fieldName, typeDesc string, minimum, maximum float64) {


Do we actually care about "JavaScript safe bounds"? In my eyes, this is just the bounds Kubernetes API conventions are enforcing - not sure why we need 2 separate functions for doing the bound checks?

The javascript safe bounds is a limitation of JSON. If you go outside of the 2^53 then you have to change the value to a string. So we do care about limiting the bounds here, code layout is a different question

JoelSpeed · 2025-11-06T13:31:10Z

docs/linters.md

+### Configuration
+
+This linter is **not enabled by default** as it is primarily focused on CRD validation with kubebuilder markers. It can be enabled in your configuration.
+
+To enable in `.golangci.yml`:
+```yaml
+linters-settings:
+  custom:
+    kubeapilinter:
+      settings:
+        linters:
+          enable:
+            - numericbounds
+```
+
+See the [package documentation](https://pkg.go.dev/sigs.k8s.io/kube-api-linter/pkg/analysis/numericbounds) for detailed examples and usage.


We don't have this for other linters so can probably skip unless there is any specific configuration for this rule

JoelSpeed · 2025-11-06T13:33:18Z

pkg/analysis/numericbounds/analyzer.go

+}
+
+// checkJavaScriptSafeBounds checks if int64 bounds are within JavaScript safe integer range.
+func checkJavaScriptSafeBounds(pass *analysis.Pass, field *ast.Field, fieldName, typeDesc string, minimum, maximum float64) {


The javascript safe bounds is a limitation of JSON. If you go outside of the 2^53 then you have to change the value to a string. So we do care about limiting the bounds here, code layout is a different question

JoelSpeed · 2025-11-06T13:34:22Z

pkg/analysis/numericbounds/analyzer.go

+	maxSafeInt32 = 2147483647        // 2^31 - 1 (fits in JS Number)
+	minSafeInt32 = -2147483648       // -2^31 (fits in JS Number)


These are the same as maxInt32/minInt32, not sure why we would duplicate?

JoelSpeed · 2025-11-06T13:34:37Z

pkg/analysis/numericbounds/analyzer.go

+	maxFloat32 = 3.40282346638528859811704183484516925440e+38  // max float32
+	minFloat32 = -3.40282346638528859811704183484516925440e+38 // min float32


No flato64 bounds?

Hadn't committed changes for float64 and int64. It's now incorporated in the latest commit

JoelSpeed · 2025-11-06T13:35:51Z

pkg/analysis/numericbounds/analyzer.go

+	// Unwrap pointers and slices to get the underlying type
+	fieldType, isSlice := unwrapType(field.Type)
+
+	// Get the underlying numeric type identifier (int32, int64, float32, float64)
+	ident := getNumericTypeIdent(pass, fieldType)
+	if ident == nil {
+		return
+	}


Using TypeChecker sounds like a good idea

JoelSpeed · 2025-11-06T13:37:38Z

pkg/analysis/numericbounds/analyzer.go

+	// Validate bounds are within the type's range
+	checkBoundsWithinTypeRange(pass, field, fieldName, typeDesc, minimum, maximum)
+
+	// For int64 fields, check if bounds are within JavaScript safe integer range
+	checkJavaScriptSafeBounds(pass, field, fieldName, typeDesc, minimum, maximum)


We can probably combine bounds within range type checks

sivchari · 2025-11-07T03:42:10Z

pkg/analysis/numericbounds/analyzer.go

+const (
+	maxInt32   = 2147483647                                      // 2^31 - 1
+	minInt32   = -2147483648                                     // -2^31
+	maxInt64   = 9223372036854775807                             // 2^63 - 1


maxInt64 and minInt64 are not used anywhere, so let's remove it.

good catch. Removed, since we already have maxSafeInt64 and minSafeInt64 👍🏻

sivchari · 2025-11-07T03:53:16Z

pkg/analysis/numericbounds/analyzer.go

+
+// containsArrayElement checks if the prefix string contains "array element"
+// which indicates we're checking a slice element type.
+func containsArrayElement(prefix string) bool {


You can use utilis.IsArrayType which is better than checking whether the prefix has specific strings.

used IsArrayTypeOrAlias() as it's already exported 👍🏻

JoelSpeed · 2025-11-07T18:20:41Z

docs/linters.md

+- Bounds values are within the type's valid range:
+  - int32: full int32 range (±2^31-1)
+  - int64: JavaScript-safe range (±2^53-1) per Kubernetes API conventions
+  - float32/float64: within their respective ranges


Is there any limitation in JSON for the size of a float64?

no, we don't have limitation in json for float64. however, i'm still validating bounds to enforce k8s convention to have numeric fields bounded

JoelSpeed · 2025-11-07T18:22:12Z

pkg/analysis/numericbounds/testdata/src/a/a.go

+
+// InvalidInt32NoBounds should have bounds markers
+type InvalidInt32NoBounds struct {
+	NoBounds int32 // want "field NoBounds is missing minimum bounds validation marker" "field NoBounds is missing maximum bounds validation marker"


Nit, probably minimum bound and maximum bound. i.e removing the s (I think this is appropriate english at least)

pkg/analysis/numericbounds/analyzer.go

JoelSpeed · 2025-11-07T18:39:14Z

pkg/analysis/numericbounds/analyzer.go

+// compatibility with JavaScript clients.
+//
+//nolint:cyclop
+func checkBoundsWithinTypeRange(pass *analysis.Pass, field *ast.Field, prefix, typeName string, minimum, maximum float64) {


If you were to use generics for these bounds checks, you might be able to avoid int to float to int conversions on the integer bounds checks

I suspect a bounds checking function that takes a boundary, a message, an upper and lower boundary could be leveraged here

…ssed in reviews

…, and used utils

k8s-ci-robot · 2025-11-12T16:58:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

JoelSpeed · 2025-11-12T16:46:12Z

docs/linters.md

+- Numeric fields have both `+kubebuilder:validation:Minimum` and `+kubebuilder:validation:Maximum` markers
+- K8s declarative validation markers (`+k8s:minimum` and `+k8s:maximum`) are also supported


I would swap the way we talk about these as we expect the k8s versions to take precedence in the long term

JoelSpeed · 2025-11-12T16:46:49Z

docs/linters.md

+
+**Note:** While `+k8s:minimum` is documented in the official Kubernetes declarative validation spec, `+k8s:maximum` is not yet officially documented but is supported by this linter for forward compatibility and consistency.
+
+This linter is **not enabled by default** as it is primarily focused on CRD validation with kubebuilder markers.


This is fine for now, I expect we will flip this in the future

JoelSpeed · 2025-11-12T16:50:32Z

pkg/analysis/numericbounds/analyzer.go

+		return nil, kalerrors.ErrCouldNotGetInspector
+	}
+
+	inspect.InspectFields(func(field *ast.Field, _ extractjsontags.FieldTagInfo, markersAccess markershelper.Markers, _ string) {


The final argument in this function is called qualifiedName. Please use this argument instead of working out your own field names here. This provides consistent naming across all linters

Or is there an issue making this work with the typechecker?

using qualifiedFieldName just like it's being used in arrayofstruct, commentstart, conflictingmarkers, and other linters 👍🏻

JoelSpeed · 2025-11-12T16:53:37Z

pkg/analysis/numericbounds/analyzer.go

+	// For array elements, update prefix to include type information
+	// e.g., "field Ports array element pointer" -> "field Ports array element type of int32"
+	if isSlice {
+		prefix = buildArrayElementPrefix(prefix, ident.Name)
+	}


I thought at least some of this was handled by the type checker 🤔

Removed buildArrayElementPrefix entirely 👍🏻
now using qualifiedFieldName directly from inspector and utils.IsArrayTypeOrAlias() to detect arrays

pkg/analysis/numericbounds/analyzer.go

JoelSpeed · 2025-11-12T16:55:54Z

pkg/analysis/numericbounds/analyzer.go

+// buildArrayElementPrefix updates the prefix for array elements to include type information.
+// Replaces TypeChecker's suffix (like "pointer" or "type X") with "type of {typeName}".
+// Example: "field Ports array element pointer" -> "field Ports array element type of int32".
+func buildArrayElementPrefix(prefix, typeName string) string {
+	idx := strings.Index(prefix, "array element")
+	if idx == -1 {
+		return prefix
+	}
+
+	return prefix[:idx+len("array element")] + " type of " + typeName
+}


Why are we doing this?

removed 👍🏻

…ames, and other reviews

JoelSpeed

One suggestion for a generics improvement, but then I think this is good to go, thanks @krishagarwal278

JoelSpeed · 2025-11-12T19:57:49Z

pkg/analysis/numericbounds/analyzer.go

+// checkBoundInRange checks if an integer bound value is within the valid range.
+// Uses generics to avoid type conversions.
+func checkBoundInRange[T constraints.Integer](pass *analysis.Pass, field *ast.Field, prefix string, value float64, minBound, maxBound T, boundType, typeName string, extraMsg ...string) {
+	if value < float64(minBound) || value > float64(maxBound) {
+		msg := fmt.Sprintf("%s has %s bound %%v that is outside the %s range [%%d, %%d]", prefix, boundType, typeName)
+		if len(extraMsg) > 0 {
+			msg += ". " + extraMsg[0]
+		}
+
+		pass.Reportf(field.Pos(), msg, value, minBound, maxBound)
+	}
+}
+
+// checkFloatBoundInRange checks if a float bound value is within the valid range.
+func checkFloatBoundInRange[T constraints.Float](pass *analysis.Pass, field *ast.Field, prefix string, value float64, minBound, maxBound T, boundType, typeName string) {
+	if value < float64(minBound) || value > float64(maxBound) {
+		pass.Reportf(field.Pos(), "%s has %s bound %v that is outside the valid %s range", prefix, boundType, value, typeName)
+	}
+}


Check how we did the GetMarkerNumericValue function implementation to work for both floats and integers, we should unify these using the same pattern (a shared number interface that accepts both floats and integers)

agreed. Latest commit now has the same pattern 👍🏻 i.e. one function for both int and float bounds

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2025

k8s-ci-robot requested review from everettraven and sivchari October 31, 2025 22:12

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 31, 2025

krishagarwal278 changed the title ~~[WIP]: NumericBounds Linter~~ NumericBounds Linter Nov 3, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 3, 2025

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 3, 2025

JoelSpeed reviewed Nov 3, 2025

View reviewed changes

krishagarwal278 force-pushed the numericbounds branch 2 times, most recently from 16a7234 to 2dd6445 Compare November 4, 2025 20:23

everettraven reviewed Nov 5, 2025

View reviewed changes

JoelSpeed reviewed Nov 6, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2025

krishagarwal278 force-pushed the numericbounds branch from 8f148c2 to a68ca3b Compare November 6, 2025 20:37

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2025

sivchari reviewed Nov 7, 2025

View reviewed changes

krishagarwal278 force-pushed the numericbounds branch 2 times, most recently from 8260a81 to 9487576 Compare November 7, 2025 15:37

JoelSpeed reviewed Nov 7, 2025

View reviewed changes

krishagarwal278 force-pushed the numericbounds branch 2 times, most recently from 2aa9995 to 36f5a50 Compare November 10, 2025 19:12

krishagarwal278 requested a review from everettraven November 11, 2025 19:57

krishagarwal278 added 5 commits November 12, 2025 11:58

added the numeric bounds linter

ef45cce

reorganize code to mimic project conventions

fcd8ab6

Added float32/64, moved docs, and followed other conventions as discu…

626dbb7

…ssed in reviews

added float64 and but removed unused int64. Updated Docs, initializer…

e73e30a

…, and used utils

Type casting for safeInt64

798e62a

krishagarwal278 force-pushed the numericbounds branch from 36f5a50 to 68d9c70 Compare November 12, 2025 16:58

JoelSpeed reviewed Nov 12, 2025

View reviewed changes

generics for bound checks, typos, updated tests for qualified field n…

a6609de

…ames, and other reviews

krishagarwal278 force-pushed the numericbounds branch from 68d9c70 to a6609de Compare November 12, 2025 19:20

JoelSpeed reviewed Nov 12, 2025

View reviewed changes

krishagarwal278 added 2 commits November 12, 2025 15:33

checkBoundInRange: one function for int and float

830ee74

typechecker uses qualifiedFieldName

bdb99e5


		## NumericBounds

		The `numericbounds` linter checks that numeric fields (`int32` and `int64`) have appropriate bounds validation markers.

		maxSafeInt = 9007199254740991 // 2^53 - 1
		minSafeInt = -9007199254740991 // -(2^53 - 1)

	// NewTypeChecker returns a new TypeChecker with the provided checkFunc.
	func NewTypeChecker(checkFunc func(pass analysis.Pass, ident ast.Ident, node ast.Node, prefix string)) TypeChecker {
	return &typeChecker{
	checkFunc: checkFunc,
	}
	}

	// IsArrayTypeOrAlias checks if the field type is an array type or an alias to an array type.
	func IsArrayTypeOrAlias(pass analysis.Pass, field ast.Field) bool {
	if _, ok := field.Type.(*ast.ArrayType); ok {
	return true
	}

	if ident, ok := field.Type.(*ast.Ident); ok {
	typeOf := pass.TypesInfo.TypeOf(ident)
	if typeOf == nil {
	return false
	}

	return isArrayType(typeOf)
	}

	return false
	}

		maxSafeInt32 = 2147483647 // 2^31 - 1 (fits in JS Number)
		minSafeInt32 = -2147483648 // -2^31 (fits in JS Number)

		maxFloat32 = 3.40282346638528859811704183484516925440e+38 // max float32
		minFloat32 = -3.40282346638528859811704183484516925440e+38 // min float32

		- Numeric fields have both `+kubebuilder:validation:Minimum` and `+kubebuilder:validation:Maximum` markers
		- K8s declarative validation markers (`+k8s:minimum` and `+k8s:maximum`) are also supported


		Note: While `+k8s:minimum` is documented in the official Kubernetes declarative validation spec, `+k8s:maximum` is not yet officially documented but is supported by this linter for forward compatibility and consistency.

		This linter is not enabled by default as it is primarily focused on CRD validation with kubebuilder markers.

NumericBounds Linter #176

Are you sure you want to change the base?

NumericBounds Linter #176

Conversation

krishagarwal278 commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!