Skip schemas that don't have CEL rules in NewValidator

[jpbetz]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #111158 (comment)

Structural schemas are considered valid even if the root object does not explicitly set Type: "Object". This wasn't noticed until the feature gate was enabled by default for CEL and k8s.io/kubernetes/test/integration/apiserver/apply: TestApplyCRDUnhandledSchema started failing consistently because it tests this specific case.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

[jpbetz]

/sig api-machinery
/triage accepted
/priority important-soon

cc @liggitt @cici37

[cici37]

Found the issue in

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go

Line 69 in a4a22a2

return validator(s, true, model.SchemaDeclType(s, isResourceRoot), perCallLimit)

Opened a pr against your branch: jpbetz#3

[cici37]

Notes:
We have existing validation and test coverage for type required in root level:

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

Lines 121 to 122 in a4a22a2

	case rootLevel:
	allErrs = append(allErrs, field.Required(fldPath.Child("type"), "must not be empty at the root"))

And root level type must be object:

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

Lines 133 to 135 in a4a22a2

	if lvl == rootLevel && len(s.Type) > 0 && s.Type != "object" {
	allErrs = append(allErrs, field.Invalid(fldPath.Child("type"), s.Type, "must be object at the root"))
	}

The failing test is to test "type: array" without any items at all is completely valid:

kubernetes/test/integration/apiserver/apply/apply_crd_test.go

Lines 489 to 492 in a4a22a2

	"openAPIV3Schema": {
	"properties": {
	"TypeFooBar": {
	"type": "array"

So how about instead of adding type missing test case which ideally will not be reached, we could directly fix the currently integration test. The proposed PR: #111504

[jpbetz]

The failing test is to test "type: array" without any items at all is completely valid:

kubernetes/test/integration/apiserver/apply/apply_crd_test.go

Lines 489 to 492 in a4a22a2

	"openAPIV3Schema": {
	"properties": {
	"TypeFooBar": {
	"type": "array"

The problem here is that the root schema has properties but no type, it it had a type it would look like:

"openAPIV3Schema": { 
 "type": "object",
  "properties": { 
    "TypeFooBar": { 
      "type": "array" 

[cici37]

Thanks for addressing it. I agree we should add handle when type is empty(ideally it should not be hit though). Our existing validation process do validating if items value is missing when type=array or type missing which are bypassed by this integration test since it's calling etcdclient.Put directly(That is why it's been hit).

The problem here is that the root schema has properties but no type, it it had a type it would look like:

"openAPIV3Schema": { 
 "type": "object",
  "properties": { 
    "TypeFooBar": { 
      "type": "array" 

[jpbetz]

this integration test since it's calling etcdclient.Put directly(That is why it's been hit).

Good eye. Okay, I feel slightly better about this not being a general class of problems now. Thanks for digging in!

[liggitt]

this integration test since it's calling etcdclient.Put directly(That is why it's been hit).

Good eye. Okay, I feel slightly better about this not being a general class of problems now. Thanks for digging in!

That is simulating CRDs created via v1beta1, which allowed missing type fields. Those CRDs can be updated via v1, so it's still a scenario that can be encountered.

[jpbetz]

That is simulating CRDs created via v1beta1, which allowed missing type fields. Those CRDs can be updated via v1, so it's still a scenario that can be encountered.

Oh right. Okay, so our options are:

1. Do another structural schema validation check when loading CRDs from storage and if it fails, don't add a CEL validator at all, eliminating the need to support v1beta1 schemas (which can't have CEL rules anyway)
2. Try to harden the code against all valid v1beta1 OpenAPIv3 schemas

(1) would need to be handled both for validation rule registration and default checking. (2) requires test coverage for all the valid v1beta1 schema differences

I'm inclined to work toward (1). Any thoughts?

[liggitt]

• eliminating the need to support v1beta1 schemas (which can't have CEL rules anyway)

the scenario is:

1. create in antiquity past via v1beta1
2. fixup enough to pass structural validation and add cel rules via v1

am I reading correctly that there are things that pass structural validation that CEL still chokes on?

[jpbetz]

the scenario is:

1. create in antiquity past via v1beta1
2. fixup enough to pass structural validation and add cel rules via v1

am I reading correctly that there are things that pass structural validation that CEL still chokes on?

No, CEL is only choking on things that fail structural validation. I've rewritten this fix to do another structural validation check whenever constructing a cel.NewValidator, we already have the structural validation check check on the CRD create/update operations, but when loading CRDs from disk and preparing for CR handling we didn't do another structural schema validation check, so this PR now adds that.

[jpbetz]

I've manually verified that TestApplyCRDUnhandledSchema passes with this fix with the feature gate enabled.

[liggitt]

1. When writing a CRD, do we ensure any CRD that contains CEL rules passes structural validation?
2. Do we limit ourselves to calling cel.NewValidator to CRDs that contain CEL rules?

If both 1 and 2 are true, I'm not sure we need the additional structural validation check

[cici37]

For 1, yes. Though we do all the validation(structural + cel validation) and save all possible errs back at once. Later we could possible improve it by skipping cel check if other validation err already identified(especially type err).
For 2, yes we use cel.NewValidator to validation cel rules.
I agree with Jordan on don't need another structural schema validation check for CR handling.

1. When writing a CRD, do we ensure any CRD that contains CEL rules passes structural validation?
2. Do we limit ourselves to calling cel.NewValidator to CRDs that contain CEL rules?

If both 1 and 2 are true, I'm not sure we need the additional structural validation check

[jpbetz]

1. When writing a CRD, do we ensure any CRD that contains CEL rules passes structural validation?
2. Do we limit ourselves to calling cel.NewValidator to CRDs that contain CEL rules?

If both 1 and 2 are true, I'm not sure we need the additional structural validation check

(2) is false, currently at least. It would be most optimal for for CRDs with no CEL rules to have a dedicated routine that checks for validation rules. cel.NewValidator COULD be modified to check as it goes, but that would be a more involved code change that would only benefitt CRDs with CEL rules, and I suspect by not all that much.

[jpbetz]

Switched this to use a "does the structural schema contain cel validation rules?" check.

[jpbetz]

I've also opened #111519 to guard the validation codepath. I started looking at it more closely because of this PR. I had been planning to do a change in that code to hide CEL errors if there are schema errors anyway..

[liggitt]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, 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/OWNERS [liggitt]

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

[jpbetz]

/retest

[cici37]

/test pull-kubernetes-e2e-gce-ubuntu-containerd