Research on tests for API Changes options

[nesmabadr]

Description

Since we can have multiple API versions supported (now we have v1beta1 and v1beta2), we need to investigate a way to add tests to validate that any changes/fields added to an API version get added to the other version as well.

Reasons

Since we can have multiple API versions supported, it can likely happen that we only add a field to the new API version forgetting about the old ones, and this can introduce issues in the Lifecycle Manager

Acceptance Criteria

• Think of a way to test that API changes are consistent among the different versions.
• Investigate whether such tests could be implemented/how to do that
• Create a follow-up issue for the implementation

Feature Testing

No response

Testing approach

No response

Attachments

No response

[c-pius]

Idea 1

Use the generated OpenAPISpecification (OAS) v3 schema and compare old vs new version for differences in a GH Action.

TL;DR: the only viable option seems to be option 3

For testing, the schema of our KymaCRD is taken and the following additional changes are done to v1beta2:

• various descriptions are changed (this should be ignored)
• some property is removed (needs to be detected)
• some property is added (needs to be detected)
• some enum entry is added (needs to be detected)
• some type is changed (needs to be detected)

1. dyff

Plain YAML diff tool.

+ can work directly on the same file with two different root paths
+ large numbers of stars (1.2k)
+ allows to filter out description changes ('--exclude-regexp ".*description"')
+ nice output describing differences (value change, key addition, key removal, ...)
+ under MIT license (low risk)
- filtering/exclusion options do not allow filtering on certain keys added/removed from a map (e.g., to filter out the removed entry 'properties.spec.properties.sync', we would need to filter out the whole 'properties.spec.properties' path)
- large number of open and unaddressed issues
- moderate maintenance (one commit every month or so)

Example

dyff between ./config/crd/bases/operator.kyma-project.io_kymas.yaml ./config/crd/bases/operator.kyma-project.io_kymas.yaml --chroot-of-from "spec.versions.0.schema.openAPIV3Schema" --chroot-of-to "spec.versions.1.schema.openAPIV3Schema" --exclude-regexp ".*description"
     _        __  __
   _| |_   _ / _|/ _|  between ./config/crd/bases/operator.kyma-project.io_kymas.yaml, YAML root was changed to spec.versions.0.schema.openAPIV3Schema
 / _' | | | | |_| |_       and ./config/crd/bases/operator.kyma-project.io_kymas.yaml, YAML root was changed to spec.versions.1.schema.openAPIV3Schema
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned six differences
        |___/

properties.spec.properties
  - one map entry removed:
    sync:
    │ type: object
    │ description: "Active Synchronization Settings"
    │ properties:
    │ │ enabled:
    │ │ │ type: boolean
    │ │ │ default: false
    │ │ │ description: |
    │ │ │ │ Enabled set to true will look up a kubeconfig for the remote cluster based on the strategy
    │ │ │ │ and synchronize its state there.
    │ │ moduleCatalog:
    │ │ │ type: boolean
    │ │ │ default: true
    │ │ │ description: |
    │ │ │ │ ModuleCatalog set to true will cause a copy of all ModuleTemplate in the cluster
    │ │ │ │ to be synchronized for discovery purposes
    │ │ namespace:
    │ │ │ type: string
    │ │ │ description: |
    │ │ │ │ The target namespace, if empty the namespace is reflected from the control plane
    │ │ │ │ Note that cleanup is currently not supported if you are switching the namespace, so you will
    │ │ │ │ manually need to clean up old synchronized Kymas
    │ │ noModuleCopy:
    │ │ │ type: boolean
    │ │ │ default: true
    │ │ │ description: |
    │ │ │ │ NoModuleCopy set to true will cause the remote Kyma to be initialized without copying over the
    │ │ │ │ module spec of the control plane into the SKR
    │ │ strategy:
    │ │ │ type: string
    │ │ │ default: secret
    │ │ │ description: "Strategy determines the way to look up the remotely synced kubeconfig, by default it is fetched from a secret"

properties.spec.properties.modules
  + two map entries added:
    x-kubernetes-list-map-keys:
    - name
    x-kubernetes-list-type: map

properties.spec.properties.modules.items.properties
  - one map entry removed:
    remoteModuleTemplateRef:
    │ type: string
    │ description: |
    │ │ RemoteModuleTemplateRef is deprecated and will no longer have any functionality.
    │ │ It will be removed in the upcoming API version.

