From 9ee6d97fc057c3696977fb865b9b47037b407bb9 Mon Sep 17 00:00:00 2001 From: Alexander Zielenski Date: Tue, 18 Jul 2023 11:24:21 -0700 Subject: [PATCH] refactor: add ValidateCustomResourceUpdate to support future validators for CRD Updates --- .../pkg/apiserver/customresource_handler.go | 12 ++-- .../pkg/apiserver/validation/validation.go | 60 ++++++++++++++++++- .../pkg/registry/customresource/strategy.go | 5 +- .../pkg/registry/customresource/validator.go | 17 ++++-- 4 files changed, 76 insertions(+), 18 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index a993aa46410..2a6dcb80a7c 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -79,8 +79,6 @@ import ( "k8s.io/klog/v2" "k8s.io/kube-openapi/pkg/spec3" "k8s.io/kube-openapi/pkg/validation/spec" - "k8s.io/kube-openapi/pkg/validation/strfmt" - "k8s.io/kube-openapi/pkg/validation/validate" ) // crdHandler serves the `/apis` endpoint. @@ -740,8 +738,9 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd return nil, fmt.Errorf("the server could not properly serve the CR schema") } var internalSchemaProps *apiextensionsinternal.JSONSchemaProps + var internalValidationSchema *apiextensionsinternal.CustomResourceValidation if validationSchema != nil { - internalValidationSchema := &apiextensionsinternal.CustomResourceValidation{} + internalValidationSchema = &apiextensionsinternal.CustomResourceValidation{} if err := apiextensionsv1.Convert_v1_CustomResourceValidation_To_apiextensions_CustomResourceValidation(validationSchema, internalValidationSchema, nil); err != nil { return nil, fmt.Errorf("failed to convert CRD validation to internal version: %v", err) } @@ -753,7 +752,7 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd } var statusSpec *apiextensionsinternal.CustomResourceSubresourceStatus - var statusValidator *validate.SchemaValidator + var statusValidator apiservervalidation.SchemaValidator subresources, err := apiextensionshelpers.GetSubresourcesForVersion(crd, v.Name) if err != nil { utilruntime.HandleError(err) @@ -768,11 +767,10 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd // for the status subresource, validate only against the status schema if internalValidationSchema != nil && internalValidationSchema.OpenAPIV3Schema != nil && internalValidationSchema.OpenAPIV3Schema.Properties != nil { if statusSchema, ok := internalValidationSchema.OpenAPIV3Schema.Properties["status"]; ok { - openapiSchema := &spec.Schema{} - if err := apiservervalidation.ConvertJSONSchemaPropsWithPostProcess(&statusSchema, openapiSchema, apiservervalidation.StripUnsupportedFormatsPostProcess); err != nil { + statusValidator, _, err = apiservervalidation.NewSchemaValidator(&statusSchema) + if err != nil { return nil, err } - statusValidator = validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default) } } } diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go index 13e7d7739c8..8ead1038a60 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/validation.go @@ -29,8 +29,34 @@ import ( "k8s.io/kube-openapi/pkg/validation/validate" ) +type SchemaValidator interface { + SchemaCreateValidator + ValidateUpdate(new, old interface{}) *validate.Result +} + +type SchemaCreateValidator interface { + Validate(value interface{}) *validate.Result +} + +// basicSchemaValidator wraps a kube-openapi SchemaCreateValidator to +// support ValidateUpdate. It implements ValidateUpdate by simply validating +// the new value via kube-openapi, ignoring the old value. +type basicSchemaValidator struct { + *validate.SchemaValidator +} + +func (s basicSchemaValidator) ValidateUpdate(new, old interface{}) *validate.Result { + return s.Validate(new) +} + // NewSchemaValidator creates an openapi schema validator for the given CRD validation. -func NewSchemaValidator(customResourceValidation *apiextensions.JSONSchemaProps) (*validate.SchemaValidator, *spec.Schema, error) { +// +// If feature `CRDValidationRatcheting` is disabled, this returns validator which +// validates all `Update`s and `Create`s as a `Create` - without considering old value. +// +// If feature `CRDValidationRatcheting` is enabled - the validator returned +// will support ratcheting unchanged correlatable fields across an update. +func NewSchemaValidator(customResourceValidation *apiextensions.JSONSchemaProps) (SchemaValidator, *spec.Schema, error) { // Convert CRD schema to openapi schema openapiSchema := &spec.Schema{} if customResourceValidation != nil { @@ -39,12 +65,35 @@ func NewSchemaValidator(customResourceValidation *apiextensions.JSONSchemaProps) return nil, nil, err } } - return validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default), openapiSchema, nil + + return basicSchemaValidator{validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default)}, openapiSchema, nil +} + +// ValidateCustomResourceUpdate validates the transition of Custom Resource from +// `old` to `new` against the schema in the CustomResourceDefinition. +// Both customResource and old represent a JSON data structures. +// +// If feature `CRDValidationRatcheting` is disabled, this behaves identically to +// ValidateCustomResource(customResource). +func ValidateCustomResourceUpdate(fldPath *field.Path, customResource, old interface{}, validator SchemaValidator) field.ErrorList { + // Additional feature gate check for sanity + if !utilfeature.DefaultFeatureGate.Enabled(features.CRDValidationRatcheting) { + return ValidateCustomResource(nil, customResource, validator) + } else if validator == nil { + return nil + } + + result := validator.ValidateUpdate(customResource, old) + if result.IsValid() { + return nil + } + + return kubeOpenAPIResultToFieldErrors(fldPath, result) } // ValidateCustomResource validates the Custom Resource against the schema in the CustomResourceDefinition. // CustomResource is a JSON data structure. -func ValidateCustomResource(fldPath *field.Path, customResource interface{}, validator *validate.SchemaValidator) field.ErrorList { +func ValidateCustomResource(fldPath *field.Path, customResource interface{}, validator SchemaCreateValidator) field.ErrorList { if validator == nil { return nil } @@ -53,6 +102,11 @@ func ValidateCustomResource(fldPath *field.Path, customResource interface{}, val if result.IsValid() { return nil } + + return kubeOpenAPIResultToFieldErrors(fldPath, result) +} + +func kubeOpenAPIResultToFieldErrors(fldPath *field.Path, result *validate.Result) field.ErrorList { var allErrs field.ErrorList for _, err := range result.Errors { switch err := err.(type) { diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go index 92a688a56d7..b534c627343 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/registry/customresource/strategy.go @@ -19,13 +19,12 @@ package customresource import ( "context" - "k8s.io/kube-openapi/pkg/validation/validate" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" structuralschema "k8s.io/apiextensions-apiserver/pkg/apiserver/schema" "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel" structurallisttype "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/listtype" schemaobjectmeta "k8s.io/apiextensions-apiserver/pkg/apiserver/schema/objectmeta" + "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -58,7 +57,7 @@ type customResourceStrategy struct { kind schema.GroupVersionKind } -func NewStrategy(typer runtime.ObjectTyper, namespaceScoped bool, kind schema.GroupVersionKind, schemaValidator, statusSchemaValidator *validate.SchemaValidator, structuralSchemas map[string]*structuralschema.Structural, status *apiextensions.CustomResourceSubresourceStatus, scale *apiextensions.CustomResourceSubresourceScale) customResourceStrategy { +func NewStrategy(typer runtime.ObjectTyper, namespaceScoped bool, kind schema.GroupVersionKind, schemaValidator, statusSchemaValidator validation.SchemaValidator, structuralSchemas map[string]*structuralschema.Structural, status *apiextensions.CustomResourceSubresourceStatus, scale *apiextensions.CustomResourceSubresourceScale) customResourceStrategy { celValidators := map[string]*cel.Validator{} if utilfeature.DefaultFeatureGate.Enabled(features.CustomResourceValidationExpressions) { for name, s := range structuralSchemas { 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 e64955a40b2..a158aa3b6dc 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 @@ -28,7 +28,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" - "k8s.io/kube-openapi/pkg/validation/validate" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" apiextensionsvalidation "k8s.io/apiextensions-apiserver/pkg/apiserver/validation" @@ -37,8 +36,8 @@ import ( type customResourceValidator struct { namespaceScoped bool kind schema.GroupVersionKind - schemaValidator *validate.SchemaValidator - statusSchemaValidator *validate.SchemaValidator + schemaValidator apiextensionsvalidation.SchemaValidator + statusSchemaValidator apiextensionsvalidation.SchemaValidator } func (a customResourceValidator) Validate(ctx context.Context, obj runtime.Object, scale *apiextensions.CustomResourceSubresourceScale) field.ErrorList { @@ -70,6 +69,10 @@ func (a customResourceValidator) ValidateUpdate(ctx context.Context, obj, old ru if !ok { return field.ErrorList{field.Invalid(field.NewPath(""), u, fmt.Sprintf("has type %T. Must be a pointer to an Unstructured type", u))} } + oldU, ok := old.(*unstructured.Unstructured) + if !ok { + return field.ErrorList{field.Invalid(field.NewPath(""), old, fmt.Sprintf("has type %T. Must be a pointer to an Unstructured type", u))} + } objAccessor, err := meta.Accessor(obj) if err != nil { return field.ErrorList{field.Invalid(field.NewPath("metadata"), nil, err.Error())} @@ -86,7 +89,7 @@ func (a customResourceValidator) ValidateUpdate(ctx context.Context, obj, old ru var allErrs field.ErrorList allErrs = append(allErrs, validation.ValidateObjectMetaAccessorUpdate(objAccessor, oldAccessor, field.NewPath("metadata"))...) - allErrs = append(allErrs, apiextensionsvalidation.ValidateCustomResource(nil, u.UnstructuredContent(), a.schemaValidator)...) + allErrs = append(allErrs, apiextensionsvalidation.ValidateCustomResourceUpdate(nil, u.UnstructuredContent(), oldU.UnstructuredContent(), a.schemaValidator)...) allErrs = append(allErrs, a.ValidateScaleSpec(ctx, u, scale)...) allErrs = append(allErrs, a.ValidateScaleStatus(ctx, u, scale)...) @@ -103,6 +106,10 @@ func (a customResourceValidator) ValidateStatusUpdate(ctx context.Context, obj, if !ok { return field.ErrorList{field.Invalid(field.NewPath(""), u, fmt.Sprintf("has type %T. Must be a pointer to an Unstructured type", u))} } + oldU, ok := old.(*unstructured.Unstructured) + if !ok { + return field.ErrorList{field.Invalid(field.NewPath(""), old, fmt.Sprintf("has type %T. Must be a pointer to an Unstructured type", u))} + } objAccessor, err := meta.Accessor(obj) if err != nil { return field.ErrorList{field.Invalid(field.NewPath("metadata"), nil, err.Error())} @@ -119,7 +126,7 @@ func (a customResourceValidator) ValidateStatusUpdate(ctx context.Context, obj, var allErrs field.ErrorList allErrs = append(allErrs, validation.ValidateObjectMetaAccessorUpdate(objAccessor, oldAccessor, field.NewPath("metadata"))...) - allErrs = append(allErrs, apiextensionsvalidation.ValidateCustomResource(nil, u.UnstructuredContent(), a.schemaValidator)...) + allErrs = append(allErrs, apiextensionsvalidation.ValidateCustomResourceUpdate(nil, u.UnstructuredContent(), oldU, a.schemaValidator)...) allErrs = append(allErrs, a.ValidateScaleStatus(ctx, u, scale)...) return allErrs