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