From 400f52d4919d7fb6e7f54e24b20199fc2a9214dc Mon Sep 17 00:00:00 2001 From: Nikhil Sharma Date: Tue, 21 Jun 2022 11:36:14 +0530 Subject: [PATCH 1/2] added ratcheting validation for embedded resource and x-kubernetes-list-type validation Signed-off-by: Paco Xu --- .../pkg/registry/customresource/status_strategy.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go index 074c4b73c96..042d972122c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go @@ -22,6 +22,7 @@ import ( "sigs.k8s.io/structured-merge-diff/v4/fieldpath" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel" + structurallisttype "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" @@ -97,6 +98,11 @@ func (a statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj v := obj.GetObjectKind().GroupVersionKind().Version + // ratcheting validation of x-kubernetes-list-type value map and set + if oldErrs := structurallisttype.ValidateListSetsAndMaps(nil, a.structuralSchemas[v], uOld.Object); len(oldErrs) == 0 { + errs = append(errs, structurallisttype.ValidateListSetsAndMaps(nil, a.structuralSchemas[v], uNew.Object)...) + } + // validate x-kubernetes-validations rules if celValidator, ok := a.customResourceStrategy.celValidators[v]; ok { if has, err := hasBlockingErr(errs); has { From 6dd961bddc8ad749be390d9e84814104a31a6990 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Mon, 22 Aug 2022 10:41:09 +0800 Subject: [PATCH 2/2] add test case for array checking with dup values Signed-off-by: Paco Xu --- .../customresource/status_strategy.go | 13 ++- .../customresource/status_strategy_test.go | 110 ++++++++++++++++++ 2 files changed, 119 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go index 042d972122c..efd9b2dff55 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy.go @@ -92,15 +92,20 @@ func (a statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj return errs } uOld, ok := old.(*unstructured.Unstructured) + var oldObject map[string]interface{} if !ok { - uOld = nil // as a safety precaution, continue with validation if uOld self cannot be cast + oldObject = nil + } else { + oldObject = uOld.Object } v := obj.GetObjectKind().GroupVersionKind().Version // ratcheting validation of x-kubernetes-list-type value map and set - if oldErrs := structurallisttype.ValidateListSetsAndMaps(nil, a.structuralSchemas[v], uOld.Object); len(oldErrs) == 0 { - errs = append(errs, structurallisttype.ValidateListSetsAndMaps(nil, a.structuralSchemas[v], uNew.Object)...) + if newErrs := structurallisttype.ValidateListSetsAndMaps(nil, a.structuralSchemas[v], uNew.Object); len(newErrs) > 0 { + if oldErrs := structurallisttype.ValidateListSetsAndMaps(nil, a.structuralSchemas[v], oldObject); len(oldErrs) == 0 { + errs = append(errs, newErrs...) + } } // validate x-kubernetes-validations rules @@ -108,7 +113,7 @@ func (a statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj if has, err := hasBlockingErr(errs); has { errs = append(errs, err) } else { - err, _ := celValidator.Validate(ctx, nil, a.customResourceStrategy.structuralSchemas[v], uNew.Object, uOld.Object, cel.RuntimeCELCostBudget) + err, _ := celValidator.Validate(ctx, nil, a.customResourceStrategy.structuralSchemas[v], uNew.Object, oldObject, cel.RuntimeCELCostBudget) errs = append(errs, err...) } } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy_test.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy_test.go index 8a651d12bb0..a91e8f0587d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/status_strategy_test.go @@ -21,7 +21,12 @@ import ( "reflect" "testing" + "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/yaml" ) func TestPrepareForUpdate(t *testing.T) { @@ -136,3 +141,108 @@ func TestPrepareForUpdate(t *testing.T) { } } } + +const listTypeResourceSchema = ` +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: foos.test +spec: + group: test + names: + kind: Foo + listKind: FooList + plural: foos + singular: foo + scope: Cluster + versions: + - name: v1 + schema: + openAPIV3Schema: + type: object + properties: + numArray: + type: array + x-kubernetes-list-type: set + items: + type: object + served: true + storage: true + - name: v2 + schema: + openAPIV3Schema: + type: object + properties: + numArray2: + type: array +` + +func TestStatusStrategyValidateUpdate(t *testing.T) { + crdV1 := &apiextensionsv1.CustomResourceDefinition{} + err := yaml.Unmarshal([]byte(listTypeResourceSchema), &crdV1) + if err != nil { + t.Fatalf("unexpected decoding error: %v", err) + } + t.Logf("crd v1 details: %v", crdV1) + crd := &apiextensions.CustomResourceDefinition{} + if err = apiextensionsv1.Convert_v1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(crdV1, crd, nil); err != nil { + t.Fatalf("unexpected convert error: %v", err) + } + t.Logf("crd details: %v", crd) + + strategy := statusStrategy{} + kind := schema.GroupVersionKind{ + Version: crd.Spec.Versions[0].Name, + Kind: crd.Spec.Names.Kind, + Group: crd.Spec.Group, + } + strategy.customResourceStrategy.validator.kind = kind + ss, _ := structuralschema.NewStructural(crd.Spec.Versions[0].Schema.OpenAPIV3Schema) + strategy.structuralSchemas = map[string]*structuralschema.Structural{ + crd.Spec.Versions[0].Name: ss, + } + + ctx := context.TODO() + + tcs := []struct { + name string + old *unstructured.Unstructured + obj *unstructured.Unstructured + isValid bool + }{ + { + name: "bothValid", + old: &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "test/v1", "kind": "Foo", "numArray": []interface{}{1, 2}}}, + obj: &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "test/v1", "kind": "Foo", "numArray": []interface{}{1, 3}, "metadata": map[string]interface{}{"resourceVersion": "1"}}}, + isValid: true, + }, + { + name: "change to invalid", + old: &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "test/v1", "kind": "Foo", "spec": "old", "numArray": []interface{}{1, 2}}}, + obj: &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "test/v1", "kind": "Foo", "spec": "new", "numArray": []interface{}{1, 1}, "metadata": map[string]interface{}{"resourceVersion": "1"}}}, + isValid: false, + }, + { + name: "change to valid", + old: &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "test/v1", "kind": "Foo", "spec": "new", "numArray": []interface{}{1, 1}}}, + obj: &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "test/v1", "kind": "Foo", "spec": "old", "numArray": []interface{}{1, 2}, "metadata": map[string]interface{}{"resourceVersion": "1"}}}, + isValid: true, + }, + { + name: "keeps invalid", + old: &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "test/v1", "kind": "Foo", "spec": "new", "numArray": []interface{}{1, 1}}}, + obj: &unstructured.Unstructured{Object: map[string]interface{}{"apiVersion": "test/v1", "kind": "Foo", "spec": "old", "numArray": []interface{}{1, 1}, "metadata": map[string]interface{}{"resourceVersion": "1"}}}, + isValid: true, + }, + } + + for _, tc := range tcs { + errs := strategy.ValidateUpdate(ctx, tc.obj, tc.old) + if tc.isValid && len(errs) > 0 { + t.Errorf("%v: unexpected error: %v", tc.name, errs) + } + if !tc.isValid && len(errs) == 0 { + t.Errorf("%v: unexpected non-error", tc.name) + } + } +}