diff --git a/pkg/crd/markers/validation.go b/pkg/crd/markers/validation.go index 78ae5e087..5d1496189 100644 --- a/pkg/crd/markers/validation.go +++ b/pkg/crd/markers/validation.go @@ -17,9 +17,9 @@ limitations under the License. package markers import ( - "fmt" - "encoding/json" + "fmt" + "math" apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -37,7 +37,7 @@ const ( // reusable and writing complex validations on slice items. var ValidationMarkers = mustMakeAllWithPrefix("kubebuilder:validation", markers.DescribesField, - // integer markers + // numeric markers Maximum(0), Minimum(0), @@ -123,11 +123,19 @@ func init() { // +controllertools:marker:generateHelp:category="CRD validation" // Maximum specifies the maximum numeric value that this field can have. -type Maximum int +type Maximum float64 + +func (m Maximum) Value() float64 { + return float64(m) +} // +controllertools:marker:generateHelp:category="CRD validation" -// Minimum specifies the minimum numeric value that this field can have. Negative integers are supported. -type Minimum int +// Minimum specifies the minimum numeric value that this field can have. Negative numbers are supported. +type Minimum float64 + +func (m Minimum) Value() float64 { + return float64(m) +} // +controllertools:marker:generateHelp:category="CRD validation" // ExclusiveMinimum indicates that the minimum is "up to" but not including that value. @@ -139,7 +147,11 @@ type ExclusiveMaximum bool // +controllertools:marker:generateHelp:category="CRD validation" // MultipleOf specifies that this field must have a numeric value that's a multiple of this one. -type MultipleOf int +type MultipleOf float64 + +func (m MultipleOf) Value() float64 { + return float64(m) +} // +controllertools:marker:generateHelp:category="CRD validation" // MaxLength specifies the maximum length for this string. @@ -252,6 +264,14 @@ type XIntOrString struct{} // to be used only as a last resort. type Schemaless struct{} +func hasNumericType(schema *apiext.JSONSchemaProps) bool { + return schema.Type == "integer" || schema.Type == "number" +} + +func isIntegral(value float64) bool { + return value == math.Trunc(value) && !math.IsNaN(value) && !math.IsInf(value, 0) +} + // +controllertools:marker:generateHelp:category="CRD validation" // XValidation marks a field as requiring a value for which a given // expression evaluates to true. @@ -264,40 +284,60 @@ type XValidation struct { } func (m Maximum) ApplyToSchema(schema *apiext.JSONSchemaProps) error { - if schema.Type != "integer" { - return fmt.Errorf("must apply maximum to an integer") + if !hasNumericType(schema) { + return fmt.Errorf("must apply maximum to a numeric value, found %s", schema.Type) + } + + if schema.Type == "integer" && !isIntegral(m.Value()) { + return fmt.Errorf("cannot apply non-integral maximum validation (%v) to integer value", m.Value()) } - val := float64(m) + + val := m.Value() schema.Maximum = &val return nil } + func (m Minimum) ApplyToSchema(schema *apiext.JSONSchemaProps) error { - if schema.Type != "integer" { - return fmt.Errorf("must apply minimum to an integer") + if !hasNumericType(schema) { + return fmt.Errorf("must apply minimum to a numeric value, found %s", schema.Type) + } + + if schema.Type == "integer" && !isIntegral(m.Value()) { + return fmt.Errorf("cannot apply non-integral minimum validation (%v) to integer value", m.Value()) } - val := float64(m) + + val := m.Value() schema.Minimum = &val return nil } + func (m ExclusiveMaximum) ApplyToSchema(schema *apiext.JSONSchemaProps) error { - if schema.Type != "integer" { - return fmt.Errorf("must apply exclusivemaximum to an integer") + if !hasNumericType(schema) { + return fmt.Errorf("must apply exclusivemaximum to a numeric value, found %s", schema.Type) } schema.ExclusiveMaximum = bool(m) return nil } + func (m ExclusiveMinimum) ApplyToSchema(schema *apiext.JSONSchemaProps) error { - if schema.Type != "integer" { - return fmt.Errorf("must apply exclusiveminimum to an integer") + if !hasNumericType(schema) { + return fmt.Errorf("must apply exclusiveminimum to a numeric value, found %s", schema.Type) } + schema.ExclusiveMinimum = bool(m) return nil } + func (m MultipleOf) ApplyToSchema(schema *apiext.JSONSchemaProps) error { - if schema.Type != "integer" { - return fmt.Errorf("must apply multipleof to an integer") + if !hasNumericType(schema) { + return fmt.Errorf("must apply multipleof to a numeric value, found %s", schema.Type) } - val := float64(m) + + if schema.Type == "integer" && !isIntegral(m.Value()) { + return fmt.Errorf("cannot apply non-integral multipleof validation (%v) to integer value", m.Value()) + } + + val := m.Value() schema.MultipleOf = &val return nil } @@ -310,6 +350,7 @@ func (m MaxLength) ApplyToSchema(schema *apiext.JSONSchemaProps) error { schema.MaxLength = &val return nil } + func (m MinLength) ApplyToSchema(schema *apiext.JSONSchemaProps) error { if schema.Type != "string" { return fmt.Errorf("must apply minlength to a string") @@ -318,6 +359,7 @@ func (m MinLength) ApplyToSchema(schema *apiext.JSONSchemaProps) error { schema.MinLength = &val return nil } + func (m Pattern) ApplyToSchema(schema *apiext.JSONSchemaProps) error { // Allow string types or IntOrStrings. An IntOrString will still // apply the pattern validation when a string is detected, the pattern @@ -337,6 +379,7 @@ func (m MaxItems) ApplyToSchema(schema *apiext.JSONSchemaProps) error { schema.MaxItems = &val return nil } + func (m MinItems) ApplyToSchema(schema *apiext.JSONSchemaProps) error { if schema.Type != "array" { return fmt.Errorf("must apply minitems to an array") @@ -345,6 +388,7 @@ func (m MinItems) ApplyToSchema(schema *apiext.JSONSchemaProps) error { schema.MinItems = &val return nil } + func (m UniqueItems) ApplyToSchema(schema *apiext.JSONSchemaProps) error { if schema.Type != "array" { return fmt.Errorf("must apply uniqueitems to an array") @@ -388,6 +432,7 @@ func (m Enum) ApplyToSchema(schema *apiext.JSONSchemaProps) error { schema.Enum = vals return nil } + func (m Format) ApplyToSchema(schema *apiext.JSONSchemaProps) error { schema.Format = string(m) return nil diff --git a/pkg/crd/markers/zz_generated.markerhelp.go b/pkg/crd/markers/zz_generated.markerhelp.go index 9a007e792..a3c054748 100644 --- a/pkg/crd/markers/zz_generated.markerhelp.go +++ b/pkg/crd/markers/zz_generated.markerhelp.go @@ -215,7 +215,7 @@ func (Minimum) Help() *markers.DefinitionHelp { return &markers.DefinitionHelp{ Category: "CRD validation", DetailedHelp: markers.DetailedHelp{ - Summary: "specifies the minimum numeric value that this field can have. Negative integers are supported.", + Summary: "specifies the minimum numeric value that this field can have. Negative numbers are supported.", Details: "", }, FieldHelp: map[string]markers.DetailedHelp{}, diff --git a/pkg/crd/parser_integration_test.go b/pkg/crd/parser_integration_test.go index 2828c3298..b9fcab09f 100644 --- a/pkg/crd/parser_integration_test.go +++ b/pkg/crd/parser_integration_test.go @@ -75,6 +75,7 @@ var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func Collector: &markers.Collector{Registry: reg}, Checker: &loader.TypeChecker{}, IgnoreUnexportedFields: true, + AllowDangerousTypes: true, // need to allow “dangerous types” in this file for testing } crd.AddKnownTypes(parser) @@ -189,6 +190,5 @@ var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func By("checking that no errors occurred along the way (expect for type errors)") Expect(packageErrors(cronJobPkg, packages.TypeError)).NotTo(HaveOccurred()) - }) }) diff --git a/pkg/crd/testdata/cronjob_types.go b/pkg/crd/testdata/cronjob_types.go index 9edf68a6f..66222f6ea 100644 --- a/pkg/crd/testdata/cronjob_types.go +++ b/pkg/crd/testdata/cronjob_types.go @@ -16,7 +16,7 @@ limitations under the License. // TODO(directxman12): test this across both versions (right now we're just // trusting k/k conversion, which is probably fine though) -//go:generate ../../../.run-controller-gen.sh crd:ignoreUnexportedFields=true paths=./;./deprecated;./unserved output:dir=. +//go:generate ../../../.run-controller-gen.sh crd:ignoreUnexportedFields=true,allowDangerousTypes=true paths=./;./deprecated;./unserved output:dir=. // +groupName=testdata.kubebuilder.io // +versionName=v1 @@ -182,6 +182,26 @@ type CronJobSpec struct { // Maps of arrays of things-that-aren’t-strings are permitted MapOfArraysOfFloats map[string][]bool `json:"mapOfArraysOfFloats,omitempty"` + // +kubebuilder:validation:Minimum=-0.5 + // +kubebuilder:validation:Maximum=1.5 + // +kubebuilder:validation:MultipleOf=0.5 + FloatWithValidations float64 `json:"floatWithValidations"` + + // +kubebuilder:validation:Minimum=-0.5 + // +kubebuilder:validation:Maximum=1.5 + // +kubebuilder:validation:MultipleOf=0.5 + Float64WithValidations float64 `json:"float64WithValidations"` + + // +kubebuilder:validation:Minimum=-2 + // +kubebuilder:validation:Maximum=2 + // +kubebuilder:validation:MultipleOf=2 + IntWithValidations int `json:"intWithValidations"` + + // +kubebuilder:validation:Minimum=-2 + // +kubebuilder:validation:Maximum=2 + // +kubebuilder:validation:MultipleOf=2 + Int32WithValidations int32 `json:"int32WithValidations"` + // This tests that unexported fields are skipped in the schema generation unexportedField string diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml index a69e7bc4b..a4b031f47 100644 --- a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml +++ b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml @@ -139,10 +139,26 @@ spec: a pointer to distinguish between explicit zero and not specified. format: int32 type: integer + float64WithValidations: + maximum: 1.5 + minimum: -0.5 + multipleOf: 0.5 + type: number + floatWithValidations: + maximum: 1.5 + minimum: -0.5 + multipleOf: 0.5 + type: number foo: description: This tests that exported fields are not skipped in the schema generation type: string + int32WithValidations: + format: int32 + maximum: 2 + minimum: -2 + multipleOf: 2 + type: integer intOrStringWithAPattern: anyOf: - type: integer @@ -153,6 +169,11 @@ spec: for having a pattern on this type. pattern: ^((100|[0-9]{1,2})%|[0-9]+)$ x-kubernetes-int-or-string: true + intWithValidations: + maximum: 2 + minimum: -2 + multipleOf: 2 + type: integer jobTemplate: description: Specifies the job that will be created when executing a CronJob. @@ -7346,7 +7367,11 @@ spec: - defaultedSlice - defaultedString - embeddedResource + - float64WithValidations + - floatWithValidations - foo + - int32WithValidations + - intWithValidations - jobTemplate - mapOfInfo - patternObject diff --git a/pkg/markers/parse.go b/pkg/markers/parse.go index fcb33925e..3e1d75a83 100644 --- a/pkg/markers/parse.go +++ b/pkg/markers/parse.go @@ -84,6 +84,8 @@ const ( InvalidType ArgumentType = iota // IntType is an int IntType + // NumberType is a float64 + NumberType // StringType is a string StringType // BoolType is a bool @@ -127,6 +129,8 @@ func (a Argument) typeString(out *strings.Builder) { out.WriteString("") case IntType: out.WriteString("int") + case NumberType: + out.WriteString("float64") case StringType: out.WriteString("string") case BoolType: @@ -180,6 +184,8 @@ func makeSliceType(itemType Argument) (reflect.Type, error) { switch itemType.Type { case IntType: itemReflectedType = reflect.TypeOf(int(0)) + case NumberType: + itemReflectedType = reflect.TypeOf(float64(0)) case StringType: itemReflectedType = reflect.TypeOf("") case BoolType: @@ -215,6 +221,8 @@ func makeMapType(itemType Argument) (reflect.Type, error) { switch itemType.Type { case IntType: itemReflectedType = reflect.TypeOf(int(0)) + case NumberType: + itemReflectedType = reflect.TypeOf(float64(0)) case StringType: itemReflectedType = reflect.TypeOf("") case BoolType: @@ -346,9 +354,13 @@ func guessType(scanner *sc.Scanner, raw string, allowSlice bool) *Argument { if nextTok == '-' { nextTok = subScanner.Scan() } + if nextTok == sc.Int { return &Argument{Type: IntType} } + if nextTok == sc.Float { + return &Argument{Type: NumberType} + } } // otherwise assume bare strings @@ -471,7 +483,7 @@ func (a *Argument) parseMap(scanner *sc.Scanner, raw string, out reflect.Value) func (a *Argument) parse(scanner *sc.Scanner, raw string, out reflect.Value, inSlice bool) { // nolint:gocyclo if a.Type == InvalidType { - scanner.Error(scanner, fmt.Sprintf("cannot parse invalid type")) + scanner.Error(scanner, "cannot parse invalid type") return } if a.Pointer { @@ -485,6 +497,32 @@ func (a *Argument) parse(scanner *sc.Scanner, raw string, out reflect.Value, inS // consume everything else for tok := scanner.Scan(); tok != sc.EOF; tok = scanner.Scan() { } + case NumberType: + nextChar := scanner.Peek() + isNegative := false + if nextChar == '-' { + isNegative = true + scanner.Scan() // eat the '-' + } + + tok := scanner.Scan() + if tok != sc.Float && tok != sc.Int { + scanner.Error(scanner, fmt.Sprintf("expected integer or float, got %q", scanner.TokenText())) + return + } + + text := scanner.TokenText() + if isNegative { + text = "-" + text + } + + val, err := strconv.ParseFloat(text, 64) + if err != nil { + scanner.Error(scanner, fmt.Sprintf("unable to parse number: %v", err)) + return + } + + castAndSet(out, reflect.ValueOf(val)) case IntType: nextChar := scanner.Peek() isNegative := false @@ -597,6 +635,8 @@ func ArgumentFromType(rawType reflect.Type) (Argument, error) { arg.Type = StringType case reflect.Int, reflect.Int32: // NB(directxman12): all ints in kubernetes are int32, so explicitly support that arg.Type = IntType + case reflect.Float64: + arg.Type = NumberType case reflect.Bool: arg.Type = BoolType case reflect.Slice: @@ -755,7 +795,7 @@ func (d *Definition) loadFields() error { func parserScanner(raw string, err func(*sc.Scanner, string)) *sc.Scanner { scanner := &sc.Scanner{} scanner.Init(bytes.NewBufferString(raw)) - scanner.Mode = sc.ScanIdents | sc.ScanInts | sc.ScanStrings | sc.ScanRawStrings | sc.SkipComments + scanner.Mode = sc.ScanIdents | sc.ScanInts | sc.ScanFloats | sc.ScanStrings | sc.ScanRawStrings | sc.SkipComments scanner.Error = err return scanner