From 8264dbe17d04243bae45e32885ca7d921379d294 Mon Sep 17 00:00:00 2001 From: elihe Date: Thu, 1 Jul 2021 19:06:05 +0800 Subject: [PATCH] Add unit tests for validateStructuralInvariants --- .../pkg/apiserver/schema/validation.go | 56 ++++---- .../pkg/apiserver/schema/validation_test.go | 123 ++++++++++++++++++ 2 files changed, 156 insertions(+), 23 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go index 73c847e8915..6a8be321b40 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation.go @@ -145,29 +145,9 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path) allErrs = append(allErrs, field.Invalid(fldPath.Child("properties").Key("apiVersion").Child("type"), apiVersion.Type, "must be string")) } } - if metadata, found := s.Properties["metadata"]; found && checkMetadata { - if metadata.Type != "object" { - allErrs = append(allErrs, field.Invalid(fldPath.Child("properties").Key("metadata").Child("type"), metadata.Type, "must be object")) - } - } - if metadata, found := s.Properties["metadata"]; found && lvl == rootLevel { - // metadata is a shallow copy. We can mutate it. - _, foundName := metadata.Properties["name"] - _, foundGenerateName := metadata.Properties["generateName"] - if foundName && foundGenerateName && len(metadata.Properties) == 2 { - metadata.Properties = nil - } else if (foundName || foundGenerateName) && len(metadata.Properties) == 1 { - metadata.Properties = nil - } - metadata.Type = "" - metadata.Default.Object = nil // this is checked in API validation (and also tested) - if metadata.ValueValidation == nil { - metadata.ValueValidation = &ValueValidation{} - } - if !reflect.DeepEqual(metadata, Structural{ValueValidation: &ValueValidation{}}) { - // TODO: this is actually a field.Invalid error, but we cannot do JSON serialization of metadata here to get a proper message - allErrs = append(allErrs, field.Forbidden(fldPath.Child("properties").Key("metadata"), "must not specify anything other than name and generateName, but metadata is implicitly specified")) - } + + if metadata, found := s.Properties["metadata"]; found { + allErrs = append(allErrs, validateStructuralMetadataInvariants(&metadata, checkMetadata, lvl, fldPath.Child("properties").Key("metadata"))...) } if s.XEmbeddedResource && !s.XPreserveUnknownFields && len(s.Properties) == 0 { @@ -177,6 +157,36 @@ func validateStructuralInvariants(s *Structural, lvl level, fldPath *field.Path) return allErrs } +func validateStructuralMetadataInvariants(s *Structural, checkMetadata bool, lvl level, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + if checkMetadata && s.Type != "object" { + allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), s.Type, "must be object")) + } + + if lvl == rootLevel { + // metadata is a shallow copy. We can mutate it. + _, foundName := s.Properties["name"] + _, foundGenerateName := s.Properties["generateName"] + if foundName && foundGenerateName && len(s.Properties) == 2 { + s.Properties = nil + } else if (foundName || foundGenerateName) && len(s.Properties) == 1 { + s.Properties = nil + } + s.Type = "" + s.Default.Object = nil // this is checked in API validation (and also tested) + if s.ValueValidation == nil { + s.ValueValidation = &ValueValidation{} + } + if !reflect.DeepEqual(*s, Structural{ValueValidation: &ValueValidation{}}) { + // TODO: this is actually a field.Invalid error, but we cannot do JSON serialization of metadata here to get a proper message + allErrs = append(allErrs, field.Forbidden(fldPath, "must not specify anything other than name and generateName, but metadata is implicitly specified")) + } + } + + return allErrs +} + func isIntOrStringAnyOfPattern(s *Structural) bool { if s == nil || s.ValueValidation == nil { return false diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go index 15c7118e49b..7b3595f8cb9 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/validation_test.go @@ -20,9 +20,132 @@ import ( "reflect" "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + fuzz "github.com/google/gofuzz" ) +func TestValidateStructuralMetadataInvariants(t *testing.T) { + fuzzer := fuzz.New() + fuzzer.Funcs( + func(s *JSON, c fuzz.Continue) { + if c.RandBool() { + s.Object = float64(42.0) + } + }, + func(s **StructuralOrBool, c fuzz.Continue) { + if c.RandBool() { + *s = &StructuralOrBool{} + } + }, + func(s **Structural, c fuzz.Continue) { + if c.RandBool() { + *s = &Structural{} + } + }, + func(s *Structural, c fuzz.Continue) { + if c.RandBool() { + *s = Structural{} + } + }, + func(vv **NestedValueValidation, c fuzz.Continue) { + if c.RandBool() { + *vv = &NestedValueValidation{} + } + }, + func(vv *NestedValueValidation, c fuzz.Continue) { + if c.RandBool() { + *vv = NestedValueValidation{} + } + }, + ) + fuzzer.NilChance(0) + + // check that type must be object + typeNames := []string{"object", "array", "number", "integer", "boolean", "string"} + for _, typeName := range typeNames { + s := Structural{ + Generic: Generic{ + Type: typeName, + }, + } + + errs := validateStructuralMetadataInvariants(&s, true, rootLevel, nil) + if len(errs) != 0 { + t.Logf("errors returned: %v", errs) + } + if len(errs) != 0 && typeName == "object" { + t.Errorf("unexpected forbidden field validation errors for: %#v", s) + } + if len(errs) == 0 && typeName != "object" { + t.Errorf("expected forbidden field validation errors for: %#v", s) + } + } + + // check that anything other than name and generateName of ObjectMeta in metadata properties is forbidden + tt := reflect.TypeOf(metav1.ObjectMeta{}) + for i := 0; i < tt.NumField(); i++ { + property := tt.Field(i).Name + s := &Structural{ + Generic: Generic{ + Type: "object", + }, + Properties: map[string]Structural{ + property: {}, + }, + } + + errs := validateStructuralMetadataInvariants(s, true, rootLevel, nil) + if len(errs) != 0 { + t.Logf("errors returned: %v", errs) + } + if len(errs) != 0 && (property == "name" || property == "generateName") { + t.Errorf("unexpected forbidden field validation errors for: %#v", s) + } + if len(errs) == 0 && property != "name" && property != "generateName" { + t.Errorf("expected forbidden field validation errors for: %#v", s) + } + } + + // check that anything other than type and properties in metadata is forbidden + tt = reflect.TypeOf(Structural{}) + for i := 0; i < tt.NumField(); i++ { + s := Structural{} + x := reflect.ValueOf(&s).Elem() + fuzzer.Fuzz(x.Field(i).Addr().Interface()) + s.Type = "object" + s.Properties = map[string]Structural{ + "name": {}, + "generateName": {}, + } + s.Default.Object = nil // this is checked in API validation, we don't need to test it here + + valid := reflect.DeepEqual(s, Structural{ + Generic: Generic{ + Type: "object", + Default: JSON{ + Object: nil, + }, + }, + Properties: map[string]Structural{ + "name": {}, + "generateName": {}, + }, + }) + + errs := validateStructuralMetadataInvariants(s.DeepCopy(), true, rootLevel, nil) + if len(errs) != 0 { + t.Logf("errors returned: %v", errs) + } + if len(errs) != 0 && valid { + t.Errorf("unexpected forbidden field validation errors for: %#v", s) + } + if len(errs) == 0 && !valid { + t.Errorf("expected forbidden field validation errors for: %#v", s) + } + } +} + func TestValidateNestedValueValidationComplete(t *testing.T) { fuzzer := fuzz.New() fuzzer.Funcs(