Merge pull request #121460 from jiahuif-forks/feature/crd-validation-expressions/existing-expressions-cost-exempt

CRD Validation Rule: skip individual cost limit check for expressions of untouched versions.
This commit is contained in:
Kubernetes Prow Robot 2023-10-30 18:49:03 +01:00 committed by GitHub
commit 4e45e1d625
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 296 additions and 3 deletions

View File

@ -128,6 +128,13 @@ type validationOptions struct {
requireMapListKeysMapSetValidation bool
// preexistingExpressions tracks which CEL expressions existed in an object before an update. May be nil for create.
preexistingExpressions preexistingExpressions
// versionsWithUnchangedSchemas tracks schemas of which versions are unchanged when updating a CRD.
// Does not apply to creation or deletion.
// Some checks use this to avoid rejecting previously accepted versions due to a control plane upgrade/downgrade.
versionsWithUnchangedSchemas sets.Set[string]
// suppressPerExpressionCost indicates whether CEL per-expression cost limit should be suppressed.
// It will be automatically set during Versions validation if the version is in versionsWithUnchangedSchemas.
suppressPerExpressionCost bool
celEnvironmentSet *environment.EnvSet
}
@ -176,6 +183,37 @@ func findPreexistingExpressionsInSchema(schema *apiextensions.JSONSchemaProps, e
})
}
// findVersionsWithUnchangedSchemas finds each version that is in the new CRD object and differs from that of the old CRD object.
// It returns a set of the names of mutated versions.
// This function does not check for duplicated versions, top-level version not in versions, or coexistence of
// top-level and per-version schemas, as further validations will check for these problems.
func findVersionsWithUnchangedSchemas(obj, oldObject *apiextensions.CustomResourceDefinition) sets.Set[string] {
versionsWithUnchangedSchemas := sets.New[string]()
for _, version := range obj.Spec.Versions {
newSchema, err := apiextensions.GetSchemaForVersion(obj, version.Name)
if err != nil {
continue
}
oldSchema, err := apiextensions.GetSchemaForVersion(oldObject, version.Name)
if err != nil {
continue
}
if apiequality.Semantic.DeepEqual(newSchema, oldSchema) {
versionsWithUnchangedSchemas.Insert(version.Name)
}
}
return versionsWithUnchangedSchemas
}
// suppressExpressionCostForUnchangedSchema returns a copy of opts with suppressPerExpressionCost set to true if
// the specified version's schema is unchanged.
func suppressExpressionCostForUnchangedSchema(opts validationOptions, version string) validationOptions {
if opts.versionsWithUnchangedSchemas.Has(version) {
opts.suppressPerExpressionCost = true
}
return opts
}
// ValidateCustomResourceDefinitionUpdate statically validates
// context is passed for supporting context cancellation during cel validation when validating defaults
func ValidateCustomResourceDefinitionUpdate(ctx context.Context, obj, oldObj *apiextensions.CustomResourceDefinition) field.ErrorList {
@ -190,6 +228,7 @@ func ValidateCustomResourceDefinitionUpdate(ctx context.Context, obj, oldObj *ap
requireAtomicSetType: requireAtomicSetType(&oldObj.Spec),
requireMapListKeysMapSetValidation: requireMapListKeysMapSetValidation(&oldObj.Spec),
preexistingExpressions: findPreexistingExpressions(&oldObj.Spec),
versionsWithUnchangedSchemas: findVersionsWithUnchangedSchemas(obj, oldObj),
celEnvironmentSet: environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()),
}
return validateCustomResourceDefinitionUpdate(ctx, obj, oldObj, opts)
@ -246,6 +285,7 @@ func validateCustomResourceDefinitionVersion(ctx context.Context, version *apiex
for _, err := range validateDeprecationWarning(version.Deprecated, version.DeprecationWarning) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("deprecationWarning"), version.DeprecationWarning, err))
}
opts = suppressExpressionCostForUnchangedSchema(opts, version.Name)
allErrs = append(allErrs, validateCustomResourceDefinitionValidation(ctx, version.Schema, statusEnabled, opts, fldPath.Child("schema"))...)
allErrs = append(allErrs, ValidateCustomResourceDefinitionSubresources(version.Subresources, fldPath.Child("subresources"))...)
for i := range version.AdditionalPrinterColumns {
@ -404,7 +444,7 @@ func validateCustomResourceDefinitionSpec(ctx context.Context, spec *apiextensio
}
allErrs = append(allErrs, ValidateCustomResourceDefinitionNames(&spec.Names, fldPath.Child("names"))...)
allErrs = append(allErrs, validateCustomResourceDefinitionValidation(ctx, spec.Validation, hasAnyStatusEnabled(spec), opts, fldPath.Child("validation"))...)
allErrs = append(allErrs, validateCustomResourceDefinitionValidation(ctx, spec.Validation, hasAnyStatusEnabled(spec), suppressExpressionCostForUnchangedSchema(opts, spec.Version), fldPath.Child("validation"))...)
allErrs = append(allErrs, ValidateCustomResourceDefinitionSubresources(spec.Subresources, fldPath.Child("subresources"))...)
for i := range spec.AdditionalPrinterColumns {
@ -1115,7 +1155,7 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch
} else {
for i, cr := range compResults {
expressionCost := getExpressionCost(cr, celContext)
if expressionCost > StaticEstimatedCostLimit {
if !opts.suppressPerExpressionCost && expressionCost > StaticEstimatedCostLimit {
costErrorMsg := getCostErrorMessage("estimated rule cost", expressionCost, StaticEstimatedCostLimit)
allErrs.CELErrors = append(allErrs.CELErrors, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("rule"), costErrorMsg))
}
@ -1133,7 +1173,7 @@ func ValidateCustomResourceDefinitionOpenAPISchema(schema *apiextensions.JSONSch
allErrs.CELErrors = append(allErrs.CELErrors, field.Invalid(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), schema.XValidations[i], cr.MessageExpressionError.Detail))
} else {
if cr.MessageExpression != nil {
if cr.MessageExpressionMaxCost > StaticEstimatedCostLimit {
if !opts.suppressPerExpressionCost && cr.MessageExpressionMaxCost > StaticEstimatedCostLimit {
costErrorMsg := getCostErrorMessage("estimated messageExpression cost", cr.MessageExpressionMaxCost, StaticEstimatedCostLimit)
allErrs.CELErrors = append(allErrs.CELErrors, field.Forbidden(fldPath.Child("x-kubernetes-validations").Index(i).Child("messageExpression"), costErrorMsg))
}

View File

@ -6698,6 +6698,259 @@ func TestValidateCustomResourceDefinitionUpdate(t *testing.T) {
forbidden("spec", "validation", "openAPIV3Schema", "properties[bar]", "items", "nullable"),
},
},
{
name: "suppress per-expression cost limit in pre-existing versions",
old: &apiextensions.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com", ResourceVersion: "1"},
Spec: apiextensions.CustomResourceDefinitionSpec{
Group: "group.com",
Scope: apiextensions.ResourceScope("Cluster"),
Names: apiextensions.CustomResourceDefinitionNames{Plural: "plural", Singular: "singular", Kind: "Plural", ListKind: "PluralList"},
Versions: []apiextensions.CustomResourceDefinitionVersion{
{
Name: "v1",
Served: true,
Storage: true,
Schema: &apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensions.JSONSchemaProps{
"f": {
Type: "array",
Items: &apiextensions.JSONSchemaPropsOrArray{
Schema: &apiextensions.JSONSchemaProps{
Type: "string",
},
},
XValidations: apiextensions.ValidationRules{
{
Rule: "true",
MessageExpression: `self[0] + self[1] + self[2] + self[3] + self[4] + self[5] + self[6] + self[7]`,
},
},
},
},
},
},
},
{
Name: "v2",
Schema: &apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensions.JSONSchemaProps{
"f": {
Type: "array",
Items: &apiextensions.JSONSchemaPropsOrArray{
Schema: &apiextensions.JSONSchemaProps{
Type: "string",
},
},
XValidations: apiextensions.ValidationRules{
{
Rule: "true",
MessageExpression: `self[0] + self[1] + self[2] + self[3] + self[4] + self[5] + self[6] + self[7]`,
},
},
},
},
},
},
},
},
},
Status: apiextensions.CustomResourceDefinitionStatus{StoredVersions: []string{"v1"}},
},
resource: &apiextensions.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com", ResourceVersion: "1"},
Spec: apiextensions.CustomResourceDefinitionSpec{
Group: "group.com",
Scope: apiextensions.ResourceScope("Cluster"),
Names: apiextensions.CustomResourceDefinitionNames{Plural: "plural", Singular: "singular", Kind: "Plural", ListKind: "PluralList"},
Versions: []apiextensions.CustomResourceDefinitionVersion{
{
Name: "v1", // unchanged
Served: true,
Storage: true,
Schema: &apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensions.JSONSchemaProps{
"f": {
Type: "array",
Items: &apiextensions.JSONSchemaPropsOrArray{
Schema: &apiextensions.JSONSchemaProps{
Type: "string",
},
},
XValidations: apiextensions.ValidationRules{
{
Rule: "true",
MessageExpression: `self[0] + self[1] + self[2] + self[3] + self[4] + self[5] + self[6] + self[7]`,
},
},
},
},
},
},
},
{
Name: "v2", // touched
Schema: &apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensions.JSONSchemaProps{
"f": {
Type: "array",
Items: &apiextensions.JSONSchemaPropsOrArray{
Schema: &apiextensions.JSONSchemaProps{
Type: "string",
},
},
XValidations: apiextensions.ValidationRules{
{
Rule: "true",
MessageExpression: `self[0] + self[1] + self[2] + self[3] + self[4] + self[5] + self[6] + self[7] + self[8]`,
},
},
},
},
},
},
},
{
Name: "v3", // new
Schema: &apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensions.JSONSchemaProps{
"f": {
Type: "array",
Items: &apiextensions.JSONSchemaPropsOrArray{
Schema: &apiextensions.JSONSchemaProps{
Type: "string",
},
},
XValidations: apiextensions.ValidationRules{
{
Rule: "true",
MessageExpression: `self[0] + self[1] + self[2] + self[3] + self[4] + self[5] + self[6] + self[7]`,
},
},
},
},
},
},
},
},
},
Status: apiextensions.CustomResourceDefinitionStatus{StoredVersions: []string{"v1"}},
},
errors: []validationMatch{
// versions[0] is exempted because it existed in oldObject
forbidden("spec", "versions[1]", "schema", "openAPIV3Schema", "properties[f]", "x-kubernetes-validations[0]", "messageExpression"),
forbidden("spec", "versions[2]", "schema", "openAPIV3Schema", "properties[f]", "x-kubernetes-validations[0]", "messageExpression"),
},
},
{
name: "suppress per-expression cost limit in new object during top-level schema to Versions extraction",
old: &apiextensions.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com", ResourceVersion: "1"},
Spec: apiextensions.CustomResourceDefinitionSpec{
Group: "group.com",
Scope: apiextensions.ResourceScope("Cluster"),
Names: apiextensions.CustomResourceDefinitionNames{Plural: "plural", Singular: "singular", Kind: "Plural", ListKind: "PluralList"},
Version: "v1",
Validation: &apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensions.JSONSchemaProps{
"f": {
Type: "array",
Items: &apiextensions.JSONSchemaPropsOrArray{
Schema: &apiextensions.JSONSchemaProps{
Type: "string",
},
},
XValidations: apiextensions.ValidationRules{
{
Rule: "true",
MessageExpression: `self[0] + self[1] + self[2] + self[3] + self[4] + self[5] + self[6] + self[7]`,
},
},
},
},
},
},
},
Status: apiextensions.CustomResourceDefinitionStatus{StoredVersions: []string{"v1"}},
},
resource: &apiextensions.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{Name: "plural.group.com", ResourceVersion: "1"},
Spec: apiextensions.CustomResourceDefinitionSpec{
Group: "group.com",
Scope: apiextensions.ResourceScope("Cluster"),
Names: apiextensions.CustomResourceDefinitionNames{Plural: "plural", Singular: "singular", Kind: "Plural", ListKind: "PluralList"},
Versions: []apiextensions.CustomResourceDefinitionVersion{
{
Name: "v1", // unchanged, was top-level
Served: true,
Storage: true,
Schema: &apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensions.JSONSchemaProps{
"f": {
Type: "array",
Items: &apiextensions.JSONSchemaPropsOrArray{
Schema: &apiextensions.JSONSchemaProps{
Type: "string",
},
},
XValidations: apiextensions.ValidationRules{
{
Rule: "true",
MessageExpression: `self[0] + self[1] + self[2] + self[3] + self[4] + self[5] + self[6] + self[7]`,
},
},
},
},
},
},
},
{
Name: "v2", // new
Schema: &apiextensions.CustomResourceValidation{
OpenAPIV3Schema: &apiextensions.JSONSchemaProps{
Type: "object",
Properties: map[string]apiextensions.JSONSchemaProps{
"f": {
Type: "array",
Items: &apiextensions.JSONSchemaPropsOrArray{
Schema: &apiextensions.JSONSchemaProps{
Type: "string",
},
},
XValidations: apiextensions.ValidationRules{
{
Rule: "true",
MessageExpression: `self[0] + self[1] + self[2] + self[3] + self[4] + self[5] + self[6] + self[7] + self[8]`,
},
},
},
},
},
},
},
},
},
Status: apiextensions.CustomResourceDefinitionStatus{StoredVersions: []string{"v1"}},
},
errors: []validationMatch{
// versions[0] is exempted because it existed in oldObject as top-level schema.
forbidden("spec", "versions[1]", "schema", "openAPIV3Schema", "properties[f]", "x-kubernetes-validations[0]", "messageExpression"),
},
},
}
for _, tc := range tests {