Support more complex validations (oneOf/anyOf/allOf/etc)

[matthchr]

According to the structural schema, CRD schemas such as the one below (from the linked article) are allowed:

type: object
properties:
  spec:
    type: object
    properties
      command:
        type: string
        minLength: 1                          # value validation
      shell:
        type: string
        minLength: 1                          # value validation
      machines:
        type: array
        items:
          type: string
          pattern: “^[a-z0-9]+(-[a-z0-9]+)*$” # value validation
    oneOf:                                    # value validation
    - required: [“command”]                   # value validation
    - required: [“shell”]                     # value validation

As far as I can tell, it is not possible to generate a schema of this shape using controller-gen today. Specifically, the oneOf validation at the bottom cannot be represented using Kubebuilders validation annotations, as there is no way to specify oneOf (or anyOf/allOf/etc) via annotations that I can tell.

Is there a plan or vision for having controller-gen support more complex validation scenarios (or does it already and I've just missed how?)

[matthchr]

There is obviously a good amount of complexity in even supporting this scenario, as in order to generate a CRD with the above shape, there needs to be some way to create various validation "groups" and join them with an operator... maybe something like this...?

type Demo struct {
  // +kubebuilder:validation:MinLength=1
  // +kubebuilder:validation:OneOf[0]:Required
  command string
  // +kubebuilder:validation:MinLength=1
  // +kubebuilder:validation:OneOf[1]:Required
  shell string
  // +kubebuilder:validation:Pattern="^[a-z0-9]+(-[a-z0-9]+)*$"
  machines []string
}

I'm wondering if there's thought/energy in this space - I looked around and didn't really see many issues related to this topic (although there was #298 which is somewhat related).

[DirectXMan12]

I think #298 is the bulk of the existing discussion.

Largely, the background here is that we wanted to make it really hard/impossible to produce non-structural schemas (considering they're "invalid" in the soft sense -- you can't use new features with non-structural schemas).

I think, at the very least, we can extend/augment the union markers to generate validation, so that the patterns captured by x-kubernetes-union and such get converted to equivalent validation. It's a little stricter than general oneOf, but it's also in line with the general patterns of kubernetes.

To clarify from your usecase a bit further, what's the desired intent of the validation? Specifically, are you saying that:

• you must specify either command or shell, or both (not a thing discussed in kubernetes up to this point, I think)?
• you must specify either command or shell, but not both (an "embedded union (no tag)", in kubernetes terms, I believe)?

[matthchr]

The above was just an example honestly, speaking about a general problem which I think will solve my specific problem. It's possible I generalized a bit too soon though as you're right the case above is really a union case (and there are specific recommendations from Kubernetes on how to deal with those based on this enhancement proposal for unions)

My actual specific use case is: I want to create a CRD where there are two distinct sets of required parameters. For example something like this dummy CRD for a SQL database in a cloud:

type: object
properties:
  spec:
    type: object
    properties
      cloudName:
        type: string
        minLength: 1
      tier:
        type: string
        minLength: 1
      otherProperty1:
        type: string
      ... more properties, elided for brevity ...
    anyOf:
    - required: [“cloudName”]
    - required: [“cloudName, "tier", "otherProperty1", (and so on)]

I'm trying to convey that there are 2 (or 3 or whatever) minimum property sets for this resource. Structurally the object is the same regardless though (so it adheres to the structural schema requirements).

Ideally I could encode these requirements into the CRD itself, so that users land into one of the two supported "flavors".

Drawing parallels to code I want two different constructors/factory methods for this CRD type.

I know there are probably other solutions to accomplishing this goal (like putting the delta between each set of required properties into their own struct and marking them all required but the top level struct optional on the spec), but that forces me to increase the complexity of the resource by adding nesting, and especially for the common case I'm thinking of (one set is always just cloudName, the other set is cloudName + a bunch of properties) it would force most of the properties down a level since the delta between each set of required properties is large. Alternatively I could go full union-style and actually make two shapes but again that ends up complicating the structure in a way that isn't technically required by the YAML.

[christarazi]

This would also be helpful over at the Cilium project as well. Our use-case is even simpler:

type CiliumClusterwideNetworkPolicy struct {
        ...

	// Spec is the desired Cilium specific rule specification.
	Spec *api.Rule `json:"spec,omitempty"`

	// Status is the status of the Cilium policy rule.
	Status CiliumNetworkPolicyStatus `json:"status"`
}

type Rule struct {
	// EndpointSelector selects all endpoints which should be subject to
	// this rule. EndpointSelector and NodeSelector cannot be both empty and
	// are mutually exclusive.
	EndpointSelector EndpointSelector `json:"endpointSelector,omitempty"`

	// NodeSelector selects all nodes which should be subject to this rule.
	// EndpointSelector and NodeSelector cannot be both empty and are mutually
	// exclusive. Can only be used in CiliumClusterwideNetworkPolicies.
	NodeSelector EndpointSelector `json:"nodeSelector,omitempty"`
        ...
}

Essentially, we'd like to say that only one of endpoint selector or node selector must be provided, but not both.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[christarazi]

/remove-lifecycle stale

[s3rj1k]

Any news about this?
Would be super helpful to get this feature.

[antonlisovenko]

I'd say the one of required is a quite common use case...

oneOf:
            - required: [x]
            - required: [y]
            - required: [z]

Would be great to get controller-gen support for this!

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[s3rj1k]

/remove-lifecycle stale

[BlaineEXE]

This is also something that would be useful in the Rook (github.com/rook/rook) project.

[yhrn]

I can't find any good documentation for x-kubernetes-union but I'm guessing this would be super useful for us. Our validating web hooks are 80% checking that exactly one out of two or sometimes three fields are set.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[m-yosefpor]

I need to define the a Host field which can be either IPv4 or Hostname. However the following code will overwrite and I can not specify multiple formats:

	// +kubebuilder:validation:Format:="hostname"
	// +kubebuilder:validation:Format:="ipv4"
	Host string `json:"host"`

I suppose the only way to specify this is with the help of oneOf described in this issue. Am I right?

[yhrn]

/remove-lifecycle stale

[erikgb]

Updated link to relevant KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1027-api-unions/README.md. Links to it are broken in older comments, and also in #298.

[adambkaplan]

#298 has been closed as it was a stale/rotten PR. Do we have any idea how someone could implement oneOf validation (this seems to be the biggest use case that is missing in controller-tools).

[erikgb]

@adambkaplan IMO this should be implemented on the basis of the the union KEP. I had a chat with @apelisse on Slack regarding this, and the advice was to wait till the KEP was implemented for native/in-tree types. But I would be happy to take a look at implementing this here, but my bandwidth is limited, as I guess it is for everyone. 😄 A couple of blocking technical PRs (testdata regeneration) got merged yesterday....

[apelisse]

I wouldn't hold my breath. The feature is not in 1.27, nor is it really planned for a future release. I'm not sure when we'll find the time to work on it.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[erikgb]

/remove-lifecycle stale

Still something we would like to see, but the upstream union design seem to be blocked.

[jpbetz]

Gateway API appears to need support for this very simple use case:

type: string
oneOf:
  - format: ipv4
  - format: ipv6

xref: https://github.com/kubernetes-sigs/gateway-api/pull/2246/files#r1276849955

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[erikgb]

This is still highly relevant.

/remove-lifecycle stale

[MikeSpreitzer]

The discussion around this seems too narrowly focused on unions. I read https://kubernetes.io/blog/2019/06/20/crd-structural-schema/ and its definition of "structural schema" seems like an overreaction to the stated problem, which is not declaring all the content. The opening comment in this issue has a good example. By conjoining "properties" with "oneOf", it declares all the content (the "properties" does that). Conjoining "properties" with "allOf" or "anyOf" would also do that --- without looking like a union.
@sttts: ^^^

[sttts]

@MikeSpreitzer not exactly sure I understand your comment right. This issue here is purely about controller-tools not being able to express everything that is valid in structural schemas. The oneOf example is totally fine in CRDs, just needs support in controller-tools via some marker. Unions are just one example, there are many more.

Side-note: probably, with CEL it is already possible to express a union through controller-tools markers. So this issue here is not a blocker for unions anymore.

[JoelSpeed]

We have used CEL to validate unions for both optional and required union members. 

In the example below we have a field Type that represents the discriminator, which is validated as a Kubebuilder Enum, and then optional and required fields can be validated using a ternary for each member. Ternary examples below:

// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'RequiredMember' ?  has(self.requiredMember) : !has(self.requiredMember)",message="requiredMember is required when type is RequiredMember, and forbidden otherwise"
// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'OptionalMember' ?  true : !has(self.optionalMember)",message="optionalMember is forbidden when type is not OptionalMember"

[sbueringer]

Largely, the background here is that we wanted to make it really hard/impossible to produce non-structural schemas

Probably obvious, but I think this part of the problem could be covered by running the resulting CRD through structural schema validation.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[theunrepentantgeek]

/remove-lifecycle stale

[sbueringer]

Generally makes sense

/lifecycle frozen
/help

[k8s-ci-robot]

@sbueringer:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

Generally makes sense

/lifecycle frozen
/help

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.