Define CRD field validation policy

[sbueringer]

As of today we have 95%+ of our field validation in webhook code.

We did this mostly because:

• most of the validation is too complex to be handled in OpenAPI schema
• it's easier to test our webhook code with simple unit tests vs. OpenAPI validation with envtest

This has the following downsides:

• our OpenAPI schema is less expressive, so users and tools can't validate field values based on our CRD schema

I think we need a clear policy going forward about where and how the validation should be done.

Additional context

• Kubernetes 1.25: CustomResourceDefinition Validation Rules Graduate to Beta: with this feature CRD validation became a lot more powerful
  • Disclaimer: CEL in CRDs is a Kubernetes-specific extension. Regular schema validation tools won't be able to run them.

Options

I see at least the following options:

1. All validation should be done in webhooks

• with only very few exceptions like: optional/required, maybe enum

2. Single field validation should be done as much as possible in OpenAPI, everything else in webhooks

• single field validation is validation which only validates a single field individually (e.g. min length, regex, ...) independent of values of other fields

3. Validation should be done as much as possible in OpenAPI, everything else in webhooks

• This includes all validation of 2. plus validation which validates multiple fields together (if field A then field B, field A > field B ...)

My opinion

I personally think 1. is the best option because:

• "OpenAPI schema is not a programming language" (easier to program validation in Go + easier to test)
• OpenAPI validation will always be incomplete, instead of relying on incomplete validation, tools should leverage dry-run to get the "full" validation from our webhooks

/kind documentation

[k8s-ci-robot]

@sbueringer: This issue is currently awaiting triage.

If CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[sbueringer]

cc @CecileRobertMichon @enxebre @fabriziopandini @vincepri

cc @killianmuldoon @JoelSpeed

[killianmuldoon]

Related discussion at #6398 (Maybe should be closed in favor of this one?)

[sbueringer]

Hm damn. Forgot we had the other issue. I'll re-read it when I get to it and then make a suggestion what we should do with it

Thx for bringing up the other issue!!

[JoelSpeed]

"OpenAPI schema is not a programming language" (easier to program validation in Go + easier to test)

The testing side of this could use some expansion IMO. In other projects I work on we have managed to leverage envtest effectively to test validations of the OpenAPI schemas from a user perspective (ie from the YAML representation). We have also used it to test the validations from a Go perspective (because Go serialisation also matters). Yes with the webhooks we can write unit tests for the webhook code, but as this project already uses a lot of envtest for integration tests, I don't see the testing as a blocker for using openapi validations.

OpenAPI validation will always be incomplete, instead of relying on incomplete validation, tools should leverage dry-run to get the "full" validation from our webhooks

Some examples here would be helpful, what validations are we making today that cannot be achieved using CEL now that it's been introduced.

A really good example of where CEL is a clear winner is in the case of a discriminated union. There's a KEP for implementing validation for this for CRDs but that's no implemented yet, in the mean time, the validation of a large discriminated union in Go is ugly (I've tried!), but in CEL, you need one ternary per member field and you're away. The example I've linked is at the struct level because it validates multiple fields within the struct, so this shows how option 3 might look I guess

[fabriziopandini]

/triage accepted
@sbueringer when you have some time please check if we can dedup

[sbueringer]

It's on my list

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[fabriziopandini]

/priority important-longterm

[fabriziopandini]

closing in favour of #6398 which have much more discussions on the same topic
/close

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

closing in favour of #6398 which have much more discussions on the same topic
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.