properties.spec.properties.modules.items.properties.controller.type
  ± value change
    - string
    + number

properties.spec.properties.modules.items.properties.customResourcePolicy.enum
  + one list entry added:
    - SomeNewEnumValue

properties.status.properties.modules.items.properties
  + one map entry added:
    newState:
    │ type: string
    │ description: "THIS IS A NEW PROPERTY THAT HAS BEEN ADDED."
    │ enum:
    │ - Processing
    │ - Deleting
    │ - Ready
    │ - Error
    │ -
    │ - Warning

Detects changes to sync and x-kubernetes-... which cannot be filtered out.

2. oasdiff

Tool that is detecting breaking changes on OAS3 documents also offering plain diff functionality.

The focus on breaking changes in OAS is likely irrelevant to us. First, our schema is described as an extension, not a request or response. Therefore the tool doesn't apply the breaking or changelog commands to our schema. Second, even if we use a modified input where our schema is a request or response, we are interested in INFO level output (field additions are not breaking changes). We will need to rely on the plain diff functionality.

+ seems to be very actively mainained (several commits each month)
+ offers a GH Action
+ under Apache 2.0 license (low risk)
- moderate number of stars
- not possible to specify root paths (we need to create two deticated schema files to compare)
- not possible to apply the filters that we need (filters work on api paths, it appears there is no possibility to filter out JSON paths within the extension) => this is a deal breaker

Example

oasdiff diff v1beta1.yaml v1beta2.yaml
extensions:
    modified:
        kind:
            - oldValue: |-
                Kind is a string value representing the REST resource this object represents.
                Servers may infer this from the endpoint the client submits requests to.
                Cannot be updated.
                In CamelCase.
                More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
              value: |-
                Kind is a string value representing the REST resource this object represents.
                Servers may infer this from the endpoint the client submits requests to.
                Cannot be updated.
                In CamelCase.
                More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
                THIS HAS BEEN MODIFIED.
              op: replace
              from: ""
              path: /description
        spec:
            - oldValue: Module defines the components to be installed.
              value: Module defines the components to be installed. THIS HAS BEEN MODIFIED.
              op: replace
              from: ""
              path: /properties/modules/items/description
            - oldValue: |-
                Channel is the desired channel of the Module. If this changes or is set, it will be used to resolve a new
                ModuleTemplate based on the new resolved resources.
              value: |-
                Channel is the desired channel of the Module. If this changes or is set, it will be used to resolve a new
                ModuleTemplate based on the new resolved resources. THIS HAS BEEN MODIFIED.
              op: replace
              from: ""
              path: /properties/modules/items/properties/channel/description
            - oldValue: |-
                ControllerName is able to set the controller used for reconciliation of the module. It can be used
                together with Cache Configuration on the Operator responsible for the templated Modules to split
                workload. THIS HAS BEEN MODIFIED.
              value: |-
                ControllerName is able to set the controller used for reconciliation of the module. It can be used
                together with Cache Configuration on the Operator responsible for the templated Modules to split
                workload.
              op: replace
              from: ""
              path: /properties/modules/items/properties/controller/description
            - oldValue: string
              value: number
              op: replace
              from: ""
              path: /properties/modules/items/properties/controller/type
            - oldValue: null
              value: SomeNewEnumValue
              op: add
              from: ""
              path: /properties/modules/items/properties/customResourcePolicy/enum/-
            - oldValue:
                description: |-
                    RemoteModuleTemplateRef is deprecated and will no longer have any functionality.
                    It will be removed in the upcoming API version.
                type: string
              value: null
              op: remove
              from: ""
              path: /properties/modules/items/properties/remoteModuleTemplateRef
        status:
            - oldValue: null
              value:
                description: THIS IS A NEW PROPERTY THAT HAS BEEN ADDED.
                enum:
                    - Processing
                    - Deleting
                    - Ready
                    - Error
                    - ""
                    - Warning
                type: string
              op: add
              from: ""
              path: /properties/modules/items/properties/newState

Detects changes to sync and x-kubernetes-... which cannot be filtered out.

3. yq preprocessing with dyff or diff

Use yq as a preprocessor to strip the parts of the schemas that are known and expected to differ. Use dyff or diff to compare the resulting document for changes.

