Merge pull request #111866 from pacoxu/validate

added ratcheting validation for x-kubernetes-list-type
2025-07-19 18:02:01 +00:00 · 2022-09-15 15:33:21 -07:00 · 2022-09-15 15:33:21 -07:00 · dc0dfdd109
commit dc0dfdd109
parent 661899f1a1 6dd961bddc
2 changed files with 123 additions and 2 deletions
--- 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"
@ -91,18 +92,28 @@ 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 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
 	if celValidator, ok := a.customResourceStrategy.celValidators[v]; ok {
 		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...)
 		}
 	}
--- 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)
+		}
+	}
+}