Merge pull request #93078 from vareti/show-error-in-status-if-preserve-unknown-field-is-true-for-nonstructural-schemas

Show error in status if preserve unknown fields is true for nonstructural schemas

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/BUILD

@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -38,3 +38,13 @@ filegroup(
     tags = ["automanaged"],
     visibility = ["//visibility:public"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["nonstructuralschema_controller_test.go"],
+    embed = [":go_default_library"],
+    deps = [
+        "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
+    ],
+)

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go

@@ -90,6 +90,12 @@ func calculateCondition(in *apiextensionsv1.CustomResourceDefinition) *apiextens
 
 	allErrs := field.ErrorList{}
 
+	if in.Spec.PreserveUnknownFields {
+		allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "preserveUnknownFields"),
+			in.Spec.PreserveUnknownFields,
+			fmt.Sprint("must be false")))
+	}
+
 	for i, v := range in.Spec.Versions {
 		if v.Schema == nil || v.Schema.OpenAPIV3Schema == nil {
 			continue

staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller_test.go

@@ -0,0 +1,66 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package nonstructuralschema
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+func Test_calculateCondition(t *testing.T) {
+	tests := []struct {
+		name string
+		args *apiextensionsv1.CustomResourceDefinition
+		want *apiextensionsv1.CustomResourceDefinitionCondition
+	}{
+		{
+			name: "preserve unknown fields is false",
+			args: &apiextensionsv1.CustomResourceDefinition{
+				Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+					PreserveUnknownFields: false,
+				},
+			},
+		},
+		{
+			name: "preserve unknown fields is true",
+			args: &apiextensionsv1.CustomResourceDefinition{
+				Spec: apiextensionsv1.CustomResourceDefinitionSpec{
+					PreserveUnknownFields: true,
+				},
+			},
+			want: &apiextensionsv1.CustomResourceDefinitionCondition{
+				Type:   apiextensionsv1.NonStructuralSchema,
+				Status: apiextensionsv1.ConditionTrue,
+				Reason: "Violations",
+				Message: field.Invalid(field.NewPath("spec", "preserveUnknownFields"),
+					true,
+					fmt.Sprint("must be false")).Error(),
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := calculateCondition(tt.args); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("calculateCondition() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go

@@ -27,6 +27,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apimachinery/pkg/util/yaml"
 
@@ -780,21 +781,25 @@ spec:
 	if v := "spec.versions[0].schema.openAPIV3Schema.properties[a].type: Required value: must not be empty for specified object fields"; !strings.Contains(cond.Message, v) {
 		t.Fatalf("expected violation %q, but got: %v", v, cond.Message)
 	}
+	if v := "spec.preserveUnknownFields: Invalid value: true: must be false"; !strings.Contains(cond.Message, v) {
+		t.Fatalf("expected violation %q, but got: %v", v, cond.Message)
+	}
 
 	// remove schema
 	t.Log("Remove schema")
 	for retry := 0; retry < 5; retry++ {
-		crd, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Get(context.TODO(), name, metav1.GetOptions{})
+		// This patch fixes two fields to resolve
-		if err != nil {
+		// 1. property type validation error
-			t.Fatalf("unexpected get error: %v", err)
+		// 2. preserveUnknownFields validation error
-		}
+		patch := []byte("[{\"op\":\"add\",\"path\":\"/spec/validation/openAPIV3Schema/properties/a/type\",\"value\":\"int\"}," +
-		crd.Spec.Validation = nil
+			"{\"op\":\"replace\",\"path\":\"/spec/preserveUnknownFields\",\"value\":false}]")
-		if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); apierrors.IsConflict(err) {
+		if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Patch(context.TODO(), name, types.JSONPatchType, patch, metav1.PatchOptions{}); apierrors.IsConflict(err) {
 			continue
 		}
 		if err != nil {
 			t.Fatalf("unexpected update error: %v", err)
 		}
+		break
 	}
 	if err != nil {
 		t.Fatalf("unexpected update error: %v", err)
@@ -814,13 +819,14 @@ spec:
 		t.Fatalf("unexpected error waiting for NonStructuralSchema condition: %v", cond)
 	}
 
-	// readd schema
+	// re-add schema
-	t.Log("Readd schema")
+	t.Log("Re-add schema")
 	for retry := 0; retry < 5; retry++ {
 		crd, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Get(context.TODO(), name, metav1.GetOptions{})
 		if err != nil {
 			t.Fatalf("unexpected get error: %v", err)
 		}
+		crd.Spec.PreserveUnknownFields = nil
 		crd.Spec.Validation = &apiextensionsv1beta1.CustomResourceValidation{OpenAPIV3Schema: origSchema}
 		if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); apierrors.IsConflict(err) {
 			continue
@@ -828,6 +834,7 @@ spec:
 		if err != nil {
 			t.Fatalf("unexpected update error: %v", err)
 		}
+		break
 	}
 	if err != nil {
 		t.Fatalf("unexpected update error: %v", err)
@@ -849,6 +856,9 @@ spec:
 	if v := "spec.versions[0].schema.openAPIV3Schema.properties[a].type: Required value: must not be empty for specified object fields"; !strings.Contains(cond.Message, v) {
 		t.Fatalf("expected violation %q, but got: %v", v, cond.Message)
 	}
+	if v := "spec.preserveUnknownFields: Invalid value: true: must be false"; !strings.Contains(cond.Message, v) {
+		t.Fatalf("expected violation %q, but got: %v", v, cond.Message)
+	}
 }
 
 func TestNonStructuralSchemaCondition(t *testing.T) {
@@ -862,6 +872,7 @@ func TestNonStructuralSchemaCondition(t *testing.T) {
 apiVersion: apiextensions.k8s.io/v1beta1
 kind: CustomResourceDefinition
 spec:
+  preserveUnknownFields: PRESERVE_UNKNOWN_FIELDS
   version: v1beta1
   names:
     plural: foos
@@ -882,6 +893,7 @@ spec:
 
 	type Test struct {
 		desc                                  string
+		preserveUnknownFields                 string
 		globalSchema, v1Schema, v1beta1Schema string
 		expectedCreateErrors                  []string
 		unexpectedCreateErrors                []string
@@ -889,7 +901,19 @@ spec:
 		unexpectedViolations                  []string
 	}
 	tests := []Test{
-		{"empty", "", "", "", nil, nil, nil, nil},
+		{
+			desc: "empty",
+			expectedViolations: []string{
+				"spec.preserveUnknownFields: Invalid value: true: must be false",
+			},
+		},
+		{
+			desc:                  "preserve unknown fields is false",
+			preserveUnknownFields: "false",
+			globalSchema: `
+type: object
+`,
+		},
 		{
 			desc: "int-or-string and preserve-unknown-fields true",
 			globalSchema: `
@@ -922,7 +946,8 @@ x-kubernetes-embedded-resource: true
 			},
 		},
 		{
-			desc: "embedded-resource without preserve-unknown-fields, but properties",
+			desc:                  "embedded-resource without preserve-unknown-fields, but properties",
+			preserveUnknownFields: "false",
 			globalSchema: `
 type: object
 x-kubernetes-embedded-resource: true
@@ -934,16 +959,15 @@ properties:
  metadata:
    type: object
 `,
-			expectedViolations: []string{},
 		},
 		{
-			desc: "embedded-resource with preserve-unknown-fields",
+			desc:                  "embedded-resource with preserve-unknown-fields",
+			preserveUnknownFields: "false",
 			globalSchema: `
 type: object
 x-kubernetes-embedded-resource: true
 x-kubernetes-preserve-unknown-fields: true
 `,
-			expectedViolations: []string{},
 		},
 		{
 			desc: "embedded-resource with wrong type",
@@ -1330,7 +1354,8 @@ oneOf:
 			},
 		},
 		{
-			desc: "structural complete",
+			desc:                  "structural complete",
+			preserveUnknownFields: "false",
 			globalSchema: `
 type: object
 properties:
@@ -1391,7 +1416,6 @@ oneOf:
 - properties:
    g: {}
 `,
-			expectedViolations: nil,
 		},
 		{
 			desc: "invalid v1beta1 schema",
@@ -1472,7 +1496,8 @@ properties:
 			},
 		},
 		{
-			desc: "metadata with name property",
+			desc:                  "metadata with name property",
+			preserveUnknownFields: "false",
 			globalSchema: `
 type: object
 properties:
@@ -1483,10 +1508,10 @@ properties:
        type: string
        pattern: "^[a-z]+$"
 `,
-			expectedViolations: []string{},
 		},
 		{
-			desc: "metadata with generateName property",
+			desc:                  "metadata with generateName property",
+			preserveUnknownFields: "false",
 			globalSchema: `
 type: object
 properties:
@@ -1497,10 +1522,10 @@ properties:
        type: string
        pattern: "^[a-z]+$"
 `,
-			expectedViolations: []string{},
 		},
 		{
-			desc: "metadata with name and generateName property",
+			desc:                  "metadata with name and generateName property",
+			preserveUnknownFields: "false",
 			globalSchema: `
 type: object
 properties:
@@ -1514,7 +1539,6 @@ properties:
        type: string
        pattern: "^[a-z]+$"
 `,
-			expectedViolations: []string{},
 		},
 		{
 			desc: "metadata under junctors",
@@ -1597,6 +1621,7 @@ properties:
 				"GLOBAL_SCHEMA", toValidationJSON(tst.globalSchema),
 				"V1BETA1_SCHEMA", toValidationJSON(tst.v1beta1Schema),
 				"V1_SCHEMA", toValidationJSON(tst.v1Schema),
+				"PRESERVE_UNKNOWN_FIELDS", tst.preserveUnknownFields,
 			).Replace(tmpl)
 
 			// decode CRD manifest