From e643286c06dd68077efc1153cd12c0661c2efed6 Mon Sep 17 00:00:00 2001 From: Siva Date: Tue, 14 Jul 2020 16:16:45 -0500 Subject: [PATCH 1/4] Show error in status if preserve unknown fields is true for nonstructural schemas --- .../nonstructuralschema_controller.go | 6 ++ .../nonstructuralschema_controller_test.go | 66 +++++++++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller_test.go diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go index 9d4d9acb534..4041d9c4355 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go +++ b/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 diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller_test.go new file mode 100644 index 00000000000..4b79f554ef5 --- /dev/null +++ b/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) + } + }) + } +} From 4c995d8fc8b3769c1f6c1e77a1d5a738ec13fde4 Mon Sep 17 00:00:00 2001 From: Siva Date: Wed, 2 Sep 2020 14:58:54 -0500 Subject: [PATCH 2/4] fix error message --- .../test/integration/validation_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index 3b33469f1ab..a25f9f3b813 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go @@ -814,8 +814,8 @@ spec: t.Fatalf("unexpected error waiting for NonStructuralSchema condition: %v", cond) } - // readd schema - t.Log("Readd schema") + // re-add 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 { From d141efc3b0f5882b8b157319fc7540b9dc76529c Mon Sep 17 00:00:00 2001 From: Siva Date: Wed, 2 Sep 2020 14:59:07 -0500 Subject: [PATCH 3/4] fix integration tests --- .../test/integration/validation_test.go | 63 +++++++++++++------ 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go index a25f9f3b813..38b41ffb6db 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/validation_test.go +++ b/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{}) - if err != nil { - t.Fatalf("unexpected get error: %v", err) - } - crd.Spec.Validation = nil - if _, err = apiExtensionClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); apierrors.IsConflict(err) { + // This patch fixes two fields to resolve + // 1. property type validation error + // 2. preserveUnknownFields validation error + patch := []byte("[{\"op\":\"add\",\"path\":\"/spec/validation/openAPIV3Schema/properties/a/type\",\"value\":\"int\"}," + + "{\"op\":\"replace\",\"path\":\"/spec/preserveUnknownFields\",\"value\":false}]") + 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) @@ -821,6 +826,7 @@ spec: 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 From 68c23dff4ec7aa15ebc9c9f6b3a418d584279006 Mon Sep 17 00:00:00 2001 From: Siva Date: Wed, 2 Sep 2020 15:21:36 -0500 Subject: [PATCH 4/4] make update --- .../pkg/controller/nonstructuralschema/BUILD | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/BUILD index 147a964f2c8..9c0afced1c6 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/BUILD +++ b/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", + ], +)