Add OpenAPI Validation across API types

[killianmuldoon]

Currently validation for our APIs is done, for the most part, in our webhooks. OpenAPI validation could be added (possibly using kubebuilder tags) to make this validation part of our OpenAPI definition. Example validations are max / min, regex matching and enum enforcement - but there's a lot of possibilities enabled by kubebuilder tags.

We currently don't use this type of validation across our API types, but we do have some examples in MachineHealthCheck, MachinePool, ClusterResourceSet and our Bootstrap types.

Putting validation in our OpenAPI specs would move implementation of validation from our webhooks to the Kubernetes tooling (kubectl and/or kube-apiserver). We would still require webhooks for a number of validations which cannot be done with OpenAPI.

Impact
From a UX perspective the change would be, in an ideal scenario, small. We wouldn't be able to customize error messages, but error messages would be more consistent. Users would be able to understand most of our validation by reading the OpenAPI spec instead of reading godoc comments (which may be out of sync with the actual implementation in the webhook).

In reality UX impact could be large and negative because of how defaulting currently works. In our current implementation webhook Defaulting happens before Validation. Users are able to leave fields blank, even though they are required in our validation, and have sane, valid defaults applied.

Currently we have the DefaultValidate test used across our API types which validates this process:

cluster-api/util/defaulting/defaulting.go

Line 37 in 09d0824

