Allow `AtMostOneOf`/`ExactlyOneOf`/`AtLeastOneOf` markers to emit OpenAPI `oneOf`/`anyOf` instead of CEL

[HadrienPatte]

This issue is a followup on #1212 (comment):

So the OpenAPI based OneOf/AnyOf constraints would be a good alternative despite the unfriendly error message.

Yeah so perhaps we take that route of a field on the marker useOpenAPIValidation=true or something like that. @\alvaroaleman any opinions?

With deeply nested types containing arrays, I have seen CEL break down. In most cases with newer k8s versions, MaxItems should suffice

Yeah I guess when adding a compatible to change to an existing API, the argument for bounding all parents can be more difficult

PR #1212 added AtMostOneOf, ExactlyOneOf, and AtLeastOneOf markers, implemented with x-kubernetes-validations (CEL) for better error messages. An option to have those generate OpenAPI oneOf/anyOf would allow avoiding CEL budget limitations.

Context

In the cilium project, we have the CiliumNetworkPolicy CRD that uses oneOf-style validation in several places. One of the most problematic ones for the CEL implementation is CIDRRule, which sits inside the ingress[]/egress[]/ingressDeny[]/egressDeny[] rule lists:

// +kubebuilder:validation:ExactlyOneOf=cidr;cidrGroupRef;cidrGroupSelector
type CIDRRule struct {
    Cidr              CIDR             `json:"cidr,omitempty"`
    CIDRGroupRef      CIDRGroupRef     `json:"cidrGroupRef,omitempty"`
    CIDRGroupSelector EndpointSelector `json:"cidrGroupSelector,omitzero"`
    // ...
}

type IngressRule struct {
    // ...
    FromCIDRSet []CIDRRule `json:"fromCIDRSet,omitzero"`
}

type Rule struct {
    Ingress     []IngressRule     `json:"ingress,omitempty"`
    IngressDeny []IngressDenyRule `json:"ingressDeny,omitempty"`
    Egress      []EgressRule      `json:"egress,omitempty"`
    EgressDeny  []EgressDenyRule  `json:"egressDeny,omitempty"`
    // ...
}

The generated CEL rule on CIDRRule is fine in isolation. But because CIDRRule is reachable through several layers of arrays, the rule is reapplied at every nesting level, so 8 separate sites in the schema each carry a copy of the same rule. The resulting CRD is rejected by the API server:

CustomResourceDefinition.apiextensions.k8s.io "ciliumnetworkpolicies.cilium.io" is invalid:
  spec.validation.openAPIV3Schema.properties[spec].properties[ingress].items.properties[fromCIDRSet].items.x-kubernetes-validations[0].rule:
    Forbidden: estimated rule cost exceeds budget by factor of 7.1x
  ...
  spec.validation.openAPIV3Schema:
    Forbidden: x-kubernetes-validations estimated rule cost total for entire OpenAPIv3 schema exceeds budget by factor of 12.7x

We did try optimizing the CEL expressions themselves (#1408) which managed to get each individual rule under budget, but the overall CRD cost still remained over budget.

We also looked at setting MaxItems to bound the array's costs, but in order for MaxItems to be effective, every single array in the chain needs to have it set and on a widely adopted stable (v2) CRD like the CiliumNetworkPolicy, we can't afford to add such constraints which would be a breaking change for users, just to get some validation.

I don't think cilium is the only project with this pattern of "polymorphic union under a list". Having the option to use openAPI validation instead of CEL for those cases would be beneficial. The cost would be a slightly worse error message for users but it's better than no validation at all.

Proposal

We'd be highly interested in having a way to generate openAPI validation for oneOf markers as was initially suggested in #1212:

// +kubebuilder:validation:ExactlyOneOf=cidr;cidrGroupRef;cidrGroupSelector,useOpenAPIValidation=true

When set, the marker would emit openAPI oneOf/anyOf/allOf instead of x-kubernetes-validations validation. The default behavior would remain CEL so existing users' error messages don't regress.

If controller-tools maintainers are open to this proposal, we'd be happy to contribute it (the actual implementation actually already exists as the first version of #1212 before it changed trajectory to use CEL instead of openAPI).

[JoelSpeed]

IMO, as long as the openapi option is the fallback and is explicit opt-in, that seems fine to me

Out of interest, I appreciate you have many levels of validation to contend with on your API, did you do any research as to what kind of length limits you would need to apply to ingress[]/egress[]/ingressDeny[]/egressDeny[] to be able to get the budget under control?

[squeed]

did you do any research as to what kind of length limits you would need to apply

I did. Because the API is deeply-nested, and the k8s cost estimator is quite pessimistic, many of the limits needed to be around ~8. This is far too limiting, especially for a large user base. If we ever spin a v3 version of the API, I'll try and flatten the design as much as possible. (This has ergonomic effects too. Multiple nested arrays in YAML are a usability nightmare.)

Of course, end-users don't actually create 8 * 8 * 8 * 8 entries. Rather, they create 16*1 entries. But the cost estimator doesn't know that.