From ba816967a0cd2e8d9b99896b1f2908c48d4e920f Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 8 Jan 2025 15:38:51 +0000 Subject: [PATCH] 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",