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

k8s-ci-robot · 2025-10-31T22:12:15Z

[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 jpbetz 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

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?

JoelSpeed · 2025-11-03T11:17:56Z

pkg/analysis/numericbounds/analyzer.go

+	// Report any invalid marker values (e.g., non-numeric values)
+	if minErr != nil && !minMissing {
+		pass.Reportf(field.Pos(), "field %s has an invalid minimum marker: %v", fieldName, minErr)
+		return
+	}
+	if maxErr != nil && !maxMissing {
+		pass.Reportf(field.Pos(), "field %s has an invalid maximum marker: %v", fieldName, maxErr)
+		return
+	}
+
+	// Report if both markers are missing
+	if minMissing && maxMissing {
+		pass.Reportf(field.Pos(), "field %s of type %s should have minimum and maximum bounds validation markers", fieldName, ident.Name)
+		return
+	}
+
+	// Report if only one marker is present
+	if minMissing {
+		pass.Reportf(field.Pos(), "field %s of type %s has maximum but is missing minimum bounds validation marker", fieldName, ident.Name)
+		return
+	}
+	if maxMissing {
+		pass.Reportf(field.Pos(), "field %s of type %s has minimum but is missing maximum bounds validation marker", fieldName, ident.Name)
+		return
+	}


It is ok to report two different messages, one for missing Max, one for missing Min. I don't think we need the complexity of this both missing vs one missing logic you have here

agreed. This logic is duplicating messages. Now reporting only missing markers 👍🏻

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

everettraven · 2025-11-05T20:14:53Z

pkg/analysis/numericbounds/analyzer.go

+	// Get minimum and maximum marker values
+	minimum, minErr := getMarkerNumericValue(fieldMarkers, minMarkers)
+	maximum, maxErr := getMarkerNumericValue(fieldMarkers, maxMarkers)


Maybe a good opportunity to export and re-use

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

Lines 290 to 304 in 5fca1e8

// getMarkerNumericValue extracts a numeric value from the default value of a marker.

// Works for markers like MaxLength, MinLength, etc.

func getMarkerNumericValue[N number](marker markershelper.Marker) (N, error) {

rawValue, ok := marker.Expressions[""]

if !ok {

return N(0), errMarkerMissingValue

}

value, err := strconv.ParseFloat(rawValue, 64)

if err != nil {

return N(0), fmt.Errorf("error converting value to number: %w", err)

}

return N(value), nil

}

?

Did this get explored?

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 👍🏻

…ssed in reviews

…, and used utils

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?

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)

JoelSpeed · 2025-11-07T18:35:06Z

pkg/analysis/numericbounds/analyzer.go

+	// Get minimum and maximum marker values
+	minimum, minErr := getMarkerNumericValue(fieldMarkers, minMarkers)
+	maximum, maxErr := getMarkerNumericValue(fieldMarkers, maxMarkers)


Did this get explored?

JoelSpeed · 2025-11-07T18:37:44Z

pkg/analysis/numericbounds/analyzer.go

+
+// getMarkerNumericValue extracts the numeric value from the first instance of any of the given marker names.
+// Checks multiple marker names to support both kubebuilder and k8s declarative validation markers.
+func getMarkerNumericValue(markerSet markershelper.MarkerSet, markerNames []string) (float64, error) {


Does this define a set precedence for the order of markers that is preferred here? If so, what do we prefer?

Do we care about the value, or just presence of at least one marker?

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

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 added 3 commits November 7, 2025 09:34

added the numeric bounds linter

1d37946

reorganize code to mimic project conventions

e159a00

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

1ac8dbc

…ssed in reviews

krishagarwal278 force-pushed the numericbounds branch from a68ca3b to 8260a81 Compare November 7, 2025 14:34

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

9487576

…, and used utils

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

Type casting for safeInt64

6b18077

JoelSpeed reviewed Nov 7, 2025

View reviewed changes


		## 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
	}

	// getMarkerNumericValue extracts a numeric value from the default value of a marker.
	// Works for markers like MaxLength, MinLength, etc.
	func getMarkerNumericValue[N number](marker markershelper.Marker) (N, error) {
	rawValue, ok := marker.Expressions[""]
	if !ok {
	return N(0), errMarkerMissingValue
	}

	value, err := strconv.ParseFloat(rawValue, 64)
	if err != nil {
	return N(0), fmt.Errorf("error converting value to number: %w", err)
	}

	return N(value), nil
	}

		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

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!