Unexpected behavior in "append" to array patch operation

[killianmuldoon]

An append or prepend operation when attempting to patch a Cluster with ClusterClass can not be used to append or prepend one array onto an existing array. Currently the operation only supports adding a single value to the array. In a case where multiple values are required there needs to be a patch for each value.

If an array is added as a variable in an append operation reconciliation fails and an error like the below occurs:

E0117 14:18:28.709080      18 controller.go:317] controller/topology/cluster "msg"="Reconciler error" "error"="error reconciling the Cluster topology: failed to create MachineDeployment/my-cluster-default-worker-topo-4h7mw: failed to create DockerMachineTemplate/my-cluster-default-worker-topo-infra-nwt7k: DockerMachineTemplate.infrastructure.cluster.x-k8s.io \"my-cluster-default-worker-topo-infra-nwt7k\" is invalid: spec.template.spec.preLoadImages: Invalid value: \"array\": spec.template.spec.preLoadImages in body must be of type string: \"array\"" "name"="my-cluster" "namespace"="default" "reconciler group"="cluster.x-k8s.io" "reconciler kind"="Cluster" 

For reference this error occurs with a patch and variable combination as below:

  variables:
    - name: preloadImages
      required: true
      schema:
        openAPIV3Schema:
          type: array
          items:
            type: string
          default: [kindest/haproxy]
  patches:
    - name: preloadImages
      definitions:
        - selector:
            apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
            kind: DockerMachineTemplate
            matchResources:
              machineDeploymentClass:
                names:
                - default-worker
          jsonPatches:
            - op: add
              path: "/spec/template/spec/preLoadImages/0"
              valueFrom:
                variable: preloadImages

As a user I would expect the items in the variables array to be added to the target array by the patch. Instead I get a repeated error during reconciliation.

Possible solutions:

• Handle the flattening of the array inside the reconciliation loop
• Block this type of append operation in the webhook on clusterClass creation (idon't allow a variable of type array to be used in an append operation)

[killianmuldoon]

/area topology

[fabriziopandini]

Let start documenting this for now @sbueringer
/milestone v1.1

[fabriziopandini]

/kind documentation
For the initial part of this effort, the we will figure it out if code changes are required

[tenczar]

I also have a situation where an add all array operation would be very useful. My template includes branching logic when skipping image repository tls verification. This could potentially be pulled out into separate patch definitions but it really complicates the logic and we need to make sure the ordering is correct so the system restart is called after the conditionally added values.

- op: add
  path: /spec/template/spec/kubeadmConfigSpec/preKubeadmCommands/-
    valueFrom:
      template: |
        - sed -i 's|\".*/pause|\{{.TKG_IMAGE_REPO}}/pause|' /etc/containerd/config.toml
        {{- if .TKG_CUSTOM_IMAGE_REPOSITORY_SKIP_TLS_VERIFY}}
        - echo '[plugins.\"io.containerd.grpc.v1.cri\".registry.configs.\"{{.TKG_CUSTOM_IMAGE_REPOSITORY_HOSTNAME}}\".tls]' >> /etc/containerd/config.toml
        - echo '  insecure_skip_verify = true' >> /etc/containerd/config.toml
        {{- end}}
        - systemctl restart containerd

[sbueringer]

Thx for the additional use case.

I think our main problem is how we would model this in the API. Currently, we are pretty close to the JSON patch spec (RFC 6902).

[sbueringer]

We documented the limitation in "Writing a ClusterClass" (https://cluster-api.sigs.k8s.io/tasks/experimental-features/cluster-class/write-clusterclass.html)

Let's close this issue in favor of #6245 regarding a potential implementation.

/close

[k8s-ci-robot]

@sbueringer: Closing this issue.

In response to this:

We documented the limitation in "Writing a ClusterClass" (https://cluster-api.sigs.k8s.io/tasks/experimental-features/cluster-class/write-clusterclass.html)

Let's close this issue in favor of #6245 regarding a potential implementation.

/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.