Property Validation: oneOf

[veepeeaee]

Fix for #25

Depends on #37

Adds a validation for the oneOf constraint.
The following changes are considered invalid -

• adding a new oneOf constraint where previously there was none
• removing an existing subschema from a oneOf constraint
• adding a new subschema to a oneOf constraint

Potential improvements (in this or subsequent PRs)

• I've created a struct for configuring the behaviour of the validation (similar to in enum.go) but left it empty - should we start off with a config here to allow adding new subschemas to a oneOf constraint?
• the marshallSchema function tries to hash a schema based on the fields which are "structurally" relevant - here I've set fields like description and example to empty and converted the rest to json. Is there a better way/existing utility method?
• while this validation reports changes in a oneOf clause correctly, it doesn't prevent downstream validations from emitting false positives, because of index-wise comparison of the old and new sub-schemas (more details in the discussion in Property Validation: oneOf #25)
• there might be common functionality here which could be reused in the implementations of the other issues in change module and documentation references from crd-diff to crdify #14 - can refactor out to a common library

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: veepeeaee
Once this PR has been reviewed and has the lgtm label, please assign jpbetz for approval. For more information see the Code Review Process.

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

Details

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

[k8s-ci-robot]

Welcome @veepeeaee!

It looks like this is your first PR to kubernetes-sigs/crdify 🎉. 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/crdify 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. 😃

[veepeeaee]

/hold

[veepeeaee]

/hold cancel

[veepeeaee]

/hold

[JoelSpeed]

Is there a way that we could detect a loosening validation that went from format: string to oneOf: [{format: string}, {format: integer}]? This wouldn't necessarily break existing values, but could break older clients reading the types. Some may chose to allow this kind of change?

[veepeeaee]

Is there a way that we could detect a loosening validation that went from format: string to oneOf: [{format: string}, {format: integer}]? This wouldn't necessarily break existing values, but could break older clients reading the types. Some may chose to allow this kind of change?

Any changes in format should hit an unhandled error until #18 is addressed, but otherwise this PR should detect a looseing validation - it would also throw an error about a new oneOf being added when there was none.

Example -

apiVersion: apiextensions.k8s.io/v1
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: myresources.stable.example.com
spec:
  group: stable.example.com
  names:
    plural: myresources
    singular: myresource
    kind: MyResource
    shortNames:
      - mr
  scope: Namespaced
  versions:
    - name: v1
      served: true
      storage: true
      schema:
        openAPIV3Schema:
          type: object
          properties:
            spec:
              type: object
              properties:
                port:
-                 type: integer
-                 format: int32
+                 oneOf:
+                   - type: integer
+                      format: int32
+                   - type: string
+                      format: uuid

Output

- v1 - ^.spec.port - oneOf - ERROR - [oneOf constraint added when there was none previously, allowed oneOf schemas added: [{"type":"integer","format":"int32"} {"type":"string","format":"uuid"}]]
- v1 - ^.spec.port - type - ERROR - type changed : "integer" -> ""
- v1 - ^.spec.port - unhandled - ERROR - unhandled changes found :
  &v1.JSONSchemaProps{
  	... // 3 identical fields
  	Description: "",
  	Type:        "",
- 	Format:      "int32",
+ 	Format:      "",
  	Title:       "",
  	Default:     nil,
  	... // 36 identical fields
  }

I think this might be the best we can do with the current flattening-diff approach. Lmk what you think

[everettraven]

I think this might be the best we can do with the current flattening-diff approach. Lmk what you think

I don't think this is a limitation in the diff flattening approach actually. I think this is a limitation in having the individual validations reset the schema properties that they handle. I imagine that deferring that behavior instead of having that in each validation may unlock the ability to do more complex cross-property validations.

I think the use case that @JoelSpeed is calling out here is absolutely reasonable for us to try and consider, but I think we need a bit more nuanced exploration into how we achieve that. In my head, I'm imaging some kind of more generic common logic shared between all validations that can automatically check if an existing schema configuration is still valid with a *Of schema-property change.

Without having reviewed this particular PR further, I'd say starting with implementing the basics is generally better than having nothing and this more complex behavior isn't something I'd likely block this PR on (although bonus points if you do get it implemented in this in a generically usable way).

If we don't do it as part of this PR though, I would say we certainly should create an issue to track the need to do this work.

[k8s-triage-robot]

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle stale

[k8s-triage-robot]

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle rotten