Simplify status subresource ratcheting testing

Author: JoelSpeed

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))
 	}
 

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",