From c46ffc2b422ef531e7215f9188ab74e0c84162c2 Mon Sep 17 00:00:00 2001 From: Cici Huang Date: Thu, 20 Jul 2023 02:18:22 +0000 Subject: [PATCH 1/2] Refactor jsonpath parser and add tests. Co-authored-by: Joe Betz --- .../apiextensions/validation/validation.go | 5 +- .../validation/validation_test.go | 109 +++++++----- .../pkg/apiserver/schema/cel/compilation.go | 2 +- .../pkg/apiserver/schema/cel/validation.go | 164 +++++++++--------- .../apiserver/schema/cel/validation_test.go | 144 ++++++++------- 5 files changed, 229 insertions(+), 195 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go index 33a412d965c3..d804b9b87fb1 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go @@ -1092,9 +1092,6 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must not contain line breaks")) } if len(rule.FieldPath) > 0 { - if errs := validateSimpleJSONPath(rule.FieldPath, fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath")); len(errs) > 0 { - allErrs.SchemaErrors = append(allErrs.SchemaErrors, errs...) - } if !pathValid(schema, rule.FieldPath) { allErrs.SchemaErrors = append(allErrs.SchemaErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("fieldPath"), rule.FieldPath, "fieldPath must be a valid path")) } @@ -1163,7 +1160,7 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch func pathValid(schema *apiextensions.JSONSchemaProps, path string) bool { // To avoid duplicated code and better maintain, using ValidaFieldPath func to check if the path is valid if ss, err := structuralschema.NewStructural(schema); err == nil { - _, err := cel.ValidFieldPath(path, nil, ss) + _, err := cel.ValidFieldPath(path, ss) return err == nil } return true diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go index 876f38621866..13b328a3c7c7 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go @@ -76,13 +76,6 @@ func (v validationMatch) matches(err *field.Error) bool { return err.Type == v.errorType && err.Field == v.path.String() && strings.Contains(err.Error(), v.contains) } -func (v validationMatch) empty() bool { - if v.path == nil && v.errorType == "" && v.contains == "" { - return true - } - return false -} - func strPtr(s string) *string { return &s } func TestValidateCustomResourceDefinition(t *testing.T) { @@ -4140,18 +4133,45 @@ func TestValidateCustomResourceDefinition(t *testing.T) { Type: "object", XValidations: apiextensions.ValidationRules{ { - Rule: "self.a.b.c > 0.0", - FieldPath: ".a.b.c", + Rule: "true", + FieldPath: ".foo['b.c']['c\\a']", + }, + { + Rule: "true", + FieldPath: "['a.c']", + }, + { + Rule: "true", + FieldPath: ".a.c", + }, + { + Rule: "true", + FieldPath: ".list[0]", + }, + { + Rule: "true", + FieldPath: " ", + }, + { + Rule: "true", + FieldPath: ".", + }, + { + Rule: "true", + FieldPath: "..", }, }, Properties: map[string]apiextensions.JSONSchemaProps{ - "a": { + "a.c": { + Type: "number", + }, + "foo": { Type: "object", Properties: map[string]apiextensions.JSONSchemaProps{ - "b": { + "b.c": { Type: "object", Properties: map[string]apiextensions.JSONSchemaProps{ - "c": { + "c\a": { Type: "number", }, }, @@ -4179,7 +4199,14 @@ func TestValidateCustomResourceDefinition(t *testing.T) { StoredVersions: []string{"version"}, }, }, - errors: []validationMatch{}, + errors: []validationMatch{ + invalid("spec", "validation", "openAPIV3Schema", "x-kubernetes-validations[2]", "fieldPath"), + invalid("spec", "validation", "openAPIV3Schema", "x-kubernetes-validations[3]", "fieldPath"), + invalid("spec", "validation", "openAPIV3Schema", "x-kubernetes-validations[4]", "fieldPath"), + invalid("spec", "validation", "openAPIV3Schema", "x-kubernetes-validations[4]", "fieldPath"), + invalid("spec", "validation", "openAPIV3Schema", "x-kubernetes-validations[5]", "fieldPath"), + invalid("spec", "validation", "openAPIV3Schema", "x-kubernetes-validations[6]", "fieldPath"), + }, }, { name: "x-kubernetes-validations have invalid fieldPath", @@ -4368,168 +4395,156 @@ func TestValidateFieldPath(t *testing.T) { fieldPath string pathOfFieldPath *field.Path schema *apiextensions.JSONSchemaProps - error validationMatch + errMsg string }{ { name: "Valid .a", fieldPath: ".a", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { name: "Valid .a.b", fieldPath: ".a.bbb", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { name: "Valid .foo.f1", fieldPath: ".foo.f1", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { name: "Invalid map syntax .a.b", fieldPath: ".a['bbb']", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { name: "Valid .a['bbb.c']", fieldPath: ".a['bbb.c']", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { - name: "Invalid .a['bbb.c'].a-b34", + name: "Valid .a['bbb.c'].a-b34", fieldPath: ".a['bbb.c'].a-b34", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), }, { name: "Valid .a['bbb.c']['a-b34']", fieldPath: ".a['bbb.c']['a-b34']", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { name: "Valid .a.bbb.c", fieldPath: ".a.bbb.c", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { name: "Valid .a.bbb.34", fieldPath: ".a.bbb['34']", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { name: "Invalid map key", fieldPath: ".a.foo", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "does not refer to a valid field", }, { name: "Malformed map key", fieldPath: ".a.bbb[0]", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "expected single quoted string but got 0", }, { - name: "Special field names", + name: "number in field names", fieldPath: ".a.bbb.34", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), }, { name: "Special field names", fieldPath: ".a.bbb['34']", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { name: "Valid .list", fieldPath: ".list", pathOfFieldPath: path, schema: &schema, - error: validationMatch{}, }, { name: "Invalid .list[1]", fieldPath: ".list[1]", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "expected single quoted string but got 1", }, { name: "Unsopported .list.a", fieldPath: ".list.a", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "does not refer to a valid field", }, { name: "Unsupported .list['a-b.34']", fieldPath: ".list['a-b.34']", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "does not refer to a valid field", }, { name: "Invalid .list.a-b.34", fieldPath: ".list.a-b.34", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "does not refer to a valid field", }, { name: "Missing leading dot", fieldPath: "a", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "expected [ or . but got: a", }, { name: "Nonexistent field", fieldPath: ".c", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "does not refer to a valid field", }, { name: "Duplicate dots", fieldPath: ".a..b", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "does not refer to a valid field", }, { name: "Negative array index", fieldPath: ".list[-1]", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "expected single quoted string but got -1", }, { name: "Floating-point array index", fieldPath: ".list[1.0]", pathOfFieldPath: path, schema: &schema, - error: invalid(path.String()), + errMsg: "expected single quoted string but got 1", }, } @@ -4539,13 +4554,15 @@ func TestValidateFieldPath(t *testing.T) { if err != nil { t.Fatalf("error when converting schema to structural schema: %v", err) } - _, e := celschema.ValidFieldPath(tc.fieldPath, tc.pathOfFieldPath, ss) - if e == nil && !tc.error.empty() { - t.Errorf("expected %v at %v but got nil", tc.error.errorType, tc.error.path.String()) - } else if e != nil && tc.error.empty() { - t.Errorf("unexpected error: %v at %v", tc.error.errorType, tc.error.path.String()) - } else if !tc.error.empty() && !tc.error.matches(e) { - t.Errorf("expected %v at %v, got %v", tc.error.errorType, tc.error.path.String(), err) + _, err = celschema.ValidFieldPath(tc.fieldPath, ss) + if err == nil && tc.errMsg != "" { + t.Errorf("expected err contains: %v but get nil", tc.errMsg) + } + if err != nil && tc.errMsg == "" { + t.Errorf("unexpected error: %v", err) + } + if err != nil && !strings.Contains(err.Error(), tc.errMsg) { + t.Errorf("expected error to contain: %v, but get: %v", tc.errMsg, err) } }) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go index 15d40e3e4f74..270c73c7f1e1 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go @@ -249,7 +249,7 @@ func compileRule(s *schema.Structural, rule apiextensions.ValidationRule, envSet compilationResult.MessageExpressionMaxCost = costEst.Max } if rule.FieldPath != "" { - validFieldPath, err := ValidFieldPath(rule.FieldPath, nil, s) + validFieldPath, err := ValidFieldPath(rule.FieldPath, s) if err == nil { compilationResult.NormalizedRuleFieldPath = validFieldPath.String() } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go index 94222d99fc97..3dbf3b26c0f7 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go @@ -17,13 +17,13 @@ limitations under the License. package cel import ( + "bufio" "context" "fmt" "math" "reflect" "regexp" "strings" - "text/scanner" "time" celgo "github.com/google/cel-go/cel" @@ -322,101 +322,103 @@ func unescapeSingleQuote(s string) (string, error) { return unescaped, err } -// ValidFieldPath returns a valid field path. -func ValidFieldPath(fieldPath string, pathOfFieldPath *field.Path, schema *schema.Structural) (validFieldPath *field.Path, err *field.Error) { - validFieldPath = pathOfFieldPath - if len(fieldPath) == 0 { - return pathOfFieldPath, field.Invalid(pathOfFieldPath, fieldPath, "must not be empty") +// ValidFieldPath validates that jsonPath is a valid JSON Path containing only field and map accessors +// that are valid for the given schema, and returns a field.Path representation of the validated jsonPath or an error. +func ValidFieldPath(jsonPath string, schema *schema.Structural) (validFieldPath *field.Path, err error) { + appendToPath := func(name string) error { + if schema.AdditionalProperties != nil { + validFieldPath = validFieldPath.Key(name) + schema = schema.AdditionalProperties.Structural + } else if schema.Properties != nil { + validFieldPath = validFieldPath.Child(name) + val, ok := schema.Properties[name] + if !ok { + return fmt.Errorf("does not refer to a valid field") + } + schema = &val + } else { + return fmt.Errorf("does not refer to a valid field") + } + return nil } - invalidFieldError := field.Invalid(pathOfFieldPath, fieldPath, "does not refer to an valid field") + validFieldPath = nil - var s scanner.Scanner - s.Init(strings.NewReader(fieldPath)) - s.Filename = pathOfFieldPath.String() - s.Mode = scanner.ScanInts | scanner.ScanIdents | scanner.ScanChars | scanner.ScanStrings - s.Error = func(s *scanner.Scanner, msg string) { - field.Invalid(pathOfFieldPath, fieldPath, fmt.Sprintf("failed to parse JSON Path: %s", msg)) - } + scanner := bufio.NewScanner(strings.NewReader(jsonPath)) - found := false - for true { - tok := s.Scan() - if tok == scanner.EOF { - found = true - break - } - switch schema.Type { - case "object": - isMapSyntax := false - if s.TokenText() == "[" { - isMapSyntax = true - } else if s.TokenText() != "." { - return pathOfFieldPath, field.Invalid(pathOfFieldPath, fieldPath, "expected [ or . but got: "+s.TokenText()) - } - - tok = s.Scan() - if tok == scanner.EOF { - return pathOfFieldPath, field.Invalid(pathOfFieldPath, fieldPath, "unexpected end of JSON path") - } - fieldName := s.TokenText() - if isMapSyntax { - if tok == scanner.Char { - newS := fieldName[1 : len(fieldName)-1] - newS, err := unescapeSingleQuote(newS) - if err != nil { - return pathOfFieldPath, field.Invalid(pathOfFieldPath, fieldPath, fmt.Sprintf("failed to unescape: %v", err)) + // configure the scanner to split the string into tokens. + // The three delimiters ('.', '[', ']') will be returned as single char tokens. + // All other text between delimiters is returned as string tokens. + scanner.Split(func(data []byte, atEOF bool) (advance int, token []byte, err error) { + if len(data) > 0 { + for i := 0; i < len(data); i++ { + // If in a single quoted string, look for the end of string + // ignoring delimiters. + if data[0] == '\'' { + if i > 0 && data[i] == '\'' && data[i-1] != '\\' { + // Return quoted string + return i + 1, data[:i+1], nil } - fieldName = newS - if schema.AdditionalProperties != nil { - validFieldPath = validFieldPath.Key(fieldName) + continue + } + switch data[i] { + case '.', '[', ']': // delimiters + if i == 0 { + // Return the delimiter. + return 1, data[:1], nil } else { - validFieldPath = validFieldPath.Child(fieldName) + // Return identifier leading up to the delimiter. + // The next call to split will return the delimiter. + return i, data[:i], nil } - } else { - return pathOfFieldPath, field.Invalid(pathOfFieldPath, fieldPath, "unexpected format of fieldName: "+fieldName) - } - } else if tok != scanner.Ident { - return pathOfFieldPath, invalidFieldError - } else { - if schema.AdditionalProperties != nil { - validFieldPath = validFieldPath.Key(fieldName) - } else { - validFieldPath = validFieldPath.Child(fieldName) } } - - if schema.Properties != nil { - propertySchema, ok := schema.Properties[fieldName] - if ok { - schema = &propertySchema - } else { - return pathOfFieldPath, invalidFieldError - } - } else if schema.AdditionalProperties != nil { - schema = schema.AdditionalProperties.Structural - } else { - return pathOfFieldPath, invalidFieldError + if atEOF { + // Return the string. + return len(data), data, nil } + } + return 0, nil, nil + }) - if isMapSyntax { - if tok == scanner.EOF { - return pathOfFieldPath, field.Invalid(pathOfFieldPath, fieldPath, "unexpected end of JSON path") - } - s.Scan() - if s.TokenText() != "]" { - return pathOfFieldPath, field.Invalid(pathOfFieldPath, fieldPath, "expect ] but get: "+s.TokenText()) - } + var tok string + for scanner.Scan() { + tok = scanner.Text() + switch tok { + case "[": + if !scanner.Scan() { + return nil, fmt.Errorf("unexpected end of JSON path") + } + tok = scanner.Text() + if len(tok) < 2 || tok[0] != '\'' || tok[len(tok)-1] != '\'' { + return nil, fmt.Errorf("expected single quoted string but got %s", tok) + } + unescaped, err := unescapeSingleQuote(tok[1 : len(tok)-1]) + if err != nil { + return nil, fmt.Errorf("invalid string literal: %v", err) + } + if err := appendToPath(unescaped); err != nil { + return nil, err + } + if !scanner.Scan() { + return nil, fmt.Errorf("unexpected end of JSON path") + } + tok = scanner.Text() + if tok != "]" { + return nil, fmt.Errorf("expected ] but got %s", tok) + } + case ".": + if !scanner.Scan() { + return nil, fmt.Errorf("unexpected end of JSON path") + } + tok = scanner.Text() + if err := appendToPath(tok); err != nil { + return nil, err } default: - return pathOfFieldPath, invalidFieldError + return nil, fmt.Errorf("expected [ or . but got: %s", tok) } } - - if !found { - return pathOfFieldPath, invalidFieldError - } - return validFieldPath, nil } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go index 6c5c16e7dd5c..57117b9a9dd1 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go @@ -2634,7 +2634,7 @@ func TestReasonAndFldPath(t *testing.T) { }, }, schema: withRulePtr(objectTypePtr(map[string]schema.Structural{ - "f": withReasonAndFldPath(objectType(map[string]schema.Structural{"m": integerType}), "self.m == 2", ". m", forbiddenReason), + "f": withReasonAndFldPath(objectType(map[string]schema.Structural{"m": integerType}), "self.m == 2", ".m", forbiddenReason), }), "1 == 1"), errorType: field.ErrorTypeForbidden, errors: []string{"root.f.m: Forbidden"}, @@ -2701,6 +2701,11 @@ func TestValidateFieldPath(t *testing.T) { }, }, }, + "white space": { + Generic: schema.Generic{ + Type: "number", + }, + }, "a": { Generic: schema.Generic{ Type: "object", @@ -2716,6 +2721,11 @@ func TestValidateFieldPath(t *testing.T) { Type: "number", }, }, + "bb[b": { + Generic: schema.Generic{ + Type: "number", + }, + }, "bbb": { Generic: schema.Generic{ Type: "object", @@ -2779,39 +2789,75 @@ func TestValidateFieldPath(t *testing.T) { fieldPath string pathOfFieldPath *field.Path schema *schema.Structural - error validationMatch + errDetail string validFieldPath *field.Path }{ { name: "Valid .a", - fieldPath: ". a ", + fieldPath: ".a", pathOfFieldPath: path, schema: &sts, - error: validationMatch{}, validFieldPath: path.Child("a"), }, { - name: "Valid .a.bbb", - fieldPath: ". a. bbb", + name: "Invalid with whitespace", + fieldPath: ". a", + pathOfFieldPath: path, + schema: &sts, + errDetail: "does not refer to a valid field", + }, + { + name: "Valid with whitespace inside field", + fieldPath: ".white space", + pathOfFieldPath: path, + schema: &sts, + }, + { + name: "Valid with whitespace inside field", + fieldPath: "['white space']", + pathOfFieldPath: path, + schema: &sts, + }, + { + name: "invalid dot annotation", + fieldPath: ".a.bb[b", + pathOfFieldPath: path, + schema: &sts, + errDetail: "does not refer to a valid field", + }, + { + name: "valid with .", + fieldPath: ".a['bbb.c']", pathOfFieldPath: path, schema: &sts, - error: validationMatch{}, - validFieldPath: path.Child("a", "bbb"), + validFieldPath: path.Child("a", "bbb.c"), + }, + { + name: "Unclosed ]", + fieldPath: ".a['bbb.c'", + pathOfFieldPath: path, + schema: &sts, + errDetail: "unexpected end of JSON path", + }, + { + name: "Unexpected end of JSON path", + fieldPath: ".", + pathOfFieldPath: path, + schema: &sts, + errDetail: "unexpected end of JSON path", }, { name: "Valid map syntax .a.bbb", - fieldPath: ".a['bbb']", + fieldPath: ".a['bbb.c']", pathOfFieldPath: path, schema: &sts, - error: validationMatch{}, - validFieldPath: path.Child("a").Child("bbb"), + validFieldPath: path.Child("a").Child("bbb.c"), }, { name: "Valid map key", fieldPath: ".foo.subAdd", pathOfFieldPath: path, schema: &sts, - error: validationMatch{}, validFieldPath: path.Child("foo").Key("subAdd"), }, { @@ -2819,7 +2865,6 @@ func TestValidateFieldPath(t *testing.T) { fieldPath: ".foo['subAdd']", pathOfFieldPath: path, schema: &sts, - error: validationMatch{}, validFieldPath: path.Child("foo").Key("subAdd"), }, { @@ -2827,14 +2872,12 @@ func TestValidateFieldPath(t *testing.T) { fieldPath: ".a.foo's", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), }, { name: "Escaping", fieldPath: ".a['foo\\'s']", pathOfFieldPath: path, schema: &sts, - error: validationMatch{}, validFieldPath: path.Child("a").Child("foo's"), }, { @@ -2842,7 +2885,6 @@ func TestValidateFieldPath(t *testing.T) { fieldPath: ".a['test\\a']", pathOfFieldPath: path, schema: &sts, - error: validationMatch{}, validFieldPath: path.Child("a").Child("test\a"), }, @@ -2851,35 +2893,33 @@ func TestValidateFieldPath(t *testing.T) { fieldPath: ".a.foo", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), + errDetail: "does not refer to a valid field", }, { name: "Malformed map key", fieldPath: ".a.bbb[0]", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), + errDetail: "expected single quoted string but got 0", }, { - name: "Invalid refer for special map key", + name: "Valid refer for name has number", fieldPath: ".a.bbb.34", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), }, { name: "Map syntax for special field names", fieldPath: ".a.bbb['34']", pathOfFieldPath: path, schema: &sts, - error: validationMatch{}, + //errDetail: "does not refer to a valid field", }, { name: "Valid .list", - fieldPath: ". list ", + fieldPath: ".list", pathOfFieldPath: path, schema: &sts, - error: validationMatch{}, validFieldPath: path.Child("list"), }, { @@ -2887,94 +2927,72 @@ func TestValidateFieldPath(t *testing.T) { fieldPath: ".list[0]", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), + errDetail: "expected single quoted string but got 0", }, { name: "Invalid list reference", fieldPath: ".list. a", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), + errDetail: "does not refer to a valid field", }, { name: "Invalid .list.a", fieldPath: ".list['a']", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), + errDetail: "does not refer to a valid field", }, { name: "Missing leading dot", fieldPath: "a", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), + errDetail: "expected [ or . but got: a", }, { name: "Nonexistent field", fieldPath: ".c", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), + errDetail: "does not refer to a valid field", }, { name: "Duplicate dots", fieldPath: ".a..b", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), + errDetail: "does not refer to a valid field", }, { name: "object of array", fieldPath: ".list.a-b.34", pathOfFieldPath: path, schema: &sts, - error: invalid(path.String()), + errDetail: "does not refer to a valid field", }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - validField, err := ValidFieldPath(tc.fieldPath, tc.pathOfFieldPath, tc.schema) - - if err == nil && !tc.error.empty() { - t.Errorf("expected %v at %v but got nil", tc.error.errorType, tc.error.path.String()) - } else if err != nil && tc.error.empty() { - t.Errorf("unexpected error: %v at %v", tc.error.errorType, tc.error.path.String()) - } else if !tc.error.matches(err) { - t.Errorf("expected %v at %v, got %v", tc.error.errorType, tc.error.path.String(), err) + validField, err := ValidFieldPath(tc.fieldPath, tc.schema) + + if err == nil && tc.errDetail != "" { + t.Errorf("expected err contains: %v but get nil", tc.errDetail) + } + if err != nil && tc.errDetail == "" { + t.Errorf("unexpected error: %v", err) } - if tc.validFieldPath != nil && tc.validFieldPath.String() != validField.String() { + if err != nil && !strings.Contains(err.Error(), tc.errDetail) { + t.Errorf("expected error to contain: %v, but get: %v", tc.errDetail, err) + } + if tc.validFieldPath != nil && tc.validFieldPath.String() != path.Child(validField.String()).String() { t.Errorf("expected %v, got %v", tc.validFieldPath, validField) } }) } } -type validationMatch struct { - path *field.Path - errorType field.ErrorType - contains string -} - -func invalid(path ...string) validationMatch { - return validationMatch{path: field.NewPath(path[0], path[1:]...), errorType: field.ErrorTypeInvalid} -} - -func (v validationMatch) matches(err *field.Error) bool { - if err == nil && v.empty() { - return true - } - return err.Type == v.errorType && err.Field == v.path.String() && strings.Contains(err.Error(), v.contains) -} - -func (v validationMatch) empty() bool { - if v.path == nil && v.errorType == "" && v.contains == "" { - return true - } - return false -} - func genString(n int, c rune) string { b := strings.Builder{} for i := 0; i < n; i++ { From f50e74206e117857878170a57de5abadcc3d0b97 Mon Sep 17 00:00:00 2001 From: Cici Huang Date: Thu, 20 Jul 2023 21:36:44 +0000 Subject: [PATCH 2/2] Address comment --- .../pkg/apiserver/schema/cel/validation.go | 28 ++++++++++++++----- .../apiserver/schema/cel/validation_test.go | 21 +++++++++++++- 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go index 3dbf3b26c0f7..60e0de8ac90c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go @@ -325,19 +325,17 @@ func unescapeSingleQuote(s string) (string, error) { // ValidFieldPath validates that jsonPath is a valid JSON Path containing only field and map accessors // that are valid for the given schema, and returns a field.Path representation of the validated jsonPath or an error. func ValidFieldPath(jsonPath string, schema *schema.Structural) (validFieldPath *field.Path, err error) { - appendToPath := func(name string) error { - if schema.AdditionalProperties != nil { + appendToPath := func(name string, isNamed bool) error { + if !isNamed { validFieldPath = validFieldPath.Key(name) schema = schema.AdditionalProperties.Structural - } else if schema.Properties != nil { + } else { validFieldPath = validFieldPath.Child(name) val, ok := schema.Properties[name] if !ok { return fmt.Errorf("does not refer to a valid field") } schema = &val - } else { - return fmt.Errorf("does not refer to a valid field") } return nil } @@ -382,6 +380,7 @@ func ValidFieldPath(jsonPath string, schema *schema.Structural) (validFieldPath }) var tok string + var isNamed bool for scanner.Scan() { tok = scanner.Text() switch tok { @@ -397,7 +396,15 @@ func ValidFieldPath(jsonPath string, schema *schema.Structural) (validFieldPath if err != nil { return nil, fmt.Errorf("invalid string literal: %v", err) } - if err := appendToPath(unescaped); err != nil { + + if schema.Properties != nil { + isNamed = true + } else if schema.AdditionalProperties != nil { + isNamed = false + } else { + return nil, fmt.Errorf("does not refer to a valid field") + } + if err := appendToPath(unescaped, isNamed); err != nil { return nil, err } if !scanner.Scan() { @@ -412,7 +419,14 @@ func ValidFieldPath(jsonPath string, schema *schema.Structural) (validFieldPath return nil, fmt.Errorf("unexpected end of JSON path") } tok = scanner.Text() - if err := appendToPath(tok); err != nil { + if schema.Properties != nil { + isNamed = true + } else if schema.AdditionalProperties != nil { + isNamed = false + } else { + return nil, fmt.Errorf("does not refer to a valid field") + } + if err := appendToPath(tok, isNamed); err != nil { return nil, err } default: diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go index 57117b9a9dd1..2ea8d38dbcfe 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation_test.go @@ -2706,6 +2706,11 @@ func TestValidateFieldPath(t *testing.T) { Type: "number", }, }, + "'foo'bar": { + Generic: schema.Generic{ + Type: "number", + }, + }, "a": { Generic: schema.Generic{ Type: "object", @@ -2799,6 +2804,20 @@ func TestValidateFieldPath(t *testing.T) { schema: &sts, validFieldPath: path.Child("a"), }, + { + name: "Valid 'foo'bar", + fieldPath: "['\\'foo\\'bar']", + pathOfFieldPath: path, + schema: &sts, + validFieldPath: path.Child("'foo'bar"), + }, + { + name: "Invalid 'foo'bar", + fieldPath: ".\\'foo\\'bar", + pathOfFieldPath: path, + schema: &sts, + errDetail: "does not refer to a valid field", + }, { name: "Invalid with whitespace", fieldPath: ". a", @@ -2987,7 +3006,7 @@ func TestValidateFieldPath(t *testing.T) { t.Errorf("expected error to contain: %v, but get: %v", tc.errDetail, err) } if tc.validFieldPath != nil && tc.validFieldPath.String() != path.Child(validField.String()).String() { - t.Errorf("expected %v, got %v", tc.validFieldPath, validField) + t.Errorf("expected %v, got %v", tc.validFieldPath, path.Child(validField.String())) } }) }