Improve the way we inject ci-artifacts.sh

[sbueringer]

What steps did you take and what happened:

I tried to use kubernetesversions.GenerateCIArtifactsInjectedTemplateForDebian in CAPO e2e tests and instead of just adding files and preKubeadmCommands it overwrote existing ones. In my case those were files required for the cloud controller manager / internal cloud provider.

What did you expect to happen:

I thought GenerateCIArtifactsInjectedTemplateForDebian would just add new elements into those arrays instead of overwriting them completely.

Anything else you would like to add:

The underlying issue is that kustomize just overwrites arrays of CRDs with an unknown schema (kustomize #2825) when using patchesStrategicMerge. This could be fixed by using patchesJson6902 instead.

I already have an implementation which would improve the current state: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/b9109b4125b2dc6dcca3927347252749ae647463/test/e2e/shared/kubernetesversions/template.go

During the implementation I also found a few other things that can be improved:

• gsutil is not really required to download the files from GCS, so we can speed up every server boot by about 3 minutes by skipping the gsutil installation and just using curl
• we don't have to download the control plane images on worker nodes, this also saved about 30 seconds
• bindata is unused, so it can be deleted (it has already been replaced by go embed) (is getting fixed in 🌱Remove go-bindata #4378)

(The durations are from CAPO e2e tests, in this case we are using a OpenStack devstack on a GCP VM)

Environment:

• Cluster-api version: CAPO latest
• Kubernetes version: (use kubectl version): 1.20.4
• OS (e.g. from /etc/os-release): Ubuntu 20.04 (image-builder/qemu)

/kind bug

I already implemented it for CAPO. If there's interest to have this implemented in this repo just let me know. Otherwise it would be okay for me to just maintain our own version of the func in the CAPO repo.

[fabriziopandini]

/cc @randomvariable given that he has more context on this area

My personal experience is that relying in kustomise for upgrading list is always painful, so I would avoid to spread list values between templates and GenerateCIArtifactsInjectedTemplateForDebian. But I agree that this should be at least documented.
Generally +1 for the other improvements

[sbueringer]

@fabriziopandini I agree that it should be avoided if possible.

Right now the only way to use the GenerateCIArtifactsInjectedTemplateForDebian is to not depend on any files or preKubeadmCommands, because they would get overwritten. If that's not possible, you cannot use GenerateCIArtifactsInjectedTemplateForDebian right now.

I think it wasn't an issue yet, because CAPA doesn't need files or preKubeadmCommands and CAPA and CAPO are the only provider currently requiring the GenerateCIArtifactsInjectedTemplateForDebian functionality (usage).

I think inserting at the front of these arrays via JSON patch is a reasonable improvement compared to the current overwrite of the arrays.

P.S. I can also just open a PR and we can discuss there in more detail.

[CecileRobertMichon]

@sbueringer I had the exact same issue in CAPZ which is why we're not currently using GenerateCIArtifactsInjectedTemplateForDebian. See the discussion I had with @randomvariable at the time here: kubernetes-sigs/cluster-api-provider-azure#1018 (comment)

Azure also requires the azure.json file to be written on every machine for in-tree Cloud Controller Manager / out of tree cloud provider configuration.

[sbueringer]

@CecileRobertMichon Thx for the info. That was exactly my problem.

Regarding kubernetes-sigs/cluster-api-provider-azure#1018 (comment), as far as I know we can decide if we want to add the array entries at the start or end of the array via JSON patch.

Then I'll just open a PR here and let's see if we can get consensus on the implementation.

Just that I mentioned it. It's now also possible with the latest kustomize version (not sure if it's already included in the release) to use the strategic merge patch with a custom OpenAPI scheme, but I think that's comparatively complicated to implement (ref: kubernetes-sigs/kustomize#2825).

[vincepri]

/milestone v0.4
/assign @sbueringer
/lifecycle active