Property Validation: `oneOf`

[everettraven]

The oneOf constraint on a property means that its value is restricted to matching exactly one of the subschemas.

This should probably follow the same pattern as the existing enum property validation where:

• Adding a new schema to oneOf to an existing oneOf constraint is only OK if it does not result in existing valid values match an existing subschema and the new schema. Default to breaking.
• Adding a net-new oneOf constraint is breaking
• Removing a oneOf schema is breaking

[joelanford]

This one popped up for OLM, reported by Red Hat here: https://issues.redhat.com/browse/OCPBUGS-60958

[veepeeaee]

/assign

[veepeeaee]

I've created a couple of examples to test existing functionality and my changes -

Example 1

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: storages.example.com
spec:
  group: example.com
  names:
    kind: Storage
    plural: storages
    singular: storage
    shortNames:
    - st
  scope: Namespaced
  versions:
    - name: v1
      served: true
      storage: true
      schema:
        openAPIV3Schema:
          type: object
          properties:
            spec:
              type: object
              oneOf:
                - required: ["s3"]
                - required: ["gcs"]
-               - required: ["azure"]
              properties:
                s3:
                  type: object
                  properties:
                    bucket:
                      type: string
                gcs:
                  type: object
                  properties:
                    bucket:
                      type: string
                azure:
                  type: object
                  properties:
                    container:
                      type: string

and

Example 2

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  name: values.example.com
spec:
  group: example.com
  names:
    kind: Value
    plural: values
    singular: value
    shortNames:
    - val
  scope: Namespaced
  versions:
    - name: v1
      served: true
      storage: true
      schema:
        openAPIV3Schema:
          type: object
          properties:
            spec:
              type: object
              properties:
                value:
                  oneOf:
                    - type: string
-                   - type: integer
                    - type: object
                      properties:
                        name:
                          type: string

When I run crdify (on the examples and their corresponding modifications), for the first example I get

CRD Validations
- existingFieldRemoval - ✓
- storedVersionRemoval - ✓
- scope - ✓


Same Version Validations
- v1 - ^.spec - maximum - ✓
- v1 - ^.spec - maxItems - ✓
- v1 - ^.spec - type - ✓
- v1 - ^.spec - minimum - ✓
- v1 - ^.spec - minLength - ✓
- v1 - ^.spec - minProperties - ✓
- v1 - ^.spec - description - ✓
- v1 - ^.spec - minItems - ✓
- v1 - ^.spec - enum - ✓
- v1 - ^.spec - required - ERROR - new required fields: [gcs]
- v1 - ^.spec - default - ✓
- v1 - ^.spec - maxProperties - ✓
- v1 - ^.spec - maxLength - ✓
- v1 - ^.spec - unhandled - ✓


Served Version Validations

and the second examples passes without throwing any error -

CRD Validations
- storedVersionRemoval - ✓
- scope - ✓
- existingFieldRemoval - ✓


Same Version Validations


Served Version Validations

A couple of observations -

• I'm not hitting unhandled changes found error in the bug reported by Redhat (I would've expected to)
• I ran the debugger on the examples, there seems to be issues in the flattening and diff logics - the diff logic doesn't seem to recognize the change in the oneOf constraints. Example 2

For the fix - I think the flattening/diff logic needs to be fixed as well in addition to creating and registering a new validation for oneOf. Let me know what you think.

[everettraven]

@veepeeaee Thanks for taking a look here!

I've been slowly working on #37 as I have time to that should address the flattening/diff logic issues for all property fields.

[veepeeaee]

Thanks for the update! Will see if I can build on you change.

[veepeeaee]

Hi @everettraven,
Pulled in your change from #37 to test - the flattening looks good! For the Example 2 above, I now see

Does the diff logic in FlattenedCRDVersionDiff need to be changed as well? It looks like it's computing the diffs index-wise - so if I remove index 1 from [0, 1, 2], in addition to reporting at the parent level that the oneOf set has changed (which is correct), it also reports that index 2 has changed (from previous value to empty) and index 1 has changed (from the old index 1 value to the old index 2 value)

[everettraven]

@veepeeaee Thanks for the update! I'm glad you were able to verify that my changes are working well for Example 2 :)

Regarding the index removal, that is something that @joelanford and I had talked about when investigating the changes I was making to the flattening logic.

I haven't had much time to dig into this particular problem more, but the issue we ran into during our initial discussion was that there doesn't seem to be a reliable way for us to uniquely identify each entry in the oneOf list.

I certainly want to explore this more, but I think it is going to be quite a challenge in figuring out how we can reliably compare schemas at the index level. I'd love for us to keep the granular ability to compare schemas at the index level, but that may not realistically be feasible for removal type operations. Maybe this weird indexing behavior on removals is just the result of the limitations in how the schemas are defined?

For now, I would say the behavior you are encountering is expected, but we should certainly continue to investigate ways to improve the experience here.