From 618e68ea56cb2657c218a34a8b95d9aab76fe842 Mon Sep 17 00:00:00 2001 From: George Pollard Date: Mon, 20 Sep 2021 11:44:05 +1200 Subject: [PATCH 1/4] Allow floating-point values in validations --- pkg/crd/markers/validation.go | 85 ++++++++++++++----- pkg/crd/markers/zz_generated.markerhelp.go | 2 +- pkg/crd/parser_integration_test.go | 6 +- pkg/crd/testdata/cronjob_types.go | 22 ++++- .../testdata.kubebuilder.io_cronjobs.yaml | 25 ++++++ pkg/markers/parse.go | 43 +++++++++- 6 files changed, 156 insertions(+), 27 deletions(-) diff --git a/pkg/crd/markers/validation.go b/pkg/crd/markers/validation.go index b20ff1623..8085a58e6 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), @@ -122,11 +122,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. @@ -138,7 +146,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. @@ -251,41 +263,69 @@ 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) +} + 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 } @@ -298,6 +338,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") @@ -306,6 +347,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 @@ -325,6 +367,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") @@ -333,6 +376,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") @@ -376,6 +420,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 03e3e44bf..13773f704 100644 --- a/pkg/crd/markers/zz_generated.markerhelp.go +++ b/pkg/crd/markers/zz_generated.markerhelp.go @@ -214,7 +214,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 0903ec0cf..d010306de 100644 --- a/pkg/crd/parser_integration_test.go +++ b/pkg/crd/parser_integration_test.go @@ -72,8 +72,9 @@ var _ = Describe("CRD Generation From Parsing to CustomResourceDefinition", func reg := &markers.Registry{} Expect(crdmarkers.Register(reg)).To(Succeed()) parser := &crd.Parser{ - Collector: &markers.Collector{Registry: reg}, - Checker: &loader.TypeChecker{}, + Collector: &markers.Collector{Registry: reg}, + Checker: &loader.TypeChecker{}, + AllowDangerousTypes: true, // need to allow “dangerous types” in this file for testing } crd.AddKnownTypes(parser) @@ -188,6 +189,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 f7aac8cb1..a8fa74e03 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 paths=./;./deprecated;./unserved output:dir=. +//go:generate ../../../.run-controller-gen.sh crd:allowDangerousTypes=true paths=./;./deprecated;./unserved output:dir=. // +groupName=testdata.kubebuilder.io // +versionName=v1 @@ -181,6 +181,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"` } type ContainsNestedMap struct { diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml index ceb55772f..d9d806ce7 100644 --- a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml +++ b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml @@ -125,6 +125,22 @@ 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 + int32WithValidations: + format: int32 + maximum: 2 + minimum: -2 + multipleOf: 2 + type: integer intOrStringWithAPattern: anyOf: - type: integer @@ -135,6 +151,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. @@ -7319,6 +7340,10 @@ spec: - defaultedSlice - defaultedString - embeddedResource + - float64WithValidations + - floatWithValidations + - int32WithValidations + - intWithValidations - jobTemplate - mapOfInfo - patternObject diff --git a/pkg/markers/parse.go b/pkg/markers/parse.go index fcb33925e..527de2cbd 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,8 +354,11 @@ func guessType(scanner *sc.Scanner, raw string, allowSlice bool) *Argument { if nextTok == '-' { nextTok = subScanner.Scan() } + if nextTok == sc.Int { return &Argument{Type: IntType} + } else if nextTok == sc.Float { + return &Argument{Type: NumberType} } } @@ -471,7 +482,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 +496,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 +634,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 +794,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.ScanStrings | sc.ScanRawStrings | sc.SkipComments | sc.ScanFloats scanner.Error = err return scanner From 8f0e6d3f6845a5a62ddc8f0c0ea48f5bb70173f4 Mon Sep 17 00:00:00 2001 From: George Pollard Date: Thu, 28 Apr 2022 22:16:41 +0000 Subject: [PATCH 2/4] Fix 'go generate' invocation --- pkg/crd/testdata/cronjob_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/crd/testdata/cronjob_types.go b/pkg/crd/testdata/cronjob_types.go index e1f64e4d7..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 crd:allowDangerousTypes=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 From 7fc2acb7343d2608f50c964a59991fdf70d1750a Mon Sep 17 00:00:00 2001 From: George Pollard Date: Mon, 2 May 2022 08:37:22 +1200 Subject: [PATCH 3/4] Order values better Co-authored-by: Joe Lanford --- pkg/markers/parse.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/markers/parse.go b/pkg/markers/parse.go index 527de2cbd..0f92fc71a 100644 --- a/pkg/markers/parse.go +++ b/pkg/markers/parse.go @@ -794,7 +794,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 | sc.ScanFloats + scanner.Mode = sc.ScanIdents | sc.ScanInts | sc.ScanFloats | sc.ScanStrings | sc.ScanRawStrings | sc.SkipComments scanner.Error = err return scanner From 4b9933026a3f143e08192ee80de37a0b006212d0 Mon Sep 17 00:00:00 2001 From: George Pollard Date: Mon, 2 May 2022 08:37:36 +1200 Subject: [PATCH 4/4] =?UTF-8?q?else=20if=20=E2=86=92=20if?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Joe Lanford --- pkg/markers/parse.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/markers/parse.go b/pkg/markers/parse.go index 0f92fc71a..3e1d75a83 100644 --- a/pkg/markers/parse.go +++ b/pkg/markers/parse.go @@ -357,7 +357,8 @@ func guessType(scanner *sc.Scanner, raw string, allowSlice bool) *Argument { if nextTok == sc.Int { return &Argument{Type: IntType} - } else if nextTok == sc.Float { + } + if nextTok == sc.Float { return &Argument{Type: NumberType} } }