✨ Support for oneOf Groups for Discriminate Types

[mmellison]

These changes add support for the field marker: kubebuilder:validation:OneOf which will generate a oneOf block in the CRD structural definition[1]. This could be expanded in the future to include more complex oneOf objects, however for now, it works to simply define fields which are mutually exclusive:

// Discriminator Test Spec
type TestSpec struct {

	// +kubebuilder:validation:OneOf
	Cat map[string]string `json:"cat,omitempty"`

	// +kubebuilder:validation:OneOf
	Dog map[string]string `json:"dog,omitempty"`
}

becomes

        spec:
          description: Discriminator Test Spec
          oneOf:
          - properties:
              cat: {}
            required:
            - cat
          - properties:
              dog: {}
            required:
            - dog
          properties:
            cat:
              additionalProperties:
                type: string
              type: object
            dog:
              additionalProperties:
                type: string
              type: object
          type: object

This then allows the CRD to enforce a mutually exclusive type:

apiVersion: "inflow.arroyo.io/v1"
kind: Test
metadata:
  name: test-both
spec:
  cat: {"says": "meow"}
  dog: {"says": "woof"}

produces "spec" must validate one and only one schema (oneOf). Found 2 valid alternatives

apiVersion: "inflow.arroyo.io/v1"
kind: Test
metadata:
  name: test-none
spec: {}

produces "spec" must validate one and only one schema (oneOf). Found none valid

---

[1] https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/

[k8s-ci-robot]

Welcome @seglberg!

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]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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/test-infra repository. I understand the commands that are listed here.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: seglberg
To complete the pull request process, please assign pwittrock
You can assign the PR to them by writing /assign @pwittrock in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[DirectXMan12]

/hold

a) you'll need to sign the CLA

b) I'd like to see a more comprehensive design for this

c) this might be a foot-gun considering some uses of one-of could produce "non-structural" schemata (https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#specifying-a-structural-schema)

d) we generally discourage this pattern, preferring unions instead (we'll need to support the x-kubernetes-union tag, though)

[mmellison]

I'll gladly sign the CLA if this ends up going anywhere significant since it sounds like I may have gone down the wrong road here.

I didn't seem to come across any concepts of a Union type when digging at the current sources. I had been modeling my APIs after Kubernetes 1.15 types such as VolumeSource. Is there more information or docs you could point me towards so that I may formulate a more comprehensive design for this?

[mmellison]

Ah. So I just ran across https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190425-structural-openapi.md

I'm still getting my bearing around the Kubernetes project structure... but it looks like the Union extensions have yet to land?

[DirectXMan12]

The union thing is a new feature that codifies a pattern that we've had for a while. And yeah, it's hard to navigate those some times.

union appears to have half-landed, but I think we can produce the equivalent openapi off of the standard union markers introduced in the union kep linked in that doc (+union, +unionDeprecated, and +unionDiscriminator)

@sttts

[DirectXMan12]

also @apelisse @jennybuckley might know more about the state of the union bits, but I don't see XUnion as part of the JSON schema types yet, but it's in kube-openapi already.

[mmellison]

Producing (or "unfolding" as one of the docs put it) the equivalent openapi schema shouldn't be too difficult until there is full support for the extension. That begs the question. In the example:

x-kubernetes-unions:
- discriminator: type
  fields-discriminated:
    foo: Foo
    bar: Bar
oneOf:
  - required: ["foo"]
  - required: ["bar"]
properties:
  type:
    type: string
    enum:
    - Foo
    - Bar

should the discriminator inside the oneOf be set to a narrowed enum?

Until the apiserver supports setting the discriminator field for us, we need to lock it down so users cannot do something like setting the field foo and set the type to Bar, since this will currently pass validation.

For now, I would think a more correct expansion would be:

oneOf:
  - required: ["foo"]
    properties:
        type:
            enum:
            - Foo
  - required: ["bar"]
    properties:
        type:
            enum:
            - Bar

[DirectXMan12]

There's an OpenAPI concept of a discriminator. That should be sufficient to prevent that case.

[apelisse]

+1 on what @DirectXMan12 says, we don't know if it's going to land in 1.16, we can't really agree on all the semantics for union, but we're working on it. Also, I don't think I'm opposed to have the "oneOf" field being generated by kube-builder. The "union" marker that we're proposing in the KEP is orthogonal and still requires validation.

[sttts]

I think unfolding does not harm. The first one looks good:

oneOf:
  - required: ["foo"]
  - required: ["bar"]
properties:
  type:
    type: string
    enum:
    - Foo
    - Bar

But the other one is wrong because {"foo": 1, "bar": 2, "type":"Foo"} passes that spec:

oneOf:
  - required: ["foo"]
    properties:
        type:
            enum:
            - Foo
  - required: ["bar"]
    properties:
        type:
            enum:
            - Bar

[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

[DirectXMan12]

/lifecycle frozen

[k8s-ci-robot]

@mmellison: PR needs rebase.

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/test-infra repository.

[displague]

This approach seems like it could block adopting an approach that permits multiple oneOf required groups within a type as #461 (comment) seeks.

fieldA or fieldB are required and
fieldC or fieldD are required

[k8s-triage-robot]

The lifecycle/frozen label can not be applied to PRs.

This bot removes lifecycle/frozen from PRs because:

• Commenting /lifecycle frozen on a PR has not worked since March 2021
• PRs that remain open for >150 days are unlikely to be easily rebased

You can:

• Rebase this PR and attempt to get it merged
• Close this PR with /close

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

/remove-lifecycle frozen

[adambkaplan]

@seglberg is this PR officially obsolete?

[vincepri]

Closing this due to lack of updates, feel free to pick it up again as needed

[vincepri]

/close

[k8s-ci-robot]

@vincepri: Closed this PR.

In response to this:

/close

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/test-infra repository.

[JoelSpeed]

In case anyone picks this up again, I think, from the docs about union types, that we need to support an "at most one" not an "exactly one". Based on that, I think the schema needs to be more like this below to allow 1, the other, or neither

oneOf:
- not:
    anyOf:
    - required: ["alpha"]
    - required: ["beta"]
- required: ["alpha"]
- required: ["beta"]

I also noticed that in the doc it talks about the idea of having multiple unions embedded within a single struct, to allow this we need to satisfy each union separately, so I think we can do (this represents the two inlined unions in InlinedUnion in the docs examples):

allOf:
- oneOf:
  - not:
      anyOf:
      - required: ["alpha"]
      - required: ["beta"]
  - required: ["alpha"]
  - required: ["beta"]
- oneOf:
  - not:
      anyOf:
      - required: ["fieldA"]
      - required: ["fieldB"]
  - required: ["fieldA"]
  - required: ["fieldB"]

I tested this and seemed to be able to add fieldA alone, fieldA with alpha, but then I couldn't add either fieldB or beta, which is the expected behaviour.

If no one is planning to take this work up, I might try to do this as it would be useful for something I'm working on soon