kcp looses rolloutStrategy field data when upgraded from v1alpha3 to v1alpha4

[sadysnaat]

What steps did you take and what happened:
create kind cluster

clusterctl init # Release binary v0.3.23

apply kcp

apiVersion: controlplane.cluster.x-k8s.io/v1alpha3
kind: KubeadmControlPlane
metadata:
  name: controlplane
  namespace: default
spec:
  rolloutStrategy:
    type: RollingUpdate
    rollingUpdate:
      maxSurge: 1
  replicas: 9
  version: v1.20.8
  infrastructureTemplate:
    apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
    kind: MaasMachineTemplate
    name: controlplane
    namespace: default
  kubeadmConfigSpec: {}

get kcp to verify the field data

apiVersion: controlplane.cluster.x-k8s.io/v1alpha3
kind: KubeadmControlPlane
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"controlplane.cluster.x-k8s.io/v1alpha3","kind":"KubeadmControlPlane","metadata":{"annotations":{},"name":"controlplane","namespace":"default"},"spec":{"infrastructureTemplate":{"apiVersion":"infrastructure.cluster.x-k8s.io/v1alpha3","kind":"MaasMachineTemplate","name":"controlplane","namespace":"default"},"kubeadmConfigSpec":{},"replicas":9,"rolloutStrategy":{"rollingUpdate":{"maxSurge":1},"type":"RollingUpdate"},"version":"v1.20.8"}}
  creationTimestamp: "2021-09-15T04:05:54Z"
  generation: 1
  name: controlplane
  namespace: default
  resourceVersion: "1526"
  uid: a46a0f9a-2238-4a23-ac96-f2123e324c4b
spec:
  infrastructureTemplate:
    apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
    kind: MaasMachineTemplate
    name: controlplane
    namespace: default
  kubeadmConfigSpec: {}
  replicas: 9
  rolloutStrategy:
    rollingUpdate:
      maxSurge: 1
    type: RollingUpdate
  version: v1.20.8

upgrade crd to v0.4.2

by

clusterctl4 upgrade apply --contract v1alpha4  #clusterctl4 is Release 0.4.2 binary

What did you expect to happen:
I would expect kcp to still contain rolloutstrategy but

apiVersion: controlplane.cluster.x-k8s.io/v1alpha4
kind: KubeadmControlPlane
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"controlplane.cluster.x-k8s.io/v1alpha3","kind":"KubeadmControlPlane","metadata":{"annotations":{},"name":"controlplane","namespace":"default"},"spec":{"infrastructureTemplate":{"apiVersion":"infrastructure.cluster.x-k8s.io/v1alpha3","kind":"MaasMachineTemplate","name":"controlplane","namespace":"default"},"kubeadmConfigSpec":{},"replicas":9,"rolloutStrategy":{"rollingUpdate":{"maxSurge":1},"type":"RollingUpdate"},"version":"v1.20.8"}}
  creationTimestamp: "2021-09-15T04:05:54Z"
  generation: 1
  name: controlplane
  namespace: default
  resourceVersion: "1526"
  uid: a46a0f9a-2238-4a23-ac96-f2123e324c4b
spec:
  kubeadmConfigSpec: {}
  machineTemplate:
    infrastructureRef:
      apiVersion: infrastructure.cluster.x-k8s.io/v1alpha3
      kind: MaasMachineTemplate
      name: controlplane
      namespace: default
    metadata: {}
  replicas: 9
  version: v1.20.8
status:
  initialized: false
  ready: false

As spec.infrastructureTemplate is translated to spec.machineTemplate I would expect rolloutStrategy
to not be removed

Anything else you would like to add:
No

Environment:

• Cluster-api version: 0.3.23 -> 0.4.2
• Minikube/KIND version: kind v0.11.1 go1.16.4 darwin/amd64
• Kubernetes version: (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.1", GitCommit:"632ed300f2c34f6d6d15ca4cef3d3c7073412212", GitTreeState:"clean", BuildDate:"2021-08-19T15:38:26Z", GoVersion:"go1.16.6", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.1", GitCommit:"5e58841cce77d4bc13713ad2b91fa0d961e69192", GitTreeState:"clean", BuildDate:"2021-05-21T23:01:33Z", GoVersion:"go1.16.4", Compiler:"gc", Platform:"linux/amd64"}

• OS (e.g. from /etc/os-release): 20.6.0 Darwin Kernel Version 20.6.0: Wed Jun 23 00:26:31 PDT 2021; root:xnu-7195.141.2~5/RELEASE_X86_64 x86_64

/kind bug
/area api
/area controlplane

[sadysnaat]

/area api

[sadysnaat]

/area control-plane

[sadysnaat]

/area api
/area control-plane

[sbueringer]

@sadysnaat Nice finding, thx. The issue is probably that we didn't add the rolloutStrategy to the v1alpha3 structs on the main branch (aka v0.4.x). I suspect Kubernetes prunes the rolloutStrategy field before calling the conversion webhook.

Can you try if the CRD in #5237 solves the issue?
(I think you have to use https://cluster-api.sigs.k8s.io/clusterctl/developers.html?highlight=local%20repo#create-the-local-repository and a locally build KCP controller to test it, as soon as the KCP is written with the v0.4.2 CRD the field should be gone (including the KCP controller))

EDIT: I'll test it myself first > Looks like it works. I went through our clusterctl e2e test.

[sbueringer]

/assign

@fabriziopandini @vincepri If my theory is correct, we should go over all our v1alpha3 types on main and compare them with the ones in release-0.3, WDYT?

[sbueringer]

I diffed kcp, kbpk, core and docker and the only other diff is that we we're missing a few constants in api/v1alpha3/{common_types.go,condition_consts.go} on main compared to release-0.3

@vincepri @fabriziopandini Should we add them?

[sbueringer]

@sadysnaat I tested the following with our e2e tests.

current main:

• KCP before upgrade has rolloutStrategy
• Afterwards
  • k get kcp: returns v1alpha3 KCP without rolloutStrategy
  • k get kubeadmcontrolplanes.v1alpha4.controlplane.cluster.x-k8s.io: returns v1alpha4 KCP with rolloutStrategy, but with default values, not the ones I configured before
  • k get kubeadmcontrolplanes.v1alpha3.controlplane.cluster.x-k8s.io: returns v1alpha3 KCP without rolloutStrategy, but with annotation with conversion data and rolloutStrategy defaults)
  • k get kcp: now returns v1alpha4 KCP with rolloutStrategy, but still with default values, not the ones I configured before

#5237

• KCP before upgrade has rolloutStrategy
• Afterwards: stil has the rolloutStrategy with the correct values (doesn't matter which command I use)

[fabriziopandini]

@vincepri @fabriziopandini Should we add them?

Yes, please

[vincepri]

/milestone v0.4