Skip to content

Commit

Permalink
Merge pull request kubernetes#119453 from cici37/addTest
Browse files Browse the repository at this point in the history 
Refactor jsonpath parser and add tests
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Jul 21, 2023
2 parents 76eefd3 + f50e742 commit 5e8dfe5
Show file tree
Hide file tree
Showing 5 changed files with 260 additions and 193 deletions.
5 changes: 1 addition & 4 deletions staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go
View file
Expand Up @@ -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"))
}
Expand Down Expand Up @@ -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
Expand Down
109 changes: 63 additions & 46 deletions ...g/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation_test.go
View file
Expand Up @@ -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) {
Expand Down Expand Up @@ -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",
},
},
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
},
}

Expand All @@ -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)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/compilation.go
View file
Expand Up @@ -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()
}
Expand Down

0 comments on commit 5e8dfe5

Please sign in to comment.