Fix status subresource ratcheting

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
 }
 

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

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