Merge pull request #79636 from liggitt/crd-preserve-unknown-openapi

Fix publishing x-kubernetes-preserve-unknown-fields working with kubectl

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder.go

@@ -297,7 +297,7 @@ func (b *builder) buildKubeNative(schema *structuralschema.Structural, v2 bool)
 	// and forbid anything outside of apiVersion, kind and metadata. We have to fix kubectl to stop doing this, e.g. by
 	// adding additionalProperties=true support to explicitly allow additional fields.
 	// TODO: fix kubectl to understand additionalProperties=true
-	if schema == nil {
+	if schema == nil || (v2 && schema.XPreserveUnknownFields) {
 		ret = &spec.Schema{
 			SchemaProps: spec.SchemaProps{Type: []string{"object"}},
 		}
@@ -311,7 +311,7 @@ func (b *builder) buildKubeNative(schema *structuralschema.Structural, v2 bool)
 		ret.SetProperty("metadata", *spec.RefSchema(objectMetaSchemaRef).
 			WithDescription(swaggerPartialObjectMetadataDescriptions["metadata"]))
 		addTypeMetaProperties(ret)
-		addEmbeddedProperties(ret)
+		addEmbeddedProperties(ret, v2)
 	}
 	ret.AddExtension(endpoints.ROUTE_META_GVK, []interface{}{
 		map[string]interface{}{
@@ -324,23 +324,28 @@ func (b *builder) buildKubeNative(schema *structuralschema.Structural, v2 bool)
 	return ret
 }
 
-func addEmbeddedProperties(s *spec.Schema) {
+func addEmbeddedProperties(s *spec.Schema, v2 bool) {
 	if s == nil {
 		return
 	}
 
 	for k := range s.Properties {
 		v := s.Properties[k]
-		addEmbeddedProperties(&v)
+		addEmbeddedProperties(&v, v2)
 		s.Properties[k] = v
 	}
 	if s.Items != nil {
-		addEmbeddedProperties(s.Items.Schema)
+		addEmbeddedProperties(s.Items.Schema, v2)
 	}
 	if s.AdditionalProperties != nil {
-		addEmbeddedProperties(s.AdditionalProperties.Schema)
+		addEmbeddedProperties(s.AdditionalProperties.Schema, v2)
 	}
 
+	if isTrue, ok := s.VendorExtensible.Extensions.GetBool("x-kubernetes-preserve-unknown-fields"); ok && isTrue && v2 {
+		// don't add metadata properties if we're publishing to openapi v2 and are allowing unknown fields.
+		// adding these metadata properties makes kubectl refuse to validate unknown fields.
+		return
+	}
 	if isTrue, ok := s.VendorExtensible.Extensions.GetBool("x-kubernetes-embedded-resource"); ok && isTrue {
 		s.SetProperty("apiVersion", withDescription(getDefinition(typeMetaType).SchemaProps.Properties["apiVersion"],
 			"apiVersion defines the versioned schema of this representation of an object. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources",

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder_test.go

@@ -59,6 +59,18 @@ func TestNewBuilder(t *testing.T) {
 			`{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`,
 			true,
 		},
+		{"preserve unknown at root v2",
+			`{"type":"object","x-kubernetes-preserve-unknown-fields":true}`,
+			`{"type":"object","x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
+			`{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`,
+			true,
+		},
+		{"preserve unknown at root v3",
+			`{"type":"object","x-kubernetes-preserve-unknown-fields":true}`,
+			`{"type":"object","x-kubernetes-preserve-unknown-fields":true,"properties":{"apiVersion":{"type":"string"},"kind":{"type":"string"},"metadata":{"$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"}},"x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]}`,
+			`{"$ref":"#/definitions/io.k8s.bar.v1.Foo"}`,
+			false,
+		},
 		{"with extensions",
 			`
 {
@@ -155,22 +167,7 @@ func TestNewBuilder(t *testing.T) {
     "embedded-object": {
       "x-kubernetes-embedded-resource": true,
       "x-kubernetes-preserve-unknown-fields": true,
-      "type": "object",
-      "required":["kind","apiVersion"],
-      "properties":{
-        "apiVersion":{
-          "description":"apiVersion defines the versioned schema of this representation of an object. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources",
-          "type":"string"
-        },
-        "kind":{
-          "description":"kind is a string value representing the type of this object. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds",
-          "type":"string"
-        },
-        "metadata":{
-          "description":"Standard object's metadata. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#metadata",
-          "$ref":"#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.ObjectMeta"
-        }
-      }
+      "type":"object"
     }
   },
   "x-kubernetes-group-version-kind":[{"group":"bar.k8s.io","kind":"Foo","version":"v1"}]

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/conversion.go

@@ -65,6 +65,14 @@ func ToStructuralOpenAPIV2(in *structuralschema.Structural) *structuralschema.St
 				changed = true
 			}
 
+			if s.XPreserveUnknownFields {
+				// unknown fields break if items or properties are set in kubectl
+				s.Items = nil
+				s.Properties = nil
+
+				changed = true
+			}
+
 			return changed
 		},
 		// we drop all junctors above, and hence, never reach nested value validations

test/e2e/apimachinery/crd_publish_openapi.go

@@ -159,6 +159,74 @@ var _ = SIGDescribe("CustomResourcePublishOpenAPI", func() {
 		}
 	})
 
+	ginkgo.It("works for CRD preserving unknown fields at the schema root", func() {
+		crd, err := setupCRDAndVerifySchema(f, schemaPreserveRoot, nil, "unknown-at-root", "v1")
+		if err != nil {
+			e2elog.Failf("%v", err)
+		}
+
+		meta := fmt.Sprintf(metaPattern, crd.Crd.Spec.Names.Kind, crd.Crd.Spec.Group, crd.Crd.Spec.Versions[0].Name, "test-cr")
+		ns := fmt.Sprintf("--namespace=%v", f.Namespace.Name)
+
+		ginkgo.By("client-side validation (kubectl create and apply) allows request with any unknown properties")
+		randomCR := fmt.Sprintf(`{%s,"a":{"b":[{"c":"d"}]}}`, meta)
+		if _, err := framework.RunKubectlInput(randomCR, ns, "create", "-f", "-"); err != nil {
+			e2elog.Failf("failed to create random CR %s for CRD that allows unknown properties at the root: %v", randomCR, err)
+		}
+		if _, err := framework.RunKubectl(ns, "delete", crd.Crd.Spec.Names.Plural, "test-cr"); err != nil {
+			e2elog.Failf("failed to delete random CR: %v", err)
+		}
+		if _, err := framework.RunKubectlInput(randomCR, ns, "apply", "-f", "-"); err != nil {
+			e2elog.Failf("failed to apply random CR %s for CRD without schema: %v", randomCR, err)
+		}
+		if _, err := framework.RunKubectl(ns, "delete", crd.Crd.Spec.Names.Plural, "test-cr"); err != nil {
+			e2elog.Failf("failed to delete random CR: %v", err)
+		}
+
+		ginkgo.By("kubectl explain works to explain CR")
+		if err := verifyKubectlExplain(crd.Crd.Spec.Names.Plural, fmt.Sprintf(`(?s)KIND:.*%s`, crd.Crd.Spec.Names.Kind)); err != nil {
+			e2elog.Failf("%v", err)
+		}
+
+		if err := cleanupCRD(f, crd); err != nil {
+			e2elog.Failf("%v", err)
+		}
+	})
+
+	ginkgo.It("works for CRD preserving unknown fields in an embedded object", func() {
+		crd, err := setupCRDAndVerifySchema(f, schemaPreserveNested, nil, "unknown-in-nested", "v1")
+		if err != nil {
+			e2elog.Failf("%v", err)
+		}
+
+		meta := fmt.Sprintf(metaPattern, crd.Crd.Spec.Names.Kind, crd.Crd.Spec.Group, crd.Crd.Spec.Versions[0].Name, "test-cr")
+		ns := fmt.Sprintf("--namespace=%v", f.Namespace.Name)
+
+		ginkgo.By("client-side validation (kubectl create and apply) allows request with any unknown properties")
+		randomCR := fmt.Sprintf(`{%s,"spec":{"b":[{"c":"d"}]}}`, meta)
+		if _, err := framework.RunKubectlInput(randomCR, ns, "create", "-f", "-"); err != nil {
+			e2elog.Failf("failed to create random CR %s for CRD that allows unknown properties in a nested object: %v", randomCR, err)
+		}
+		if _, err := framework.RunKubectl(ns, "delete", crd.Crd.Spec.Names.Plural, "test-cr"); err != nil {
+			e2elog.Failf("failed to delete random CR: %v", err)
+		}
+		if _, err := framework.RunKubectlInput(randomCR, ns, "apply", "-f", "-"); err != nil {
+			e2elog.Failf("failed to apply random CR %s for CRD without schema: %v", randomCR, err)
+		}
+		if _, err := framework.RunKubectl(ns, "delete", crd.Crd.Spec.Names.Plural, "test-cr"); err != nil {
+			e2elog.Failf("failed to delete random CR: %v", err)
+		}
+
+		ginkgo.By("kubectl explain works to explain CR")
+		if err := verifyKubectlExplain(crd.Crd.Spec.Names.Plural, `(?s)DESCRIPTION:.*preserve-unknown-properties in nested field for Testing`); err != nil {
+			e2elog.Failf("%v", err)
+		}
+
+		if err := cleanupCRD(f, crd); err != nil {
+			e2elog.Failf("%v", err)
+		}
+	})
+
 	ginkgo.It("works for multiple CRDs of different groups", func() {
 		ginkgo.By("CRs in different groups (two CRDs) show up in OpenAPI documentation")
 		crdFoo, err := setupCRD(f, schemaFoo, "foo", "v1")
@@ -337,18 +405,23 @@ var _ = SIGDescribe("CustomResourcePublishOpenAPI", func() {
 })
 
 func setupCRD(f *framework.Framework, schema []byte, groupSuffix string, versions ...string) (*crd.TestCrd, error) {
+	expect := schema
+	if schema == nil {
+		// to be backwards compatible, we expect CRD controller to treat
+		// CRD with nil schema specially and publish an empty schema
+		expect = []byte(`type: object`)
+	}
+	return setupCRDAndVerifySchema(f, schema, expect, groupSuffix, versions...)
+}
+
+func setupCRDAndVerifySchema(f *framework.Framework, schema, expect []byte, groupSuffix string, versions ...string) (*crd.TestCrd, error) {
 	group := fmt.Sprintf("%s-test-%s.k8s.io", f.BaseName, groupSuffix)
 	if len(versions) == 0 {
 		return nil, fmt.Errorf("require at least one version for CRD")
 	}
 
-	expect := schema
 	props := &v1beta1.JSONSchemaProps{}
-	if schema == nil {
-		// to be backwards compatible, we expect CRD controller to treat
-		// CRD with nil schema specially and publish an empty schema
-		expect = []byte(`type: object`)
-	} else {
+	if schema != nil {
 		if err := yaml.Unmarshal(schema, props); err != nil {
 			return nil, err
 		}
@@ -412,7 +485,8 @@ func mustSucceedMultipleTimes(n int, f func() (bool, error)) func() (bool, error
 	}
 }
 
-// waitForDefinition waits for given definition showing up in swagger with given schema
+// waitForDefinition waits for given definition showing up in swagger with given schema.
+// If schema is nil, only the existence of the given name is checked.
 func waitForDefinition(c k8sclientset.Interface, name string, schema []byte) error {
 	expect := spec.Schema{}
 	if err := convertJSONSchemaProps(schema, &expect); err != nil {
@@ -424,10 +498,12 @@ func waitForDefinition(c k8sclientset.Interface, name string, schema []byte) err
 		if !ok {
 			return false, fmt.Sprintf("spec.SwaggerProps.Definitions[\"%s\"] not found", name)
 		}
-		// drop properties and extension that we added
-		dropDefaults(&d)
-		if !apiequality.Semantic.DeepEqual(expect, d) {
-			return false, fmt.Sprintf("spec.SwaggerProps.Definitions[\"%s\"] not match; expect: %v, actual: %v", name, expect, d)
+		if schema != nil {
+			// drop properties and extension that we added
+			dropDefaults(&d)
+			if !apiequality.Semantic.DeepEqual(expect, d) {
+				return false, fmt.Sprintf("spec.SwaggerProps.Definitions[\"%s\"] not match; expect: %v, actual: %v", name, expect, d)
+			}
 		}
 		return true, ""
 	})
@@ -602,3 +678,45 @@ properties:
         type: array
         items:
           type: object`)
+
+var schemaPreserveRoot = []byte(`description: preserve-unknown-properties at root for Testing
+x-kubernetes-preserve-unknown-fields: true
+type: object
+properties:
+  spec:
+    description: Specification of Waldo
+    type: object
+    properties:
+      dummy:
+        description: Dummy property.
+        type: object
+  status:
+    description: Status of Waldo
+    type: object
+    properties:
+      bars:
+        description: List of Bars and their statuses.
+        type: array
+        items:
+          type: object`)
+
+var schemaPreserveNested = []byte(`description: preserve-unknown-properties in nested field for Testing
+type: object
+properties:
+  spec:
+    description: Specification of Waldo
+    type: object
+    x-kubernetes-preserve-unknown-fields: true
+    properties:
+      dummy:
+        description: Dummy property.
+        type: object
+  status:
+    description: Status of Waldo
+    type: object
+    properties:
+      bars:
+        description: List of Bars and their statuses.
+        type: array
+        items:
+          type: object`)