+ should be able to cater our needs
+ yq under MIT license (low risk)
- needs to use two tools in combination
- preprocessing the yamls may become rather chatty compared to ignoring certain paths
- diff under GNU public license (high risk)

Example

# preprocess schemas

yq '.spec.versions.[] | select(.name == "v1beta1") | .schema.openAPIV3Schema.properties | del(.spec.properties.sync)' ../../config/crd/bases/operator.kyma-project.io_kymas.yaml >> v1beta1.yaml

yq '.spec.versions.[] | select(.name == "v1beta2") | .schema.openAPIV3Schema.properties | del(.spec.properties.modules.x-kubernetes-list-map-keys) | del(.spec.properties.modules.x-kubernetes-list-type)' ../../config/crd/bases/operator.kyma-project.io_kymas.yaml >> v1beta2.yaml

# using dyiff
dyff between v1beta1.yaml v1beta2.yaml --exclude-regexp ".*description" --set-exit-code                                                                                                         

     _        __  __
   _| |_   _ / _|/ _|  between v1beta1.yaml
 / _' | | | | |_| |_       and v1beta2.yaml
| (_| | |_| |  _|  _|
 \__,_|\__, |_| |_|   returned four differences
        |___/

spec.properties.modules.items.properties
  - one map entry removed:
    remoteModuleTemplateRef:
    │ type: string
    │ description: |
    │ │ RemoteModuleTemplateRef is deprecated and will no longer have any functionality.
    │ │ It will be removed in the upcoming API version.

spec.properties.modules.items.properties.controller.type
  ± value change
    - string
    + number

spec.properties.modules.items.properties.customResourcePolicy.enum
  + one list entry added:
    - SomeNewEnumValue

status.properties.modules.items.properties
  + one map entry added:
    newState:
    │ type: string
    │ description: "THIS IS A NEW PROPERTY THAT HAS BEEN ADDED."
    │ enum:
    │ - Processing
    │ - Deleting
    │ - Ready
    │ - Error
    │ -
    │ - Warning

• various descriptions are changed (this should be ignored)
• some property is removed (needs to be detected)
• some property is added (needs to be detected)
• some enum entry is added (needs to be detected)
• some type is changed (needs to be detected)

# using diff
diff v1beta1.yaml v1beta2.yaml -I '.*description:'

14a15
>     THIS HAS BEEN MODIFIED.
35c36
<               ModuleTemplate based on the new resolved resources.
---
>               ModuleTemplate based on the new resolved resources. THIS HAS BEEN MODIFIED.
44,45c45,46
<               workload. THIS HAS BEEN MODIFIED.
<             type: string
---
>               workload.
>             type: number
54a56
>               - SomeNewEnumValue
67,71d68
<           remoteModuleTemplateRef:
<             description: |-
<               RemoteModuleTemplateRef is deprecated and will no longer have any functionality.
<               It will be removed in the upcoming API version.
<             type: string
288a286,295
>             enum:
>               - Processing
>               - Deleting
>               - Ready
>               - Error
>               - ""
>               - Warning
>             type: string
>           newState:
>             description: THIS IS A NEW PROPERTY THAT HAS BEEN ADDED.

• various descriptions are changed (this should be ignored)
  • We can observe that the regex ignoring description changes does not properly work on multiline descriptions.
• some property is removed (needs to be detected)
• some property is added (needs to be detected)
• some enum entry is added (needs to be detected)
• some type is changed (needs to be detected)

[c-pius]

Idea 2

Create a test that is looping over all fields of the respective Go structs. The test fails if one struct contains a field that the other doesn't, and if the types of the fields (only checking name) do not match. For known deviations, exceptions are added.

To prepare for future evolution of currently shared types (e.g., v1beta1 re-uses v1beta2.KymaStatus), type aliases are introduced so that the test already references the distinct type. Additionally, we explicitly move all contents that are shared between two versions to shared package. This should make it obvious that when editing a non-shared part of the API, the other versions have to be considered.

A PoC for this can be found here: https://github.com/kyma-project/lifecycle-manager/pull/1490/files

+ part of the existing unit testing pipeline (fast feedback)
- cannot detect changes originating from generator annotations (e.g., json names, enum values, k-8s map extensions, ...)

[c-pius]

Closing as we agreed to implement Idea 1 Option 3 as a first step in #1492.

If we find that it makes difficulties we will adapt the approach.