CRD Validation Rule: skip individual cost limit check for expressions of untouched versions. #121460

jiahuif · 2023-10-23T23:49:28Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR skips expressions and messageExpressions of versions that are not mutated during update for the check of per-expression cost limit.

Which issue(s) this PR fixes:

Fixes #120821

Special notes for your reviewer:

Total cost calculation still includes these expressions.

Does this PR introduce a user-facing change?

When updating a CRD, per-expression cost limit check is skipped for x-kubernetes-validations rules of versions that are not mutated.

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

jiahuif · 2023-10-23T23:50:25Z

/sig api-machinery
/assign @cici37

cici37 · 2023-10-24T17:30:31Z

/lgtm
/api-review
/assign @liggitt
for approval. Thank you :)

k8s-ci-robot · 2023-10-24T17:30:37Z

LGTM label has been added.

Git tree hash: 1aa38537f4174e0ae77379a746faf78d7839822c

MaryamTavakkoli · 2023-10-25T06:07:41Z

Hello!
Bug triage for the 1.29 release cycle is here!
The code freeze will start (01:00 UTC Wednesday 1st November 2023 / 18:00 PDT Tuesday 31st October 2023), which is about 1 week from now. And while there is still plenty of time, we want to make sure that every PR has a chance to be merged on time.

As the PR is tagged for 1.29, is it still planned for that release?

cici37 · 2023-10-25T16:07:14Z

Hello! Bug triage for the 1.29 release cycle is here! The code freeze will start (01:00 UTC Wednesday 1st November 2023 / 18:00 PDT Tuesday 31st October 2023), which is about 1 week from now. And while there is still plenty of time, we want to make sure that every PR has a chance to be merged on time.

As the PR is tagged for 1.29, is it still planned for that release?

Yes it is planned for 1.29 and waiting for the final API approval. Thanks

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

cici37 · 2023-10-26T23:00:17Z

@liggitt The PR has been updated to skip estimated cost check based on if the version schema changed and leave the overall estimated cost check as what it is. Does that align with what's in your mind? Thank you!

liggitt · 2023-10-27T03:36:54Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

+// findMutatedVersions 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, as further validations check their uniqueness.
+// This function does not care about the root version.


why doesn't this function care about the root version? if there's only a single version, or the schema for all versions is identical, that's where it lives in the internal CRD type:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go

Lines 128 to 169 in ebf46ce

// If versions[*].{subresources,schema,additionalPrinterColumns} are identical, move to spec

subresources := out.Versions[0].Subresources

subresourcesIdentical := true

validation := out.Versions[0].Schema

validationIdentical := true

additionalPrinterColumns := out.Versions[0].AdditionalPrinterColumns

additionalPrinterColumnsIdentical := true

// Detect if per-version fields are identical

for _, v := range out.Versions {

if subresourcesIdentical && !apiequality.Semantic.DeepEqual(v.Subresources, subresources) {

subresourcesIdentical = false

}

if validationIdentical && !apiequality.Semantic.DeepEqual(v.Schema, validation) {

validationIdentical = false

}

if additionalPrinterColumnsIdentical && !apiequality.Semantic.DeepEqual(v.AdditionalPrinterColumns, additionalPrinterColumns) {

additionalPrinterColumnsIdentical = false

}

}

// If they are, set the top-level fields and clear the per-version fields

if subresourcesIdentical {

out.Subresources = subresources

}

if validationIdentical {

out.Validation = validation

}

if additionalPrinterColumnsIdentical {

out.AdditionalPrinterColumns = additionalPrinterColumns

}

for i := range out.Versions {

if subresourcesIdentical {

out.Versions[i].Subresources = nil

}

if validationIdentical {

out.Versions[i].Schema = nil

}

if additionalPrinterColumnsIdentical {

out.Versions[i].AdditionalPrinterColumns = nil

}

}

use the GetSchemaForVersion helper method to get the schema for the each version

something like this:

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 reflect.DeepEqual(newSchema, oldSchema) { versionsWithUnchangedSchemas.Insert(version.Name) } }

Thank you for the hint! main reason for not wanting to use GetSchemaForVersion is that it scans through versions, makes this function O(N^2) for N versions. However, because the number of versions are small, it is better to reuse existing helper function.

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go

jiahuif · 2023-10-27T19:20:45Z

@liggitt Thank you SO much for reviewing! Now that all comments are addressed, could you take another look?

cici37 · 2023-10-30T15:09:08Z

/lgtm
@liggitt Would you mind taking a look when have time? Thank you!

k8s-ci-robot · 2023-10-30T15:09:16Z

LGTM label has been added.

Git tree hash: 023e4aa0f005510b8d0172181320feeb129d721e

liggitt · 2023-10-30T16:06:14Z

/lgtm
/approve

k8s-ci-robot · 2023-10-30T16:06:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jiahuif, liggitt

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:

~~staging/src/k8s.io/apiextensions-apiserver/pkg/apis/OWNERS~~ [liggitt]

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

k8s-ci-robot requested review from liggitt and mikedanese October 23, 2023 23:49

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 23, 2023

k8s-ci-robot assigned cici37 Oct 23, 2023

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 23, 2023

