ClusterClass Patches add operation can not concatenate a list to existing list

[weikaipan]

User Story

As a operator I would like to author a patch that create a list of items and concatenate it to an existing list for users who supply an unknown size of list in Cluster Variables

Detailed Description

ClusterClass

patches:    
  - name: foo
      definitions:
      - selector:
          apiVersion: vmware.infrastructure.cluster.x-k8s.io/v1beta1
          kind: VSphereMachineTemplate
          matchResources:
            controlPlane: true
        jsonPatches:
          - op: add
            path: /spec/template/spec/volumes/-
            valueFrom:
              template: |
                {{- range .var }}
                - capacity:
                  name: {{ .name }}
                {{- end }}

Cluster

  variables:
      - name: var
        value:
          - name: 'foo'
          - name: 'bar'

VSphereMachineTemplate

---
apiVersion: vmware.infrastructure.cluster.x-k8s.io/v1beta1
kind: VSphereMachineTemplate
metadata:
  name: "foo"
spec:
  template:
    spec:
      volumes: []

This results in error below

 FieldValueInvalid: spec.template.spec.volumes: Invalid value: \"array\": spec.template.spec.volumes in body must be of type object: \"array\""

Since currently appending a list to a list resulting a nested list.
https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md#writing-json-patches

Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

/kind feature

[sbueringer]

I'm not sure how we can implement it given that JSON patch does not support any such operation.

Do you have an idea how this can be solved within the limitations of the JSON Patch RFC6902 spec that we're using?

(related #5944)

[weikaipan]

I tried to initialize a list by add operation.
So I still use go-template to range over a list of items, and use add with /spec/template/spec/kubeadmConfigSpec/files instead of /spec/template/spec/kubeadmConfigSpec/files/-.

As long as the stanza is empty, the json patch creates a list for me. But it means only the first patch to that stanza can work like concatenation.

       jsonPatches:
          - op: add
            path: /spec/template/spec/kubeadmConfigSpec/files
            valueFrom:
              template: |
                {{- range .foo }}
                content: |
                  {{ .data }}
                {{- end }}

[sbueringer]

Yup. I think that's how JSON Patches (RFC6902) work. I think if the array essentially does not exist, it is added. If it already exists it adds the whole array as a new array element.

[weikaipan]

But it implies that if I have another array type variable called bar. The same pattern doesn't work because files is already added. Is it right?

[sbueringer]

Yup definitely. In general adding an array to an existing array will lead to the nested array. Independent of how that situation occurs (from two variables or if the array already exists before)

[killianmuldoon]

@sbueringer should we close #5944 in favor of this? It seems to cover the same ground, just in a less detailed way.

[sbueringer]

Sounds good. I think at some point we mainly wanted to document the limitation in #5944. But as that's done we can close it. (I'll do it)

[sidharthsurana]

One possible way of solving this might to enhance the ClusterClassPatch type to include a looping construct
https://github.com/kubernetes-sigs/cluster-api/blob/main/api/v1beta1/clusterclass_types.go#L313

Examples of similar idea can be seen in ansible task loops construct https://docs.ansible.com/ansible/latest/user_guide/playbooks_loops.html

The idea would be something like below:

Cluster

  variables:
      - name: var
        value:
          - name: 'foo'
          - name: 'bar'

ClusterClass

patches:    
  - name: foo
    definitions:
     - selector:
          apiVersion: vmware.infrastructure.cluster.x-k8s.io/v1beta1
          kind: VSphereMachineTemplate
          matchResources:
            controlPlane: true
        jsonPatches:
          - op: add
            path: /spec/template/spec/volumes/-
            valueFrom:
              template: |
                - capacity:
                  name: {{ .name }}
    with_items: {{ .var }}

The above code then could iteratively apply new jsonpatch add within the guideline on the RFC and still provide a solution.

Note: What I am suggesting is not to support all possible looping constructs that ansible supports, but rather just have support for the most basic looping over array object

[fabriziopandini]

/milestone v1.2
Personally, I think that the long-term answer for all the advanced patching use cases is external patches based on Runtime SDK, and this use case is a good candidate because we are hitting the limit of JSONpatch specification.

However, given that Runtime SDK/external patches is still a WIP we could also consider an option that does not require API changes nor adding other abstractions/constructs on top of JSON patches.

The idea is to surface a new built-in variable with the current value being replaced and then rely on sprig functions; the expected result should be something like

patches:    
  - name: foo
      definitions:
      - selector:
          apiVersion: vmware.infrastructure.cluster.x-k8s.io/v1beta1
          kind: VSphereMachineTemplate
          matchResources:
            controlPlane: true
        jsonPatches:
          - op: replace
            path: /spec/template/spec/volumes
            valueFrom:
              template: {{ concat .builtin.currentValue .var }}

But this requires a little bit of research first

[k8s-triage-robot]

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

[fabriziopandini]

@sbueringer @weikaipan can we close this now that we have topology mutation hooks?

[sbueringer]

I think we have no intention to implement it for inline patches. So in my opinion - yes.

(there also wasn't that much demand for it looking at the issue history)

[vincepri]

/close

per above

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

/close

per above

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.