🐛 Fix XValidations flattening

[cezarsa]

Fixes #997

This PR ensures XValidations are merged correctly when flattening the CRD schema.

[k8s-ci-robot]

Welcome @cezarsa!

It looks like this is your first PR to kubernetes-sigs/controller-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @cezarsa. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

[sbueringer]

/ok-to-test

[JoelSpeed]

Do we need to consider the ordering in flattening? CRD validations are executed in order so do we think the field level validations or the struct level validations should come first? What will happen with this approach, I assume the output here is deterministic?

[cezarsa]

What will happen with this approach, I assume the output here is deterministic?

It is deterministic and, with the current approach, struct-level validations will come first. This kinda makes sense to me but I don't have a definitive answer if this is the intended behavior or if this has been discussed before.

Ultimately, I don't think the order can affect the correctness or the cost budget for the validations as they are all executed even after a failed rule.

[JoelSpeed]

Ultimately, I don't think the order can affect the correctness or the cost budget for the validations as they are all executed even after a failed rule.

Is that true? I was under the impression that if a rule failed, further rules were not executed until the earlier rule passed?

Either way, I guess that doesn't hugely matter since all rules must pass for the object to be accepted. I would be tempted to poke some of the API machinery folks though just in case they can foresee any issues here/have a suggestion for prioritisation

[cezarsa]

Is that true? I was under the impression that if a rule failed, further rules were not executed until the earlier rule passed?

Yes, I just tested it to be sure, using controller-gen from this branch and with an object declared as:

type FooSpec struct {
	// +kubebuilder:validation:XValidation:rule="false",message="validation 3"
	// +kubebuilder:validation:XValidation:rule="false",message="validation 4"
	Thing *Thing `json:"thing"`
}

// +kubebuilder:validation:XValidation:rule="false",message="validation 1"
// +kubebuilder:validation:XValidation:rule="false",message="validation 2"
type Thing struct {
	Field *string `json:"field"`
}

Applying it returned all the validation errors:

$  k apply -f ./example.yaml
The Foo "foo1" is invalid:
* spec.thing: Invalid value: "object": validation 1
* spec.thing: Invalid value: "object": validation 2
* spec.thing: Invalid value: "object": validation 3
* spec.thing: Invalid value: "object": validation 4

[sbueringer]

I think for cost estimation / compile checks at CRD create/update all rules are compiled and afterwards error messages are produced accordingly: https://github.com/kubernetes/kubernetes/blob/d081fd56a2aba4663f11dd5e27dbcf64c0ec6638/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/validation/validation.go#L1226

During CR create/update:

• I"m not sure if having the allOf or not makes any difference: https://github.com/kubernetes/kubernetes/blob/b34197dc14aecdd1da1c70b979d1b304f522cc30/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go#L305-L318
  • Still agree though that not adding an unnecessary allOf is preferable
• It looks to me like rule evaluation will stop once the budget limit is hit: https://github.com/kubernetes/kubernetes/blob/b34197dc14aecdd1da1c70b979d1b304f522cc30/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/schema/cel/validation.go#L422
  • But I think as long as the order is the same, it doesn't matter if the rules are in one x-validations element or separate in multiple below an allOf

@cezarsa @JoelSpeed WDYT? does that make sense?

(feel free to cc someone from api-machinery, not sure who :))

[cezarsa]

I'm not sure if having the allOf or not makes any difference:

Having allOf on validations in the CRD actually causes the apiserver to reject it. I tried applying the example CRD from #997 on a k8s v1.28.9 API and it failed with:

$ kubectl apply -f ./crd/validation.bug_foos.yaml
The CustomResourceDefinition "foos.validation.bug" is invalid:
* spec.validation.openAPIV3Schema.properties[spec].properties[thing].allOf[0].x-kubernetes-validations: Forbidden: must be empty to be structural
* spec.validation.openAPIV3Schema.properties[spec].properties[thing].allOf[1].x-kubernetes-validations: Forbidden: must be empty to be structural

[jpbetz]

I'm not sure if having the allOf or not makes any difference:

Having allOf on validations in the CRD actually causes the apiserver to reject it. I tried applying the example CRD from #997 on a k8s v1.28.9 API and it failed with:

$ kubectl apply -f ./crd/validation.bug_foos.yaml
The CustomResourceDefinition "foos.validation.bug" is invalid:
* spec.validation.openAPIV3Schema.properties[spec].properties[thing].allOf[0].x-kubernetes-validations: Forbidden: must be empty to be structural
* spec.validation.openAPIV3Schema.properties[spec].properties[thing].allOf[1].x-kubernetes-validations: Forbidden: must be empty to be structural

kubernetes/kubernetes#124381 was merged into master quite recently. Does it resolve this problem?

[cezarsa]

kubernetes/kubernetes#124381 was merged into master quite recently. Does it resolve this problem?

Hey @jpbetz, thanks I wasn't aware of that PR but apparently it doesn't solve the problem.
I've just compiled apiserver from source and still see the same error. Repro steps:

1. Clone https://github.com/kubernetes/kubernetes

2.  cd kubernetes
    make kube-apiserver kubectl
    ./hack/install-etcd.sh
    cp ./third_party/etcd/etcd _output/bin/
   

3. Clone https://github.com/cezarsa/validationbug

4.  cd validationbug
    KUBEBUILDER_ASSETS=../kubernetes/_output/bin go test
   

The test above tries to apply a CRD with x-kubernetes-validations inside a all_of block but it failed with:

--- FAIL: TestFunc (6.45s)
    main_test.go:16: unable to install CRDs onto control plane: unable to create CRD instances: unable to create CRD "foos.validation.bug": CustomResourceDefinition.apiextensions.k8s.io "foos.validation.bug" is invalid: [spec.validation.openAPIV3Schema.properties[spec].properties[thing].allOf[0].x-kubernetes-validations: Forbidden: must be empty to be structural, spec.validation.openAPIV3Schema.properties[spec].properties[thing].allOf[1].x-kubernetes-validations: Forbidden: must be empty to be structural]
FAIL
exit status 1
FAIL	github.com/cezarsa/validationbug	7.120s

[sbueringer]

I would flip the order here. I think it's better if the validation from the field is before the ones from the type.

Basically this allows to add validations add the start of the validation array when importing external types (~ that would prioritize "local" validations over imported ones)

[cezarsa]

Order flipped in 43d3d3a.

[sbueringer]

@cezarsa Sorry for the delay. Last comment from my side: #998 (comment)

I think even if the current output would be somehow valid (which it doesn't seem to be), it still makes sense to flatten in this case

[sbueringer]

/ok-to-test

[sbueringer]

Thank you very much!

/lgtm
/approve
/ok-to-test

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 41fdb218319688b4ade7972c98782bd6a77b78d0

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarsa, sbueringer

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:

• OWNERS [sbueringer]

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