k8s-ci-robot assigned liggitt Oct 24, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 24, 2023

cici37 added api-review Categorizes an issue or PR as actively needing an API review. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Oct 24, 2023

k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 24, 2023

liggitt added this to the v1.29 milestone Oct 24, 2023

liggitt reviewed Oct 26, 2023

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go Outdated Show resolved Hide resolved

jiahuif force-pushed the feature/crd-validation-expressions/existing-expressions-cost-exempt branch from 1ce45d6 to 209241b Compare October 26, 2023 22:48

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 26, 2023

k8s-ci-robot requested a review from cici37 October 26, 2023 22:48

k8s-ci-robot requested a review from liggitt October 26, 2023 22:48

jiahuif changed the title ~~CRD Validation Rule: skip existing expressions for individual cost limit.~~ CRD Validation Rule: skip individual cost limit check for expressions of untouched versions. Oct 26, 2023

liggitt reviewed Oct 27, 2023

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go Outdated Show resolved Hide resolved

liggitt reviewed Oct 27, 2023

View reviewed changes

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go Outdated Show resolved Hide resolved

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go Outdated Show resolved Hide resolved

jiahuif force-pushed the feature/crd-validation-expressions/existing-expressions-cost-exempt branch 4 times, most recently from 86dea64 to 4030667 Compare October 27, 2023 19:16

suppress per-expression cost limit for unmutated versions.

c1fd3f5

jiahuif force-pushed the feature/crd-validation-expressions/existing-expressions-cost-exempt branch from 4030667 to c1fd3f5 Compare October 27, 2023 19:16

cici37 mentioned this pull request Oct 27, 2023

[KEP-2876] Promote CRD validation rule to GA #121373

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2023

k8s-ci-robot merged commit 4e45e1d into kubernetes:master Oct 30, 2023
14 checks passed

jiahuif deleted the feature/crd-validation-expressions/existing-expressions-cost-exempt branch October 30, 2023 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRD Validation Rule: skip individual cost limit check for expressions of untouched versions. #121460

CRD Validation Rule: skip individual cost limit check for expressions of untouched versions. #121460

jiahuif commented Oct 23, 2023 •

edited

jiahuif commented Oct 23, 2023

cici37 commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

MaryamTavakkoli commented Oct 25, 2023

cici37 commented Oct 25, 2023

cici37 commented Oct 26, 2023

liggitt Oct 27, 2023 •

edited

liggitt Oct 27, 2023 •

edited

jiahuif Oct 27, 2023

jiahuif commented Oct 27, 2023

cici37 commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

liggitt commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

	// If versions[*].{subresources,schema,additionalPrinterColumns} are identical, move to spec
	subresources := out.Versions[0].Subresources
	subresourcesIdentical := true
	validation := out.Versions[0].Schema
	validationIdentical := true
	additionalPrinterColumns := out.Versions[0].AdditionalPrinterColumns
	additionalPrinterColumnsIdentical := true

	// Detect if per-version fields are identical
	for _, v := range out.Versions {
	if subresourcesIdentical && !apiequality.Semantic.DeepEqual(v.Subresources, subresources) {
	subresourcesIdentical = false
	}
	if validationIdentical && !apiequality.Semantic.DeepEqual(v.Schema, validation) {
	validationIdentical = false
	}
	if additionalPrinterColumnsIdentical && !apiequality.Semantic.DeepEqual(v.AdditionalPrinterColumns, additionalPrinterColumns) {
	additionalPrinterColumnsIdentical = false
	}
	}

	// If they are, set the top-level fields and clear the per-version fields
	if subresourcesIdentical {
	out.Subresources = subresources
	}
	if validationIdentical {
	out.Validation = validation
	}
	if additionalPrinterColumnsIdentical {
	out.AdditionalPrinterColumns = additionalPrinterColumns
	}
	for i := range out.Versions {
	if subresourcesIdentical {
	out.Versions[i].Subresources = nil
	}
	if validationIdentical {
	out.Versions[i].Schema = nil
	}
	if additionalPrinterColumnsIdentical {
	out.Versions[i].AdditionalPrinterColumns = nil
	}
	}

CRD Validation Rule: skip individual cost limit check for expressions of untouched versions. #121460

CRD Validation Rule: skip individual cost limit check for expressions of untouched versions. #121460

Conversation

jiahuif commented Oct 23, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

jiahuif commented Oct 23, 2023

cici37 commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

MaryamTavakkoli commented Oct 25, 2023

cici37 commented Oct 25, 2023

cici37 commented Oct 26, 2023

liggitt Oct 27, 2023 • edited

Choose a reason for hiding this comment

liggitt Oct 27, 2023 • edited

Choose a reason for hiding this comment

jiahuif Oct 27, 2023

Choose a reason for hiding this comment

jiahuif commented Oct 27, 2023

cici37 commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

liggitt commented Oct 30, 2023

k8s-ci-robot commented Oct 30, 2023

jiahuif commented Oct 23, 2023 •

edited

liggitt Oct 27, 2023 •

edited

liggitt Oct 27, 2023 •

edited