KEP-4008: CRD Validation Ratcheting alpha implementation

[alexzielenski]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements alpha of KEP-4008

Depends on kubernetes/kube-openapi#406

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adds new CRDValidationRatcheting alpha feature. During a PATCH or UPDATE Validation Ratcheting discards errors thrown by unchanged portions of the resource from most OpenAPI schema validations. 

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://kep.k8s.io/4008

[liggitt]

/lgtm
/approve

for reference, here's the diff of changes

diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go
index 73792e546e5..6565d83eee5 100644
--- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go
+++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/validation/ratcheting.go
@@ -59,8 +59,8 @@ func (r *RatchetingSchemaValidator) Validate(new interface{}) *validate.Result {
 	return sv.Validate(new)
 }
 
-func (r *RatchetingSchemaValidator) ValidateUpdate(old, new interface{}) *validate.Result {
-	return newRatchetingValueValidator(old, new, r.schemaArgs).Validate()
+func (r *RatchetingSchemaValidator) ValidateUpdate(new, old interface{}) *validate.Result {
+	return newRatchetingValueValidator(new, old, r.schemaArgs).Validate()
 }
 
 // ratchetingValueValidator represents an invocation of SchemaValidator.ValidateUpdate
@@ -108,7 +108,7 @@ type ratchetingValueValidator struct {
 	children map[interface{}]*ratchetingValueValidator
 }
 
-func newRatchetingValueValidator(oldValue, newValue interface{}, args schemaArgs) *ratchetingValueValidator {
+func newRatchetingValueValidator(newValue, oldValue interface{}, args schemaArgs) *ratchetingValueValidator {
 	return &ratchetingValueValidator{
 		oldValue:   oldValue,
 		value:      newValue,
@@ -192,7 +192,7 @@ func (r *ratchetingValueValidator) SubPropertyValidator(field string, schema *sp
 	}
 
 	return inlineValidator(func(new interface{}) *validate.Result {
-		childNode := newRatchetingValueValidator(oldValueForField, new, schemaArgs{
+		childNode := newRatchetingValueValidator(new, oldValueForField, schemaArgs{
 			schema:       schema,
 			root:         rootSchema,
 			path:         root,
@@ -221,7 +221,7 @@ func (r *ratchetingValueValidator) SubIndexValidator(index int, schema *spec.Sch
 	}
 
 	return inlineValidator(func(new interface{}) *validate.Result {
-		childNode := newRatchetingValueValidator(oldValueForIndex, new, schemaArgs{
+		childNode := newRatchetingValueValidator(new, oldValueForIndex, schemaArgs{
 			schema:       schema,
 			root:         rootSchema,
 			path:         root,
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 b08d9491463..e0042356ac0 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
@@ -33,22 +33,31 @@ import (
 
 type SchemaValidator interface {
 	SchemaCreateValidator
-	ValidateUpdate(old, new interface{}) *validate.Result
+	ValidateUpdate(new, old interface{}) *validate.Result
 }
 
 type SchemaCreateValidator interface {
 	Validate(value interface{}) *validate.Result
 }
 
-type schemaValidator struct {
+// 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 schemaValidator) ValidateUpdate(old, new interface{}) *validate.Result {
+func (s basicSchemaValidator) ValidateUpdate(new, old interface{}) *validate.Result {
 	return s.Validate(new)
 }
 
 // NewSchemaValidator creates an openapi schema validator for the given CRD validation.
+//
+// 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{}
@@ -62,21 +71,24 @@ func NewSchemaValidator(customResourceValidation *apiextensions.JSONSchemaProps)
 	if utilfeature.DefaultFeatureGate.Enabled(features.CRDValidationRatcheting) {
 		return NewRatchetingSchemaValidator(openapiSchema, nil, "", strfmt.Default), openapiSchema, nil
 	}
-	return schemaValidator{validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default)}, openapiSchema, nil
+	return basicSchemaValidator{validate.NewSchemaValidator(openapiSchema, nil, "", strfmt.Default)}, openapiSchema, nil
 }
 
-// ValidateCustomResourceUodate validates the transition of Custom Resource from
+// 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(nil, customResource).
-func ValidateCustomResourceUpdate(fldPath *field.Path, old, customResource interface{}, validator SchemaValidator) field.ErrorList {
-	if validator == nil {
+// 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(old, customResource)
+	result := validator.ValidateUpdate(customResource, old)
 	if result.IsValid() {
 		return nil
 	}
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 3d17cc9f556..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
@@ -89,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.ValidateCustomResourceUpdate(nil, oldU.UnstructuredContent(), 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)...)
 
@@ -126,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.ValidateCustomResourceUpdate(nil, oldU, 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

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: d1999e7538e5d78dacbcaee9823e0cfc40c06658

[liggitt]

noticed @apelisse had a comment on one of the scenarios at #118990 (comment)

can unhold once that is resolved and @apelisse lgtms

[apelisse]

/lgtm
/approve
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski, apelisse, liggitt, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/features/OWNERS [liggitt]
• staging/src/k8s.io/apiextensions-apiserver/OWNERS [liggitt,sttts]
• staging/src/k8s.io/apiextensions-apiserver/pkg/apis/OWNERS [liggitt]
• vendor/OWNERS [liggitt,sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sftim]

The changelog for this PR is light on the detail of what that feature (not feature gate, actual feature) does or means.

[sttts]

@alexzielenski ^

[alexzielenski]

Updated release note, PTAL