func DefaultValidateTest(object DefaultingValidator) func(*testing.T) {

This is used to test: Machines, MachineSets, MachineHealthChecks, MachineDeployments, ClusterResourceSets, KubeadmConfigs, KubeadmConfigTemplates, MachinePools, KubeadmControlPlane, KubeadmControlPlaneTemplate.

If we move validation to OpenAPI validation will happen before webhooks are called which would require changes in how we do defaulting and which fields we expect to be defined by users.

Defaulting could alternatively, in some cases, be done in the OpenAPI schema, but the process doesn't have a good UX right now. There's an issue open about improving this in Kubernetes at kubernetes/kubernetes#108768

This can also cause rollouts in fields that previously weren't defaulted (#6095 h/t @sbueringer ) meaning that a large implementation and testing effort could be needed to ensure the changes aren't causing new rollouts.

If we end up moving only some validation and defaulting to our OpenAPI spec we end up with validation in multiple places. Which doesn't really improve the explicitness of our API but, in the worst case scenario, could be a large implementation effort and have a negative impact on UX.

Original discussion here: #6383 (review)

As the discussion is on an experimetal API we could try to implement OpenAPI validation on that API only to assess the impact and rolling it out to more parts of the overall CAPI API.

Should we try to move more validation to the OpenAPI spec?
@fabriziopandini @JoelSpeed

/kind feature
/area api

[fabriziopandini]

I like the idea of being more explicit in the OpenAPI spec, but TBG I have some concerns about embracing kubebuilder tags for validation because

• Our current test about defaulting, validation and conversion doesn't trigger open API validation, it this could expose us to errors to slip in as already happened in the past
• We will end up managing two set of rules (the one implemented in the OpenAPI spec, the one implemented in webhooks)

Given the above reasons, I think we should not put this discussion in the critical path for #6383 and take some more time to figure out to preserve current test coverage while improving out OpenAPI spec

[enxebre]

Immutability is one important lack in current CRD OpenAPI validation.
Also the fact that any conceptually required field that you want to default needs to be declared optional for client side validation to not complain e.g kubectl, is suboptimal and confusing.

[vincepri]

Also the fact that any conceptually required field that you want to default needs to be declared optional for client side validation to not complain e.g kubectl, is suboptimal and confusing.

A defaulted field is optional from a user perspective as they don't have to specify it, with open api a required field can be defaulted and no error occurs because defaulting happens client-side before hitting the api server

[JoelSpeed]

A defaulted field is optional from a user perspective as they don't have to specify it, with open api a required field can be defaulted and no error occurs because defaulting happens client-side before hitting the api server

I just tested this on K8s 1.23 and I don't think that's correct. I added a required field with a default, left it out of the resource and got an error saying the field was required, if I make it optional, the default applies correctly. Seems the defaulting happens after the validation

[JoelSpeed]

We would still require webhooks for a number of validations which cannot be done with OpenAPI.

Is it possible to enumerate a list of the type of validations we expect to carry over in webhooks, were we to make this change?

If we move validation to OpenAPI validation will happen before webhooks are called which would require changes in how we do defaulting and which fields we expect to be defined by users.

Can you elaborate on this point? Is there conditional defaulting (or other defaulting) happening that cannot be handled by OpenAPI? Keen to know where the gaps are in the use case for Cluster API

This can also cause rollouts in fields that previously weren't defaulted (#6095 h/t @sbueringer ) meaning that a large implementation and testing effort could be needed to ensure the changes aren't causing new rollouts.

Adding or changing the behaviour of a default for a field breaks the assumptions of clients writing and consuming this API, so it's a breaking change. As far as I understand, that doesn't change whether we have OpenAPI defaults/validation vs Webhook defaults/validation, does it?

[killianmuldoon]

Is it possible to enumerate a list of the type of validations we expect to carry over in webhooks, were we to make this change?

I think so, but it's non-trivial IMO. We'd definitely want to do this well before we'd be updating the API.

Can you elaborate on this point? Is there conditional defaulting (or other defaulting) happening that cannot be handled by OpenAPI? Keen to know where the gaps are in the use case for Cluster API

One of the ones that comes to mind is where we copy a value during defaulting:
e.g

cluster-api/api/v1beta1/machine_webhook.go

Line 53 in c029dd2

m.Labels[ClusterLabelName] = m.Spec.ClusterName

There's also some places where we e.g. normalize versions:

cluster-api/exp/api/v1beta1/machinepool_webhook.go

Lines 70 to 74 in e622cbb

	// tolerate version strings without a "v" prefix: prepend it if it's not there.
	if m.Spec.Template.Spec.Version != nil && !strings.HasPrefix(*m.Spec.Template.Spec.Version, "v") {
	normalizedVersion := "v" + *m.Spec.Template.Spec.Version
	m.Spec.Template.Spec.Version = &normalizedVersion
	}

Would we be able to accomplish these with kubebuilder defaulting?

[JoelSpeed]

Would we be able to accomplish these with kubebuilder defaulting?

Conditional stuff like this is out of scope of openapi defaulting

[enxebre]

with open api a required field can be defaulted and no error occurs because defaulting happens client-side before hitting the api server

@vincepri That's actually not true afaik, structural schema CRDs don't publish defaults so client-side can't know what it would be defaulted on the server. This results in a surprising UX for fields that are both required and defaulted failing client side validation if they are not set explicitly.

[sbueringer]

A defaulted field is optional from a user perspective as they don't have to specify it, with open api a required field can be defaulted and no error occurs because defaulting happens client-side before hitting the api server

I just tested this on K8s 1.23 and I don't think that's correct. I added a required field with a default, left it out of the resource and got an error saying the field was required, if I make it optional, the default applies correctly. Seems the defaulting happens after the validation

Joel and further down Alberto are correct. kubectl only does OpenAPI validation but no defaulting. I opened an issue for this in k/k: kubernetes/kubernetes#108768.

But in my opinion this is a mostly separate discussion. Independent of if we add more OpenAPI validation or not we will have optional and required fields like today and this will be an issue until it is resolved in k/k. Of course by adding more OpenAPI validation we could theoretically hit this case more often, but I think only in rare edge cases (where the defaulting webhook "fixes up" field values, e.g. adding the v prefix on versions). The most common case of this issue is optional vs required and for those cases we already have OpenAPI validation today on required fields.

So for me it comes down to:

• adding more OpenAPI validation to the API types to make restrictions/validations more obvious when looking at the CRD / OpenAPI schema vs.
• keeping it as is and basically doing almost all validations in validation webhooks (except some basic validations we get out of the box like optional/required)

From a maintainer perspective I definitely prefer the validation in code as:

• it's just a lot more straightforward to unit test (I'm aware we could unit test OpenAPI validation via envtest, but it is slower)
• it keeps the entire validation logic in one place and thus the entire validation logic is easier to comprehend (while of course the simple OpenAPI validations we could add are less obvious as folks have to look at the code)

