controller-tools Support more complex validations (oneOf/anyOf/allOf/etc)

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

Open matthchr opened this issue 4 years ago • 45 comments

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?)

Jul 07 '20 00:07 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).

Jul 07 '20 00:07 matthchr

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)?

Jul 14 '20 23:07 DirectXMan12

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.

Jul 15 '20 00:07 matthchr

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.

Jul 20 '20 21:07 christarazi

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

Oct 18 '20 22:10 fejta-bot

/remove-lifecycle stale

Oct 19 '20 00:10 christarazi

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

Nov 04 '20 16:11 s3rj1k

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!

Nov 26 '20 16:11 antonlisovenko

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

Feb 24 '21 17:02 fejta-bot

/remove-lifecycle stale

Feb 24 '21 18:02 s3rj1k

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

Mar 03 '21 20:03 BlaineEXE

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.

Mar 27 '21 14:03 yhrn

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

Jun 25 '21 14:06 fejta-bot

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?

Jun 25 '21 16:06 m-yosefpor

/remove-lifecycle stale

Jun 25 '21 16:06 yhrn

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.

Aug 14 '21 12:08 erikgb

#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).

Sep 20 '21 17:09 adambkaplan

@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....

Sep 21 '21 06:09 erikgb

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

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

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

/lifecycle stale

Dec 20 '21 07:12 k8s-triage-robot

/remove-lifecycle stale

Dec 20 '21 09:12 s3rj1k

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

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

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

/lifecycle stale

Mar 20 '22 10:03 k8s-triage-robot

I still think this is a feature that can be useful for quite a few users in the community! /remove-lifecycle stale

Mar 20 '22 11:03 erikgb

looking forward to this support. Any updates?

May 19 '22 22:05 varks

Any updates?

Probably keep an eye on this KEP:

https://github.com/kubernetes/enhancements/issues/2876

May 20 '22 01:05 cbandy

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

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

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

/lifecycle stale

Aug 18 '22 01:08 k8s-triage-robot

/remove-lifecycle stale

Aug 19 '22 02:08 FillZpp

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

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

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

/lifecycle stale

Nov 17 '22 03:11 k8s-triage-robot

/remove-lifecycle stale

Nov 17 '22 06:11 matthchr

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

Feb 15 '23 07:02 k8s-triage-robot

/remove-lifecycle stale

Feb 15 '23 08:02 erikgb

controller-tools controller-tools copied to clipboard

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

controller-tools
controller-tools copied to clipboard