Merge pull request #103402 from ilyee/validation-test

Add unit tests for validateStructuralInvariants

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" {
+	if metadata, found := s.Properties["metadata"]; found {
-			allErrs = append(allErrs, field.Invalid(fldPath.Child("properties").Key("metadata").Child("type"), metadata.Type, "must be object"))
+		allErrs = append(allErrs, validateStructuralMetadataInvariants(&metadata, checkMetadata, lvl, fldPath.Child("properties").Key("metadata"))...)
-		}
-	}
-	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 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

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(