From a user perspective it might be better to surface the validations we could express via OpenAPI schema in the OpenAPI schema of the CRDs directly, although this doesn't provide a full picture so it might be misleading. I wonder if having a good description would be better.

Is it possible to enumerate a list of the type of validations we expect to carry over in webhooks, were we to make this change?

A few examples of validations we cannot move to OpenAPI:

• KCP validates that certain fields are immutable
• ClusterClass does a lot of advanced defaulting/validation. tl;dr is you can define OpenAPI schemas for variables in a ClusterClass and they are then defaulted and validated in the Cluster webhook. Links:
  • https://cluster-api.sigs.k8s.io/tasks/experimental-features/cluster-class/write-clusterclass.html#clusterclass-with-patches
  • https://github.com/kubernetes-sigs/cluster-api/blob/7726885336063ce3e74df26e4e6f114643f0e5e9/internal/webhooks/clusterclass.go
• Validations which check if the combination of values for multiple fields are correct:

  cluster-api/bootstrap/kubeadm/api/v1beta1/kubeadmconfig_webhook.go

  Lines 123 to 154 in ffa4348

  	if file.ContentFrom != nil {
  	if file.ContentFrom.Secret.Name == "" {
  	allErrs = append(
  	allErrs,
  	field.Required(
  	pathPrefix.Child("files").Index(i).Child("contentFrom", "secret", "name"),
  	missingSecretNameMsg,
  	),
  	)
  	}
  	if file.ContentFrom.Secret.Key == "" {
  	allErrs = append(
  	allErrs,
  	field.Required(
  	pathPrefix.Child("files").Index(i).Child("contentFrom", "secret", "key"),
  	missingSecretKeyMsg,
  	),
  	)
  	}
  	}
  	_, conflict := knownPaths[file.Path]
  	if conflict {
  	allErrs = append(
  	allErrs,
  	field.Invalid(
  	pathPrefix.Child("files").Index(i).Child("path"),
  	file,
  	pathConflictMsg,
  	),
  	)
  	}
  	knownPaths[file.Path] = struct{}{}

• Similar case: validations which ensure that the namespace in ownerRefs is the same as in the top-level resource
• Validation which checks if a label selector is valid

I'm aware that there is some ongoing work on an expression language for validation in CRDs which is really nice, but this would only work on newer Kubernetes versions which we cannot assume for a very long time (link: kubernetes/enhancements#2876)

I think there are a lot more cases and looking through our webhook code I'm not sure we could replace a significant percentage through OpenAPI validation.

[JoelSpeed]

@sbueringer thanks for taking the time to think about this, that's a very thorough write up

I just wanted to add some extra context for completeness to your list of things we cannot move.

• Immutability is on the backlog for CRDs WIP: Introduce support for Immutable fields in CRDs kubernetes/kubernetes#83743. I've spoken to a few of the API SMEs about this and the upshot was to wait for the SSA stuff to stabilise to avoid too many changes at once, but, in theory this feature is coming at some point

• Validations which check if the combination of values for multiple fields are correct

  I believe this one can be solved with the CEL when that eventually lands right? (I know you've mentioned that this is a long time away but I wanted to add that context assuming I'm correct)

• Similar case: validations which ensure that the namespace in ownerRefs is the same as in the top-level resource

  I don't think this is correct, there is no namespace field in an owner reference, that was a deliberate API design decision when the feature was introduced

[sbueringer]

I believe this one can be solved with the CEL when that eventually lands right? (I know you've mentioned that this is a long time away but I wanted to add that context assuming I'm correct)

I think you're correct, a lot of of those should be implementable with CEL, not sure if we have something which is too complex for that.

I don't think this is correct, there is no namespace field in an owner reference, that was a deliberate API design decision when the feature was introduced

My bad, I meant ObjectReference instead of OwnerRefs (e.g. https://github.com/kubernetes-sigs/cluster-api/blob/main/api/v1beta1/clusterclass_types.go#L445-L449 but also at least in MD, MS, Cluster). This might be solved by stop using the not-recommended ObjectReference upstream type and using our own one with only the useful fields instead. In cases were namespace must be always the same as the one of the top-level API type it doesn't make sense to have it configurable. (xref: #6024)