Invalid CRD generated when XValidation is used on both field and struct

[cezarsa]

When the XValidation marker is used on both a field and the type for that field the generated CRD is invalid because it tries to use an allOf property to merge the validations.

For example, given the definition of the following structs:

type FooSpec struct {
	// +kubebuilder:validation:XValidation:rule="size(self.field) > 2",message="validation 1"
	Thing Thing `json:"thing"`
}

// +kubebuilder:validation:XValidation:rule="has(self.field)",message="validation 2"
type Thing struct {
	Field *string `json:"field"`
}

The generated CRD will include:

properties:
  thing:
    allOf:
    - x-kubernetes-validations:
      - message: validation 2
        rule: has(self.field)
    - x-kubernetes-validations:
      - message: validation 1
        rule: size(self.field) > 2
    properties:
      field:
        type: string
    required:
    - field
    type: object

Where the expected result would have been:

properties:
  thing:
    x-kubernetes-validations:
    - message: validation 2
      rule: has(self.field)
    - message: validation 1
      rule: size(self.field) > 2
    properties:
      field:
        type: string
    required:
    - field
    type: object

I have a full reproduction of the bug including the invalid CRDs on https://github.com/cezarsa/validationbug/blob/a8148487551b92b4f3b6669cde9c4050e35cb98d/main.go.

[JoelSpeed]

@jpbetz @cici37 Could you comment here whether you can foresee any issues with the way the allOf is being used presently with the CEL rules? In theory, as long as they are all executed it should behave as expected, but we were concerned whether this might break the cost estimation in some regard?

See also discussion on the fix PR, esp from @sbueringer

[jpbetz]

Joe Betz Cici Huang Could you comment here whether you can foresee any issues with the way the allOf is being used presently with the CEL rules? In theory, as long as they are all executed it should behave as expected, but we were concerned whether this might break the cost estimation in some regard?

See also discussion on the fix PR, esp from Stefan Büringer

Does kubernetes/kubernetes#124381 resolve this? I posed it over on the controller-tools discussion as well.