Merge pull request #78863 from sttts/sttts-crd-embedded-resource-items-additionalProperties

apiextensions: structural schema unit test coverage

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

@@ -784,21 +784,23 @@ func (v *specStandardValidatorV3) validate(schema *apiextensions.JSONSchemaProps
 				// ignore errors here locally. They will show up for the root of the schema.
 
 				clone := runtime.DeepCopyJSONValue(interface{}(*schema.Default))
-				if !v.isInsideResourceMeta || s.XEmbeddedResource {
-					pruning.Prune(clone, s, s.XEmbeddedResource)
+				if !v.isInsideResourceMeta {
 					// If we are under metadata, there are implicitly specified fields like kind, apiVersion, metadata, labels.
 					// We cannot prune as they are pruned as well. This allows more defaults than we would like to.
 					// TODO: be precise about pruning under metadata
-				}
-				// TODO: coerce correctly if we are not at the object root, but somewhere below.
-				if err := schemaobjectmeta.Coerce(fldPath, clone, s, s.XEmbeddedResource, false); err != nil {
-					allErrs = append(allErrs, err)
-				}
-				if !reflect.DeepEqual(clone, interface{}(*schema.Default)) {
-					allErrs = append(allErrs, field.Invalid(fldPath.Child("default"), schema.Default, "must not have unknown fields"))
-				} else if s.XEmbeddedResource {
-					// validate an embedded resource
-					schemaobjectmeta.Validate(fldPath, interface{}(*schema.Default), nil, true)
+					pruning.Prune(clone, s, s.XEmbeddedResource)
+
+					// TODO: coerce correctly if we are not at the object root, but somewhere below.
+					if err := schemaobjectmeta.Coerce(fldPath, clone, s, s.XEmbeddedResource, false); err != nil {
+						allErrs = append(allErrs, err)
+					}
+
+					if !reflect.DeepEqual(clone, interface{}(*schema.Default)) {
+						allErrs = append(allErrs, field.Invalid(fldPath.Child("default"), schema.Default, "must not have unknown fields"))
+					} else if s.XEmbeddedResource {
+						// validate an embedded resource
+						schemaobjectmeta.Validate(fldPath, interface{}(*schema.Default), nil, true)
+					}
 				}
 
 				// validate the default value with user the provided schema.

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/algorithm.go

@@ -23,16 +23,18 @@ import (
 )
 
 // Coerce checks types of embedded ObjectMeta and TypeMeta and prunes unknown fields inside the former.
-// It does coerce ObjectMeta and TypeMeta at the root if includeRoot is true.
+// It does coerce ObjectMeta and TypeMeta at the root if isResourceRoot is true.
 // If dropInvalidFields is true, fields of wrong type will be dropped.
-func Coerce(pth *field.Path, obj interface{}, s *structuralschema.Structural, includeRoot, dropInvalidFields bool) *field.Error {
-	if includeRoot {
+func Coerce(pth *field.Path, obj interface{}, s *structuralschema.Structural, isResourceRoot, dropInvalidFields bool) *field.Error {
+	if isResourceRoot {
 		if s == nil {
 			s = &structuralschema.Structural{}
 		}
-		clone := *s
-		clone.XEmbeddedResource = true
-		s = &clone
+		if !s.XEmbeddedResource {
+			clone := *s
+			clone.XEmbeddedResource = true
+			s = &clone
+		}
 	}
 	c := coercer{dropInvalidFields: dropInvalidFields}
 	return c.coerce(pth, obj, s)

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation.go

@@ -28,15 +28,17 @@ import (
 )
 
 // Validate validates embedded ObjectMeta and TypeMeta.
-// It also validate those at the root if includeRoot is true.
-func Validate(pth *field.Path, obj interface{}, s *structuralschema.Structural, includeRoot bool) field.ErrorList {
-	if includeRoot {
+// It also validate those at the root if isResourceRoot is true.
+func Validate(pth *field.Path, obj interface{}, s *structuralschema.Structural, isResourceRoot bool) field.ErrorList {
+	if isResourceRoot {
 		if s == nil {
 			s = &structuralschema.Structural{}
 		}
-		clone := *s
-		clone.XEmbeddedResource = true
-		s = &clone
+		if !s.XEmbeddedResource {
+			clone := *s
+			clone.XEmbeddedResource = true
+			s = &clone
+		}
 	}
 	return validate(pth, obj, s)
 }

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta/validation_test.go

@@ -139,6 +139,20 @@ func TestValidate(t *testing.T) {
 			required("nested", "embedded", "apiVersion"),
 			required("nested", "embedded", "kind"),
 		}},
+		{name: "items", object: `
+{
+  "items": [{}]
+}`, errors: []validationMatch{
+			required("items[0]", "apiVersion"),
+			required("items[0]", "kind"),
+		}},
+		{name: "additionalProperties", object: `
+{
+  "additionalProperties": {"foo":{}}
+}`, errors: []validationMatch{
+			required("additionalProperties[foo]", "apiVersion"),
+			required("additionalProperties[foo]", "kind"),
+		}},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -151,6 +165,20 @@ func TestValidate(t *testing.T) {
 							"embedded": {Extensions: structuralschema.Extensions{XEmbeddedResource: true}},
 						},
 					},
+					"items": {
+						Items: &structuralschema.Structural{
+							Extensions: structuralschema.Extensions{XEmbeddedResource: true},
+						},
+					},
+					"additionalProperties": {
+						Generic: structuralschema.Generic{
+							AdditionalProperties: &structuralschema.StructuralOrBool{
+								Structural: &structuralschema.Structural{
+									Extensions: structuralschema.Extensions{XEmbeddedResource: true},
+								},
+							},
+						},
+					},
 				},
 			}
 

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning/algorithm.go

@@ -21,15 +21,17 @@ import (
 )
 
 // Prune removes object fields in obj which are not specified in s. It skips TypeMeta and ObjectMeta fields
-// if XEmbeddedResource is set to true, or for the root if root=true.
-func Prune(obj interface{}, s *structuralschema.Structural, root bool) {
-	if root {
+// if XEmbeddedResource is set to true, or for the root if isResourceRoot=true.
+func Prune(obj interface{}, s *structuralschema.Structural, isResourceRoot bool) {
+	if isResourceRoot {
 		if s == nil {
 			s = &structuralschema.Structural{}
 		}
-		clone := *s
-		clone.XEmbeddedResource = true
-		s = &clone
+		if !s.XEmbeddedResource {
+			clone := *s
+			clone.XEmbeddedResource = true
+			s = &clone
+		}
 	}
 	prune(obj, s)
 }

staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/pruning/algorithm_test.go

@@ -29,11 +29,11 @@ import (
 
 func TestPrune(t *testing.T) {
 	tests := []struct {
-		name                string
-		json                string
-		dontPruneMetaAtRoot bool
-		schema              *structuralschema.Structural
-		expected            string
+		name           string
+		json           string
+		isResourceRoot bool
+		schema         *structuralschema.Structural
+		expected       string
 	}{
 		{name: "empty", json: "null", expected: "null"},
 		{name: "scalar", json: "4", schema: &structuralschema.Structural{}, expected: "4"},
@@ -85,7 +85,13 @@ func TestPrune(t *testing.T) {
      "pruning": {"unspecified": "bar"},
      "preserving": {"unspecified": "bar"}
   },
-  "preservingAdditionalProperties": {
+  "preservingAdditionalPropertiesNotInheritingXPreserveUnknownFields": {
+     "foo": {
+        "specified": {"unspecified":"bar"},
+        "unspecified": "bar"
+     }
+  },
+  "preservingAdditionalPropertiesKeyPruneValues": {
      "foo": {
         "specified": {"unspecified":"bar"},
         "unspecified": "bar"
@@ -123,7 +129,8 @@ func TestPrune(t *testing.T) {
 						},
 					},
 				},
-				"preservingAdditionalProperties": {
+				"preservingAdditionalPropertiesNotInheritingXPreserveUnknownFields": {
+					// this x-kubernetes-preserve-unknown-fields is not inherited by the schema inside of additionalProperties
 					Extensions: structuralschema.Extensions{XPreserveUnknownFields: true},
 					Generic: structuralschema.Generic{
 						Type: "object",
@@ -137,6 +144,19 @@ func TestPrune(t *testing.T) {
 						},
 					},
 				},
+				"preservingAdditionalPropertiesKeyPruneValues": {
+					Generic: structuralschema.Generic{
+						Type: "object",
+						AdditionalProperties: &structuralschema.StructuralOrBool{
+							Structural: &structuralschema.Structural{
+								Generic: structuralschema.Generic{Type: "object"},
+								Properties: map[string]structuralschema.Structural{
+									"specified": {Generic: structuralschema.Generic{Type: "object"}},
+								},
+							},
+						},
+					},
+				},
 			},
 		}, expected: `
 {
@@ -154,7 +174,12 @@ func TestPrune(t *testing.T) {
      "pruning": {},
      "preserving": {"unspecified": "bar"}
   },
-  "preservingAdditionalProperties": {
+  "preservingAdditionalPropertiesNotInheritingXPreserveUnknownFields": {
+     "foo": {
+        "specified": {}
+     }
+  },
+  "preservingAdditionalPropertiesKeyPruneValues": {
      "foo": {
         "specified": {}
      }
@@ -397,7 +422,7 @@ func TestPrune(t *testing.T) {
     }
   }
 }
-`, dontPruneMetaAtRoot: true, schema: &structuralschema.Structural{
+`, isResourceRoot: true, schema: &structuralschema.Structural{
 			Generic: structuralschema.Generic{Type: "object"},
 			Properties: map[string]structuralschema.Structural{
 				"pruned": {
@@ -508,7 +533,7 @@ func TestPrune(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			Prune(in, tt.schema, tt.dontPruneMetaAtRoot)
+			Prune(in, tt.schema, tt.isResourceRoot)
 			if !reflect.DeepEqual(in, expected) {
 				var buf bytes.Buffer
 				enc := json.NewEncoder(&buf)