From a5a3b52ccd0a102977c345c2d8e6f187079848d8 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Thu, 8 Oct 2020 15:33:24 -0700 Subject: [PATCH] CRs: Default non-nullable nulls --- .../pkg/apiserver/customresource_handler.go | 4 +- .../pkg/apiserver/schema/defaulting/BUILD | 6 +- .../apiserver/schema/defaulting/algorithm.go | 26 ++++-- .../schema/defaulting/algorithm_test.go | 78 +++++++++++++++- .../apiserver/schema/defaulting/prunenulls.go | 66 +++++++++++++ .../schema/defaulting/prunenulls_test.go | 93 +++++++++++++++++++ 6 files changed, 264 insertions(+), 9 deletions(-) create mode 100644 staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls.go create mode 100644 staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls_test.go diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index d3c7457bf35..92b1381bff2 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -1216,7 +1216,8 @@ func (v schemaCoercingConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, // in addition for native types when decoding into Golang structs: // // - validating and pruning ObjectMeta -// - generic pruning of unknown fields following a structural schema. +// - generic pruning of unknown fields following a structural schema +// - removal of non-defaulted non-nullable null map values. type unstructuredSchemaCoercer struct { dropInvalidMetadata bool repairGeneration bool @@ -1250,6 +1251,7 @@ func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) error { if !v.preserveUnknownFields { // TODO: switch over pruning and coercing at the root to schemaobjectmeta.Coerce too structuralpruning.Prune(u.Object, v.structuralSchemas[gv.Version], false) + structuraldefaulting.PruneNonNullableNullsWithoutDefaults(u.Object, v.structuralSchemas[gv.Version]) } if err := schemaobjectmeta.Coerce(nil, u.Object, v.structuralSchemas[gv.Version], false, v.dropInvalidMetadata); err != nil { return err diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/BUILD index b4ac61b9dd3..c5db81c3785 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/BUILD @@ -5,6 +5,7 @@ go_library( srcs = [ "algorithm.go", "prune.go", + "prunenulls.go", "surroundingobject.go", "validation.go", ], @@ -26,7 +27,10 @@ go_library( go_test( name = "go_default_test", - srcs = ["algorithm_test.go"], + srcs = [ + "algorithm_test.go", + "prunenulls_test.go", + ], embed = [":go_default_library"], deps = [ "//staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema:go_default_library", diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go index 2c699898e11..8d6f55fb033 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm.go @@ -21,8 +21,16 @@ import ( "k8s.io/apimachinery/pkg/runtime" ) +// isNonNullalbeNull returns true if the item is nil AND it's nullable +func isNonNullableNull(x interface{}, s *structuralschema.Structural) bool { + return x == nil && s != nil && s.Generic.Nullable == false +} + // Default does defaulting of x depending on default values in s. // Default values from s are deep-copied. +// +// PruneNonNullableNullsWithoutDefaults has left the non-nullable nulls +// that have a default here. func Default(x interface{}, s *structuralschema.Structural) { if s == nil { return @@ -34,20 +42,26 @@ func Default(x interface{}, s *structuralschema.Structural) { if prop.Default.Object == nil { continue } - if _, found := x[k]; !found { + if _, found := x[k]; !found || isNonNullableNull(x[k], &prop) { x[k] = runtime.DeepCopyJSONValue(prop.Default.Object) } } - for k, v := range x { + for k := range x { if prop, found := s.Properties[k]; found { - Default(v, &prop) + Default(x[k], &prop) } else if s.AdditionalProperties != nil { - Default(v, s.AdditionalProperties.Structural) + if isNonNullableNull(x[k], s.AdditionalProperties.Structural) { + x[k] = runtime.DeepCopyJSONValue(s.AdditionalProperties.Structural.Default.Object) + } + Default(x[k], s.AdditionalProperties.Structural) } } case []interface{}: - for _, v := range x { - Default(v, s.Items) + for i := range x { + if isNonNullableNull(x[i], s.Items) { + x[i] = runtime.DeepCopyJSONValue(s.Items.Default.Object) + } + Default(x[i], s.Items) } default: // scalars, do nothing diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm_test.go index ceef3c91399..f6ecfc2931d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/algorithm_test.go @@ -135,7 +135,83 @@ func TestDefault(t *testing.T) { }, }, }, - }, `[{"a":"A"},{"a":1},{"a":0},{"a":0.0},{"a":""},{"a":null},{"a":[]},{"a":{}}]`}, + }, `[{"a":"A"},{"a":1},{"a":0},{"a":0.0},{"a":""},{"a":"A"},{"a":[]},{"a":{}}]`}, + {"null in nullable list", `[null]`, &structuralschema.Structural{ + Generic: structuralschema.Generic{ + Nullable: true, + }, + Items: &structuralschema.Structural{ + Properties: map[string]structuralschema.Structural{ + "a": { + Generic: structuralschema.Generic{ + Default: structuralschema.JSON{"A"}, + }, + }, + }, + }, + }, `[null]`}, + {"null in non-nullable list", `[null]`, &structuralschema.Structural{ + Generic: structuralschema.Generic{ + Nullable: false, + }, + Items: &structuralschema.Structural{ + Generic: structuralschema.Generic{ + Default: structuralschema.JSON{"A"}, + }, + }, + }, `["A"]`}, + {"null in nullable object", `{"a": null}`, &structuralschema.Structural{ + Generic: structuralschema.Generic{}, + Properties: map[string]structuralschema.Structural{ + "a": { + Generic: structuralschema.Generic{ + Nullable: true, + Default: structuralschema.JSON{"A"}, + }, + }, + }, + }, `{"a": null}`}, + {"null in non-nullable object", `{"a": null}`, &structuralschema.Structural{ + Properties: map[string]structuralschema.Structural{ + "a": { + Generic: structuralschema.Generic{ + Nullable: false, + Default: structuralschema.JSON{"A"}, + }, + }, + }, + }, `{"a": "A"}`}, + {"null in nullable object with additionalProperties", `{"a": null}`, &structuralschema.Structural{ + Generic: structuralschema.Generic{ + AdditionalProperties: &structuralschema.StructuralOrBool{ + Structural: &structuralschema.Structural{ + Generic: structuralschema.Generic{ + Nullable: true, + Default: structuralschema.JSON{"A"}, + }, + }, + }, + }, + }, `{"a": null}`}, + {"null in non-nullable object with additionalProperties", `{"a": null}`, &structuralschema.Structural{ + Generic: structuralschema.Generic{ + AdditionalProperties: &structuralschema.StructuralOrBool{ + Structural: &structuralschema.Structural{ + Generic: structuralschema.Generic{ + Nullable: false, + Default: structuralschema.JSON{"A"}, + }, + }, + }, + }, + }, `{"a": "A"}`}, + {"null unknown field", `{"a": null}`, &structuralschema.Structural{ + Generic: structuralschema.Generic{ + AdditionalProperties: &structuralschema.StructuralOrBool{ + Bool: true, + }, + }, + }, `{"a": null}`}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls.go new file mode 100644 index 00000000000..bc8221ac7d7 --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls.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 defaulting + +import structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" + +func isNonNullableNonDefaultableNull(x interface{}, s *structuralschema.Structural) bool { + return x == nil && s != nil && s.Generic.Nullable == false && s.Default.Object == nil +} + +func getSchemaForField(field string, s *structuralschema.Structural) *structuralschema.Structural { + if s == nil { + return nil + } + schema, ok := s.Properties[field] + if ok { + return &schema + } + if s.AdditionalProperties != nil { + return s.AdditionalProperties.Structural + } + return nil +} + +// PruneNonNullableNullsWithoutDefaults removes non-nullable +// non-defaultable null values from object. +// +// Non-nullable nulls that have a default are left alone here and will +// be defaulted later. +func PruneNonNullableNullsWithoutDefaults(x interface{}, s *structuralschema.Structural) { + switch x := x.(type) { + case map[string]interface{}: + for k, v := range x { + schema := getSchemaForField(k, s) + if isNonNullableNonDefaultableNull(v, schema) { + delete(x, k) + } else { + PruneNonNullableNullsWithoutDefaults(v, schema) + } + } + case []interface{}: + var schema *structuralschema.Structural + if s != nil { + schema = s.Items + } + for i := range x { + PruneNonNullableNullsWithoutDefaults(x[i], schema) + } + default: + // scalars, do nothing + } +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls_test.go new file mode 100644 index 00000000000..c2e620dbe38 --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/defaulting/prunenulls_test.go @@ -0,0 +1,93 @@ +/* +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 defaulting + +import ( + "bytes" + "reflect" + "testing" + + structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" + "k8s.io/apimachinery/pkg/util/json" +) + +func TestPruneNonNullableNullsWithoutDefaults(t *testing.T) { + tests := []struct { + name string + json string + schema *structuralschema.Structural + expected string + }{ + {"empty", "null", nil, "null"}, + {"scalar", "4", &structuralschema.Structural{ + Generic: structuralschema.Generic{ + Default: structuralschema.JSON{"foo"}, + }, + }, "4"}, + {"scalar array", "[1,null]", nil, "[1,null]"}, + {"object array", `[{"a":null},{"b":null},{"c":null},{"d":null},{"e":null}]`, &structuralschema.Structural{ + Items: &structuralschema.Structural{ + Properties: map[string]structuralschema.Structural{ + "a": { + Generic: structuralschema.Generic{ + Default: structuralschema.JSON{"A"}, + }, + }, + "b": { + Generic: structuralschema.Generic{ + Nullable: true, + }, + }, + "c": { + Generic: structuralschema.Generic{ + Default: structuralschema.JSON{"C"}, + Nullable: true, + }, + }, + "d": { + Generic: structuralschema.Generic{}, + }, + }, + }, + }, `[{"a":null},{"b":null},{"c":null},{},{"e":null}]`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var in interface{} + if err := json.Unmarshal([]byte(tt.json), &in); err != nil { + t.Fatal(err) + } + + var expected interface{} + if err := json.Unmarshal([]byte(tt.expected), &expected); err != nil { + t.Fatal(err) + } + + PruneNonNullableNullsWithoutDefaults(in, tt.schema) + if !reflect.DeepEqual(in, expected) { + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + enc.SetIndent("", " ") + err := enc.Encode(in) + if err != nil { + t.Fatalf("unexpected result mashalling error: %v", err) + } + t.Errorf("expected: %s\ngot: %s", tt.expected, buf.String()) + } + }) + } +}