From 091fa293908f8e9c132da315449f0c24014db478 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 7 Jan 2025 13:23:06 +0000 Subject: [PATCH 1/4] Fix status subresource ratcheting --- .../customresource/status_strategy.go | 24 ++++- .../pkg/registry/customresource/validator.go | 4 +- .../test/integration/ratcheting_test.go | 95 ++++++++++++++++++- 3 files changed, 115 insertions(+), 8 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 ceaf56629b0..24cdd3fe3ed 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,11 +22,17 @@ import ( "sigs.k8s.io/structured-merge-diff/v4/fieldpath" + "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel" + "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/model" structurallisttype "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype" + "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" + apiextensionsfeatures "k8s.io/apiextensions-apiserver/pkg/features" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" celconfig "k8s.io/apiserver/pkg/apis/cel" + "k8s.io/apiserver/pkg/cel/common" + utilfeature "k8s.io/apiserver/pkg/util/feature" ) type statusStrategy struct { @@ -94,8 +100,17 @@ func (a statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj return field.ErrorList{field.Invalid(field.NewPath(""), old, fmt.Sprintf("has type %T. Must be a pointer to an Unstructured type", old))} } + var options []validation.ValidationOption + var celOptions []cel.Option + var correlatedObject *common.CorrelatedObject + if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) { + correlatedObject = common.NewCorrelatedObject(uNew.Object, uOld.Object, &model.Structural{Structural: a.structuralSchema}) + options = append(options, validation.WithRatcheting(correlatedObject)) + celOptions = append(celOptions, cel.WithRatcheting(correlatedObject)) + } + var errs field.ErrorList - errs = append(errs, a.customResourceStrategy.validator.ValidateStatusUpdate(ctx, uNew, uOld, a.scale)...) + errs = append(errs, a.customResourceStrategy.validator.ValidateStatusUpdate(ctx, uNew, uOld, a.scale, options...)...) // ratcheting validation of x-kubernetes-list-type value map and set if newErrs := structurallisttype.ValidateListSetsAndMaps(nil, a.structuralSchema, uNew.Object); len(newErrs) > 0 { @@ -109,10 +124,15 @@ 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.structuralSchema, uNew.Object, uOld.Object, celconfig.RuntimeCELCostBudget) + err, _ := celValidator.Validate(ctx, nil, a.customResourceStrategy.structuralSchema, uNew.Object, uOld.Object, celconfig.RuntimeCELCostBudget, celOptions...) errs = append(errs, err...) } } + + // No-op if not attached to context + if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) { + validation.Metrics.ObserveRatchetingTime(*correlatedObject.Duration) + } return errs } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go index 7e2f6f4d551..eabb3fd572d 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/validator.go @@ -96,7 +96,7 @@ func validateKubeFinalizerName(stringValue string, fldPath *field.Path) []string return allWarnings } -func (a customResourceValidator) ValidateStatusUpdate(ctx context.Context, obj, old *unstructured.Unstructured, scale *apiextensions.CustomResourceSubresourceScale) field.ErrorList { +func (a customResourceValidator) ValidateStatusUpdate(ctx context.Context, obj, old *unstructured.Unstructured, scale *apiextensions.CustomResourceSubresourceScale, options ...apiextensionsvalidation.ValidationOption) field.ErrorList { if errs := a.ValidateTypeMeta(ctx, obj); len(errs) > 0 { return errs } @@ -105,7 +105,7 @@ func (a customResourceValidator) ValidateStatusUpdate(ctx context.Context, obj, allErrs = append(allErrs, validation.ValidateObjectMetaAccessorUpdate(obj, old, field.NewPath("metadata"))...) if status, hasStatus := obj.UnstructuredContent()["status"]; hasStatus { - allErrs = append(allErrs, apiextensionsvalidation.ValidateCustomResourceUpdate(field.NewPath("status"), status, old.UnstructuredContent()["status"], a.statusSchemaValidator)...) + allErrs = append(allErrs, apiextensionsvalidation.ValidateCustomResourceUpdate(field.NewPath("status"), status, old.UnstructuredContent()["status"], a.statusSchemaValidator, options...)...) } allErrs = append(allErrs, a.ValidateScaleStatus(ctx, obj, scale)...) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go index e39f0d7bb40..b00b97b29e3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go @@ -45,6 +45,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" utilyaml "k8s.io/apimachinery/pkg/util/yaml" @@ -81,6 +82,7 @@ type ratchetingTestContext struct { *testing.T DynamicClient dynamic.Interface APIExtensionsClient clientset.Interface + SkipStatus bool } type ratchetingTestOperation interface { @@ -95,6 +97,12 @@ type expectError struct { func (e expectError) Do(ctx *ratchetingTestContext) error { err := e.op.Do(ctx) if err != nil { + // If there is an error, it should happen when updating the CR + // and also when updating the CR status subresource. + if agg, ok := err.(utilerrors.Aggregate); ok && len(agg.Errors()) != 2 { + return fmt.Errorf("expected 2 errors, got %v", len(agg.Errors())) + } + return nil } return errors.New("expected error") @@ -190,8 +198,51 @@ func (a applyPatchOperation) Do(ctx *ratchetingTestContext) error { FieldManager: "manager", }) - return err + if ctx.SkipStatus { + return err + } + errs := []error{} + + if err != nil { + errs = append(errs, err) + } + + statusPatch := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "status": patch.Object, + }, + } + + statusPatch.SetKind(kind) + statusPatch.SetAPIVersion(a.gvr.GroupVersion().String()) + statusPatch.SetName(a.name) + statusPatch.SetNamespace("default") + + delete(patch.Object, "metadata") + delete(patch.Object, "apiVersion") + delete(patch.Object, "kind") + + _, err = ctx.DynamicClient. + Resource(a.gvr). + Namespace(statusPatch.GetNamespace()). + ApplyStatus( + context.TODO(), + statusPatch.GetName(), + statusPatch, + metav1.ApplyOptions{ + FieldManager: "manager", + }) + + if err != nil { + errs = append(errs, err) + } + + if len(errs) > 0 { + return utilerrors.NewAggregate(errs) + } + + return nil } func (a applyPatchOperation) Description() string { @@ -228,10 +279,21 @@ func (u updateMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { }}, } + if !ctx.SkipStatus { + // Duplicate the schema as a status schema + statusSchema := sch.DeepCopy() + sch.Properties["status"] = *statusSchema + } + for _, v := range myCRD.Spec.Versions { if v.Name != myCRDV1Beta1.Version { continue } + + *v.Subresources = apiextensionsv1.CustomResourceSubresources{ + Status: &apiextensionsv1.CustomResourceSubresourceStatus{}, + } + v.Schema.OpenAPIV3Schema = sch } @@ -288,6 +350,16 @@ func (p patchMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { return err } + statusPatch := map[string]interface{}{ + "properties": map[string]interface{}{ + "status": p.patch, + }, + } + statusPatchJSON, err := json.Marshal(statusPatch) + if err != nil { + return err + } + myCRD, err := ctx.APIExtensionsClient.ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), myCRDV1Beta1.Resource+"."+myCRDV1Beta1.Group, metav1.GetOptions{}) if err != nil { return err @@ -308,8 +380,13 @@ func (p patchMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { return err } + mergedStatus, err := jsonpatch.MergePatch(merged, statusPatchJSON) + if err != nil { + return err + } + var parsed apiextensionsv1.JSONSchemaProps - if err := json.Unmarshal(merged, &parsed); err != nil { + if err := json.Unmarshal(mergedStatus, &parsed); err != nil { return err } @@ -329,6 +406,7 @@ type ratchetingTestCase struct { Name string Disabled bool Operations []ratchetingTestOperation + SkipStatus bool } func runTests(t *testing.T, cases []ratchetingTestCase) { @@ -372,9 +450,15 @@ func runTests(t *testing.T, cases []ratchetingTestCase) { }, }, }, + "status": { + Type: "object", + }, }, }, }, + Subresources: &apiextensionsv1.CustomResourceSubresources{ + Status: &apiextensionsv1.CustomResourceSubresourceStatus{}, + }, }}, Names: apiextensionsv1.CustomResourceDefinitionNames{ Plural: resource, @@ -399,6 +483,7 @@ func runTests(t *testing.T, cases []ratchetingTestCase) { T: t, DynamicClient: dynamicClient, APIExtensionsClient: apiExtensionClient, + SkipStatus: c.SkipStatus, } for i, op := range c.Operations { @@ -1330,7 +1415,8 @@ func TestRatchetingFunctionality(t *testing.T) { }, }, { - Name: "CEL Optional OldSelf", + Name: "CEL Optional OldSelf", + SkipStatus: true, // oldSelf can never be null for a status update. Operations: []ratchetingTestOperation{ updateMyCRDV1Beta1Schema{&apiextensionsv1.JSONSchemaProps{ Type: "object", @@ -1387,7 +1473,8 @@ func TestRatchetingFunctionality(t *testing.T) { Name: "Not_should_not_ratchet", }, { - Name: "CEL_transition_rules_should_not_ratchet", + Name: "CEL_transition_rules_should_not_ratchet", + SkipStatus: true, // Adding a broken status transition rule prevents the object from being updated. Operations: []ratchetingTestOperation{ updateMyCRDV1Beta1Schema{&apiextensionsv1.JSONSchemaProps{ Type: "object", From ba816967a0cd2e8d9b99896b1f2908c48d4e920f Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 8 Jan 2025 15:38:51 +0000 Subject: [PATCH 2/4] Simplify status subresource ratcheting testing --- .../customresource/status_strategy.go | 2 +- .../test/integration/ratcheting_test.go | 178 ++++++++---------- 2 files changed, 79 insertions(+), 101 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 24cdd3fe3ed..ea5a412af59 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 @@ -105,7 +105,7 @@ func (a statusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Obj var correlatedObject *common.CorrelatedObject if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CRDValidationRatcheting) { correlatedObject = common.NewCorrelatedObject(uNew.Object, uOld.Object, &model.Structural{Structural: a.structuralSchema}) - options = append(options, validation.WithRatcheting(correlatedObject)) + options = append(options, validation.WithRatcheting(correlatedObject.Key("status"))) celOptions = append(celOptions, cel.WithRatcheting(correlatedObject)) } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go index b00b97b29e3..f0b12569e2f 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go @@ -45,7 +45,6 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" utilyaml "k8s.io/apimachinery/pkg/util/yaml" @@ -82,7 +81,7 @@ type ratchetingTestContext struct { *testing.T DynamicClient dynamic.Interface APIExtensionsClient clientset.Interface - SkipStatus bool + StatusSubresource bool } type ratchetingTestOperation interface { @@ -97,12 +96,6 @@ type expectError struct { func (e expectError) Do(ctx *ratchetingTestContext) error { err := e.op.Do(ctx) if err != nil { - // If there is an error, it should happen when updating the CR - // and also when updating the CR status subresource. - if agg, ok := err.(utilerrors.Aggregate); ok && len(agg.Errors()) != 2 { - return fmt.Errorf("expected 2 errors, got %v", len(agg.Errors())) - } - return nil } return errors.New("expected error") @@ -172,7 +165,12 @@ func (a applyPatchOperation) Do(ctx *ratchetingTestContext) error { patch := &unstructured.Unstructured{} if obj, ok := a.patch.(map[string]interface{}); ok { - patch.Object = obj + patch.Object = map[string]interface{}{} + + // Copy the map at the top level to avoid modifying the original. + for k, v := range obj { + patch.Object[k] = v + } } else if str, ok := a.patch.(string); ok { str = FixTabsOrDie(str) if err := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(str), len(str)).Decode(&patch.Object); err != nil { @@ -182,67 +180,31 @@ func (a applyPatchOperation) Do(ctx *ratchetingTestContext) error { return fmt.Errorf("invalid patch type: %T", a.patch) } + if ctx.StatusSubresource { + patch.Object = map[string]interface{}{"status": patch.Object} + } + patch.SetKind(kind) patch.SetAPIVersion(a.gvr.GroupVersion().String()) patch.SetName(a.name) patch.SetNamespace("default") - _, err := ctx.DynamicClient. - Resource(a.gvr). - Namespace(patch.GetNamespace()). - Apply( - context.TODO(), - patch.GetName(), - patch, - metav1.ApplyOptions{ - FieldManager: "manager", - }) + c := ctx.DynamicClient.Resource(a.gvr).Namespace(patch.GetNamespace()) + if ctx.StatusSubresource { + if _, err := c.Get(context.TODO(), patch.GetName(), metav1.GetOptions{}); apierrors.IsNotFound(err) { + // ApplyStatus will not automatically create an object, we must make sure it exists before we can + // apply the status to it. + _, err := c.Create(context.TODO(), patch, metav1.CreateOptions{}) + if err != nil { + return err + } + } - if ctx.SkipStatus { + _, err := c.ApplyStatus(context.TODO(), patch.GetName(), patch, metav1.ApplyOptions{FieldManager: "manager"}) return err } - - errs := []error{} - - if err != nil { - errs = append(errs, err) - } - - statusPatch := &unstructured.Unstructured{ - Object: map[string]interface{}{ - "status": patch.Object, - }, - } - - statusPatch.SetKind(kind) - statusPatch.SetAPIVersion(a.gvr.GroupVersion().String()) - statusPatch.SetName(a.name) - statusPatch.SetNamespace("default") - - delete(patch.Object, "metadata") - delete(patch.Object, "apiVersion") - delete(patch.Object, "kind") - - _, err = ctx.DynamicClient. - Resource(a.gvr). - Namespace(statusPatch.GetNamespace()). - ApplyStatus( - context.TODO(), - statusPatch.GetName(), - statusPatch, - metav1.ApplyOptions{ - FieldManager: "manager", - }) - - if err != nil { - errs = append(errs, err) - } - - if len(errs) > 0 { - return utilerrors.NewAggregate(errs) - } - - return nil + _, err := c.Apply(context.TODO(), patch.GetName(), patch, metav1.ApplyOptions{FieldManager: "manager"}) + return err } func (a applyPatchOperation) Description() string { @@ -270,6 +232,17 @@ func (u updateMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { sch.Properties = map[string]apiextensionsv1.JSONSchemaProps{} } + if ctx.StatusSubresource { + sch = &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "status": *sch, + }, + } + } + + // sentinel must be in the root level of the schema. + // Do not include this in the status schema. uuidString := string(uuid.NewUUID()) sentinelName := "__ratcheting_sentinel_field__" sch.Properties[sentinelName] = apiextensionsv1.JSONSchemaProps{ @@ -279,21 +252,11 @@ func (u updateMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { }}, } - if !ctx.SkipStatus { - // Duplicate the schema as a status schema - statusSchema := sch.DeepCopy() - sch.Properties["status"] = *statusSchema - } - for _, v := range myCRD.Spec.Versions { if v.Name != myCRDV1Beta1.Version { continue } - *v.Subresources = apiextensionsv1.CustomResourceSubresources{ - Status: &apiextensionsv1.CustomResourceSubresourceStatus{}, - } - v.Schema.OpenAPIV3Schema = sch } @@ -315,7 +278,12 @@ func (u updateMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { name: "sentinel-resource", patch: map[string]interface{}{ sentinelName: fmt.Sprintf("invalid-%d", counter), - }}.Do(ctx) + }}.Do(&ratchetingTestContext{ + T: ctx.T, + DynamicClient: ctx.DynamicClient, + APIExtensionsClient: ctx.APIExtensionsClient, + StatusSubresource: false, // Do not carry this over, sentinel check is in the root level. + }) if err == nil { return false, errors.New("expected error when creating sentinel resource") @@ -344,18 +312,17 @@ type patchMyCRDV1Beta1Schema struct { } func (p patchMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { - var err error - patchJSON, err := json.Marshal(p.patch) - if err != nil { - return err + patch := p.patch + if ctx.StatusSubresource { + patch = map[string]interface{}{ + "properties": map[string]interface{}{ + "status": patch, + }, + } } - statusPatch := map[string]interface{}{ - "properties": map[string]interface{}{ - "status": p.patch, - }, - } - statusPatchJSON, err := json.Marshal(statusPatch) + var err error + patchJSON, err := json.Marshal(patch) if err != nil { return err } @@ -380,19 +347,19 @@ func (p patchMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { return err } - mergedStatus, err := jsonpatch.MergePatch(merged, statusPatchJSON) - if err != nil { - return err - } - var parsed apiextensionsv1.JSONSchemaProps - if err := json.Unmarshal(mergedStatus, &parsed); err != nil { + if err := json.Unmarshal(merged, &parsed); err != nil { return err } return updateMyCRDV1Beta1Schema{ newSchema: &parsed, - }.Do(ctx) + }.Do(&ratchetingTestContext{ + T: ctx.T, + DynamicClient: ctx.DynamicClient, + APIExtensionsClient: ctx.APIExtensionsClient, + StatusSubresource: false, // We have already handled the status subresource. + }) } return fmt.Errorf("could not find version %v in CRD %v", myCRDV1Beta1.Version, myCRD.Name) @@ -478,14 +445,7 @@ func runTests(t *testing.T, cases []ratchetingTestCase) { continue } - t.Run(c.Name, func(t *testing.T) { - ctx := &ratchetingTestContext{ - T: t, - DynamicClient: dynamicClient, - APIExtensionsClient: apiExtensionClient, - SkipStatus: c.SkipStatus, - } - + run := func(t *testing.T, ctx *ratchetingTestContext) { for i, op := range c.Operations { t.Logf("Performing Operation: %v", op.Description()) if err := op.Do(ctx); err != nil { @@ -498,7 +458,26 @@ func runTests(t *testing.T, cases []ratchetingTestCase) { if err != nil { t.Fatal(err) } + } + + t.Run(c.Name, func(t *testing.T) { + run(t, &ratchetingTestContext{ + T: t, + DynamicClient: dynamicClient, + APIExtensionsClient: apiExtensionClient, + }) }) + + if !c.SkipStatus { + t.Run("Status: "+c.Name, func(t *testing.T) { + run(t, &ratchetingTestContext{ + T: t, + DynamicClient: dynamicClient, + APIExtensionsClient: apiExtensionClient, + StatusSubresource: true, + }) + }) + } } } @@ -1473,8 +1452,7 @@ func TestRatchetingFunctionality(t *testing.T) { Name: "Not_should_not_ratchet", }, { - Name: "CEL_transition_rules_should_not_ratchet", - SkipStatus: true, // Adding a broken status transition rule prevents the object from being updated. + Name: "CEL_transition_rules_should_not_ratchet", Operations: []ratchetingTestOperation{ updateMyCRDV1Beta1Schema{&apiextensionsv1.JSONSchemaProps{ Type: "object", From a2b12ba4061a29e04651fd343f9e81528ef336f2 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Mon, 13 Jan 2025 11:28:33 +0000 Subject: [PATCH 3/4] Simplify schema sentinel subresource logic --- .../test/integration/ratcheting_test.go | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go index f0b12569e2f..baf52e439df 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go @@ -232,17 +232,6 @@ func (u updateMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { sch.Properties = map[string]apiextensionsv1.JSONSchemaProps{} } - if ctx.StatusSubresource { - sch = &apiextensionsv1.JSONSchemaProps{ - Type: "object", - Properties: map[string]apiextensionsv1.JSONSchemaProps{ - "status": *sch, - }, - } - } - - // sentinel must be in the root level of the schema. - // Do not include this in the status schema. uuidString := string(uuid.NewUUID()) sentinelName := "__ratcheting_sentinel_field__" sch.Properties[sentinelName] = apiextensionsv1.JSONSchemaProps{ @@ -252,6 +241,15 @@ func (u updateMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { }}, } + if ctx.StatusSubresource { + sch = &apiextensionsv1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "status": *sch, + }, + } + } + for _, v := range myCRD.Spec.Versions { if v.Name != myCRDV1Beta1.Version { continue @@ -278,12 +276,7 @@ func (u updateMyCRDV1Beta1Schema) Do(ctx *ratchetingTestContext) error { name: "sentinel-resource", patch: map[string]interface{}{ sentinelName: fmt.Sprintf("invalid-%d", counter), - }}.Do(&ratchetingTestContext{ - T: ctx.T, - DynamicClient: ctx.DynamicClient, - APIExtensionsClient: ctx.APIExtensionsClient, - StatusSubresource: false, // Do not carry this over, sentinel check is in the root level. - }) + }}.Do(ctx) if err == nil { return false, errors.New("expected error when creating sentinel resource") From aa1d79c370124f8dda17239db32258428e8038d0 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Mon, 13 Jan 2025 11:45:50 +0000 Subject: [PATCH 4/4] Use DeepCopyJSON to copy testcase input Unstructured types must use int64, all integers in the test case must be explicitly cast as int64. --- .../test/integration/ratcheting_test.go | 153 +++++++++--------- 1 file changed, 74 insertions(+), 79 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go index baf52e439df..e71759d4b50 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/ratcheting_test.go @@ -165,12 +165,7 @@ func (a applyPatchOperation) Do(ctx *ratchetingTestContext) error { patch := &unstructured.Unstructured{} if obj, ok := a.patch.(map[string]interface{}); ok { - patch.Object = map[string]interface{}{} - - // Copy the map at the top level to avoid modifying the original. - for k, v := range obj { - patch.Object[k] = v - } + patch.Object = runtime.DeepCopyJSON(obj) } else if str, ok := a.patch.(string); ok { str = FixTabsOrDie(str) if err := utilyaml.NewYAMLOrJSONDecoder(strings.NewReader(str), len(str)).Decode(&patch.Object); err != nil { @@ -500,23 +495,23 @@ func TestRatchetingFunctionality(t *testing.T) { myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ - "hasMinimum": 0, - "hasMaximum": 1000, - "hasMinimumAndMaximum": 50, + "hasMinimum": int64(0), + "hasMaximum": int64(1000), + "hasMinimumAndMaximum": int64(50), }}, patchMyCRDV1Beta1Schema{ "Add stricter minimums and maximums that violate the previous object", map[string]interface{}{ "properties": map[string]interface{}{ "hasMinimum": map[string]interface{}{ - "minimum": 10, + "minimum": int64(10), }, "hasMaximum": map[string]interface{}{ - "maximum": 20, + "maximum": int64(20), }, "hasMinimumAndMaximum": map[string]interface{}{ - "minimum": 10, - "maximum": 20, + "minimum": int64(10), + "maximum": int64(20), }, "noRestrictions": map[string]interface{}{ "type": "integer", @@ -528,33 +523,33 @@ func TestRatchetingFunctionality(t *testing.T) { myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ - "noRestrictions": 50, + "noRestrictions": int64(50), }}, expectError{ applyPatchOperation{ "Change a single old field to be invalid", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ - "hasMinimum": 5, + "hasMinimum": int64(5), }}, }, expectError{ applyPatchOperation{ "Change multiple old fields to be invalid", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ - "hasMinimum": 5, - "hasMaximum": 21, + "hasMinimum": int64(5), + "hasMaximum": int64(21), }}, }, applyPatchOperation{ "Change single old field to be valid", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ - "hasMinimum": 11, + "hasMinimum": int64(11), }}, applyPatchOperation{ "Change multiple old fields to be valid", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ - "hasMaximum": 19, - "hasMinimumAndMaximum": 15, + "hasMaximum": int64(19), + "hasMinimumAndMaximum": int64(15), }}, }, }, @@ -633,8 +628,8 @@ func TestRatchetingFunctionality(t *testing.T) { "Create an instance", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ "nums": map[string]interface{}{ - "num1": 1, - "num2": 1000000, + "num1": int64(1), + "num2": int64(1000000), }, "content": map[string]interface{}{ "k1": "some content", @@ -647,7 +642,7 @@ func TestRatchetingFunctionality(t *testing.T) { "properties": map[string]interface{}{ "nums": map[string]interface{}{ "additionalProperties": map[string]interface{}{ - "minimum": 1000, + "minimum": int64(1000), }, }, }, @@ -656,16 +651,16 @@ func TestRatchetingFunctionality(t *testing.T) { "updating validating field num2 to another validating value, but rachet invalid field num1", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ "nums": map[string]interface{}{ - "num1": 1, - "num2": 2000, + "num1": int64(1), + "num2": int64(2000), }, }}, expectError{applyPatchOperation{ "update field num1 to different invalid value", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ "nums": map[string]interface{}{ - "num1": 2, - "num2": 2000, + "num1": int64(2), + "num2": int64(2000), }, }}}, }, @@ -703,8 +698,8 @@ func TestRatchetingFunctionality(t *testing.T) { map[string]interface{}{ "properties": map[string]interface{}{ "restricted": map[string]interface{}{ - "minProperties": 1, - "maxProperties": 1, + "minProperties": int64(1), + "maxProperties": int64(1), }, }, }}, @@ -736,7 +731,7 @@ func TestRatchetingFunctionality(t *testing.T) { map[string]interface{}{ "properties": map[string]interface{}{ "restricted": map[string]interface{}{ - "minProperties": 2, + "minProperties": int64(2), "maxProperties": nil, }, }, @@ -766,7 +761,7 @@ func TestRatchetingFunctionality(t *testing.T) { "properties": map[string]interface{}{ "restricted": map[string]interface{}{ "minProperties": nil, - "maxProperties": 1, + "maxProperties": int64(1), }, }, }}, @@ -820,7 +815,7 @@ func TestRatchetingFunctionality(t *testing.T) { map[string]interface{}{ "properties": map[string]interface{}{ "array": map[string]interface{}{ - "minItems": 10, + "minItems": int64(10), }, }, }}, @@ -882,7 +877,7 @@ func TestRatchetingFunctionality(t *testing.T) { map[string]interface{}{ "properties": map[string]interface{}{ "array": map[string]interface{}{ - "maxItems": 1, + "maxItems": int64(1), }, }, }}, @@ -941,10 +936,10 @@ func TestRatchetingFunctionality(t *testing.T) { map[string]interface{}{ "properties": map[string]interface{}{ "minField": map[string]interface{}{ - "minLength": 10, + "minLength": int64(10), }, "maxField": map[string]interface{}{ - "maxLength": 15, + "maxLength": int64(15), }, }, }}, @@ -1141,17 +1136,17 @@ func TestRatchetingFunctionality(t *testing.T) { "field": []interface{}{ map[string]interface{}{ "name": "nginx", - "port": 443, + "port": int64(443), "field": "value", }, map[string]interface{}{ "name": "etcd", - "port": 2379, + "port": int64(2379), "field": "value", }, map[string]interface{}{ "name": "kube-apiserver", - "port": 6443, + "port": int64(6443), "field": "value", }, }, @@ -1161,7 +1156,7 @@ func TestRatchetingFunctionality(t *testing.T) { map[string]interface{}{ "properties": map[string]interface{}{ "field": map[string]interface{}{ - "maxItems": 2, + "maxItems": int64(2), }, }, }}, @@ -1171,17 +1166,17 @@ func TestRatchetingFunctionality(t *testing.T) { "field": []interface{}{ map[string]interface{}{ "name": "kube-apiserver", - "port": 6443, + "port": int64(6443), "field": "value", }, map[string]interface{}{ "name": "nginx", - "port": 443, + "port": int64(443), "field": "value", }, map[string]interface{}{ "name": "etcd", - "port": 2379, + "port": int64(2379), "field": "value", }, }, @@ -1193,22 +1188,22 @@ func TestRatchetingFunctionality(t *testing.T) { "field": []interface{}{ map[string]interface{}{ "name": "kube-apiserver", - "port": 6443, + "port": int64(6443), "field": "value", }, map[string]interface{}{ "name": "nginx", - "port": 443, + "port": int64(443), "field": "value", }, map[string]interface{}{ "name": "etcd", - "port": 2379, + "port": int64(2379), "field": "value", }, map[string]interface{}{ "name": "dev", - "port": 8080, + "port": int64(8080), "field": "value", }, }, @@ -1222,7 +1217,7 @@ func TestRatchetingFunctionality(t *testing.T) { "items": map[string]interface{}{ "properties": map[string]interface{}{ "port": map[string]interface{}{ - "multipleOf": 2, + "multipleOf": int64(2), }, }, }, @@ -1236,17 +1231,17 @@ func TestRatchetingFunctionality(t *testing.T) { "field": []interface{}{ map[string]interface{}{ "name": "nginx", - "port": 443, + "port": int64(443), "field": "value", }, map[string]interface{}{ "name": "etcd", - "port": 2379, + "port": int64(2379), "field": "value", }, map[string]interface{}{ "name": "kube-apiserver", - "port": 6443, + "port": int64(6443), "field": "value", }, }, @@ -1258,22 +1253,22 @@ func TestRatchetingFunctionality(t *testing.T) { "field": []interface{}{ map[string]interface{}{ "name": "nginx", - "port": 443, + "port": int64(443), "field": "value", }, map[string]interface{}{ "name": "etcd", - "port": 2379, + "port": int64(2379), "field": "value", }, map[string]interface{}{ "name": "kube-apiserver", - "port": 6443, + "port": int64(6443), "field": "this is a changed value for an an invalid but grandfathered key", }, map[string]interface{}{ "name": "dev", - "port": 8080, + "port": int64(8080), "field": "value", }, }, @@ -1316,7 +1311,7 @@ func TestRatchetingFunctionality(t *testing.T) { "values": map[string]interface{}{ "items": map[string]interface{}{ "additionalProperties": map[string]interface{}{ - "minLength": 6, + "minLength": int64(6), }, }, }, @@ -1519,15 +1514,15 @@ func TestRatchetingFunctionality(t *testing.T) { "field": map[string]interface{}{ "object1": map[string]interface{}{ "stringField": "a string", - "intField": 5, + "intField": int64(5), }, "object2": map[string]interface{}{ "stringField": "another string", - "intField": 15, + "intField": int64(15), }, "object3": map[string]interface{}{ "stringField": "a third string", - "intField": 7, + "intField": int64(7), }, }, }}, @@ -1557,19 +1552,19 @@ func TestRatchetingFunctionality(t *testing.T) { "field": map[string]interface{}{ "object1": map[string]interface{}{ "stringField": "a string", - "intField": 5, + "intField": int64(5), }, "object2": map[string]interface{}{ "stringField": "another string", - "intField": 15, + "intField": int64(15), }, "object3": map[string]interface{}{ "stringField": "a third string", - "intField": 7, + "intField": int64(7), }, "object4": map[string]interface{}{ "stringField": "k8s third string", - "intField": 7, + "intField": int64(7), }, }, }}, @@ -1579,12 +1574,12 @@ func TestRatchetingFunctionality(t *testing.T) { "field": map[string]interface{}{ "object1": map[string]interface{}{ "stringField": "a string", - "intField": 15, + "intField": int64(15), }, "object2": map[string]interface{}{ "stringField": "another string", - "intField": 10, - "otherIntField": 20, + "intField": int64(10), + "otherIntField": int64(20), }, }, }}, @@ -1629,11 +1624,11 @@ func TestRatchetingFunctionality(t *testing.T) { "field": map[string]interface{}{ "object1": map[string]interface{}{ "stringField": "a string", // invalid. even number length, no k8s prefix - "intField": 1000, + "intField": int64(1000), }, "object4": map[string]interface{}{ "stringField": "k8s third string", // invalid. even number length. ratcheted - "intField": 7000, + "intField": int64(7000), }, }, }}, @@ -1644,11 +1639,11 @@ func TestRatchetingFunctionality(t *testing.T) { "field": map[string]interface{}{ "object1": map[string]interface{}{ "stringField": "k8s third string", - "intField": 1000, + "intField": int64(1000), }, "object4": map[string]interface{}{ "stringField": "a string", - "intField": 7000, + "intField": int64(7000), }, }, }}}, @@ -1658,11 +1653,11 @@ func TestRatchetingFunctionality(t *testing.T) { "field": map[string]interface{}{ "object1": map[string]interface{}{ "stringField": "k8s a stringy", - "intField": 1000, + "intField": int64(1000), }, "object4": map[string]interface{}{ "stringField": "k8s third stringy", - "intField": 7000, + "intField": int64(7000), }, }, }}, @@ -1701,7 +1696,7 @@ func TestRatchetingFunctionality(t *testing.T) { "reate a list of numbers with duplicates using the old simple schema", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ "values": map[string]interface{}{ - "dups": []interface{}{1, 2, 2, 3, 1000, 2000}, + "dups": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000)}, }, }}, patchMyCRDV1Beta1Schema{ @@ -1720,15 +1715,15 @@ func TestRatchetingFunctionality(t *testing.T) { "change original without removing duplicates", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ "values": map[string]interface{}{ - "dups": []interface{}{1, 2, 2, 3, 1000, 2000, 3}, + "dups": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000), int64(3)}, }, }}}, expectError{applyPatchOperation{ "add another list with duplicates", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ "values": map[string]interface{}{ - "dups": []interface{}{1, 2, 2, 3, 1000, 2000}, - "dups2": []interface{}{1, 2, 2, 3, 1000, 2000}, + "dups": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000)}, + "dups2": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000)}, }, }}}, // Can add a valid sibling field @@ -1738,8 +1733,8 @@ func TestRatchetingFunctionality(t *testing.T) { "add a valid sibling field", myCRDV1Beta1, myCRDInstanceName, map[string]interface{}{ "values": map[string]interface{}{ - "dups": []interface{}{1, 2, 2, 3, 1000, 2000}, - "otherField": []interface{}{1, 2, 3}, + "dups": []interface{}{int64(1), int64(2), int64(2), int64(3), int64(1000), int64(2000)}, + "otherField": []interface{}{int64(1), int64(2), int64(3)}, }, }}, // Can remove dups to make valid @@ -1752,8 +1747,8 @@ func TestRatchetingFunctionality(t *testing.T) { myCRDInstanceName, map[string]interface{}{ "values": map[string]interface{}{ - "dups": []interface{}{1, 3, 1000, 2000}, - "otherField": []interface{}{1, 2, 3}, + "dups": []interface{}{int64(1), int64(3), int64(1000), int64(2000)}, + "otherField": []interface{}{int64(1), int64(2), int64(3)}, }, }}, },