Fix patchOpenStackCluster spec overwrite

[nilpntr]

When deploying a kamaji cluster on Openstack I would get this error(in the capi-kamaji-controller-manager) after the TenantControlPlane had been created, initialised and being ready:

2024-11-13T10:34:25Z	ERROR	Reconciler error	{"controller": "kamajicontrolplane", "controllerGroup": "controlplane.cluster.x-k8s.io", "controllerKind": "KamajiControlPlane", "KamajiControlPlane": {"name":"test-kamaji-cluster-control-plane","namespace":"test-kamaji-cluster"}, "namespace": "test-kamaji-cluster", "name": "test-kamaji-cluster-control-plane", "reconcileID": "42e03cd9-c0b9-445e-ac8f-0d451ddf8e63", "error": "cannot perform PATCH update for the OpenStackCluster resource: failed to patch OpenStackCluster test-kamaji-cluster/test-kamaji-cluster: OpenStackCluster.infrastructure.cluster.x-k8s.io \"test-kamaji-cluster\" is invalid: [spec.identityRef: Required value, <nil>: Invalid value: \"null\": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation]", "errorVerbose": "OpenStackCluster.infrastructure.cluster.x-k8s.io \"test-kamaji-cluster\" is invalid: [spec.identityRef: Required value, <nil>: Invalid value: \"null\": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation]\nfailed to patch OpenStackCluster test-kamaji-cluster/test-kamaji-cluster\nsigs.k8s.io/cluster-api/util/patch.(*Helper).Patch\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/util/patch/patch.go:157\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.(*KamajiControlPlaneReconciler).patchOpenStackCluster\n\t/workspace/controllers/kamajicontrolplane_controller_cluster_patch.go:234\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.(*KamajiControlPlaneReconciler).patchCluster\n\t/workspace/controllers/kamajicontrolplane_controller_cluster_patch.go:101\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.(*KamajiControlPlaneReconciler).Reconcile.func6\n\t/workspace/controllers/kamajicontrolplane_controller.go:185\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.TrackConditionType\n\t/workspace/controllers/conditions.go:26\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.(*KamajiControlPlaneReconciler).Reconcile\n\t/workspace/controllers/kamajicontrolplane_controller.go:184\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Reconcile\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:116\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:303\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:263\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2.2\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:224\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1695\ncannot perform PATCH update for the OpenStackCluster resource\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.(*KamajiControlPlaneReconciler).patchOpenStackCluster\n\t/workspace/controllers/kamajicontrolplane_controller_cluster_patch.go:235\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.(*KamajiControlPlaneReconciler).patchCluster\n\t/workspace/controllers/kamajicontrolplane_controller_cluster_patch.go:101\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.(*KamajiControlPlaneReconciler).Reconcile.func6\n\t/workspace/controllers/kamajicontrolplane_controller.go:185\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.TrackConditionType\n\t/workspace/controllers/conditions.go:26\ngithub.com/clastix/cluster-api-control-plane-provider-kamaji/controllers.(*KamajiControlPlaneReconciler).Reconcile\n\t/workspace/controllers/kamajicontrolplane_controller.go:184\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Reconcile\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:116\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).reconcileHandler\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:303\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).processNextWorkItem\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:263\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller[...]).Start.func2.2\n\t/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:224\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1695"}

After digging in the code I discovered that the patchOpenStackCluster function would try to overwrite the spec of an OpenstackCluster. To test my solution I've created a little test script which loads a json kind: Cluster and tries the old implementation and the fix:

package main

import (
	"context"
	"encoding/json"
	"fmt"
	"github.com/pkg/errors"
	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
	"k8s.io/apimachinery/pkg/types"
	"k8s.io/client-go/tools/clientcmd"
	"k8s.io/client-go/util/homedir"
	"log"
	"os"
	"path/filepath"
	capiv1beta1 "sigs.k8s.io/cluster-api/api/v1beta1"
	"sigs.k8s.io/controller-runtime/pkg/client"
)

func main() {
	kubeConfig := filepath.Join(homedir.HomeDir(), ".kube", "config")
	cfg, err := clientcmd.BuildConfigFromFlags("", kubeConfig)
	if err != nil {
		fmt.Printf("Error loading kubeconfig: %v\n", err)
		return
	}

	k8sClient, err := client.New(cfg, client.Options{})
	if err != nil {
		fmt.Printf("Error creating client: %v\n", err)
		return
	}

	raw, err := os.ReadFile("cluster.json")
	if err != nil {
		log.Fatal(err)
	}

	var cluster capiv1beta1.Cluster
	if err = json.Unmarshal(raw, &cluster); err != nil {
		log.Fatal(err)
	}

	ctx := context.Background()

	if err = patchOpenStackCluster(ctx, k8sClient, cluster, "123.456.78.91", 6443); err != nil {
		log.Fatal(err)
	}

	if err = patchOpenStackClusterFix(ctx, k8sClient, cluster, "123.456.78.91", 6443); err != nil {
		log.Fatal(err)
	}
}

func patchOpenStackCluster(ctx context.Context, k8sClient client.Client, cluster capiv1beta1.Cluster, endpoint string, port int64) error {
	osc := unstructured.Unstructured{}

	osc.SetGroupVersionKind(cluster.Spec.InfrastructureRef.GroupVersionKind())
	osc.SetName(cluster.Spec.InfrastructureRef.Name)
	osc.SetNamespace(cluster.Spec.InfrastructureRef.Namespace)

	if err := k8sClient.Get(ctx, types.NamespacedName{Name: osc.GetName(), Namespace: osc.GetNamespace()}, &osc); err != nil {
		return errors.Wrap(err, fmt.Sprintf("cannot retrieve the %s resource", osc.GetKind()))
	}

	if err := unstructured.SetNestedMap(osc.Object, map[string]interface{}{
		"apiServerFixedIP": endpoint,
		"apiServerPort":    port,
	}, "spec"); err != nil {
		return errors.Wrap(err, fmt.Sprintf("unable to set unstructured %s spec patch", osc.GetKind()))
	}

	out, err := osc.MarshalJSON()
	if err != nil {
		return errors.Wrap(err, fmt.Sprintf("unable to marshal unstructured %s spec patch", osc.GetKind()))
	}

	if err = os.WriteFile("cluster-patch.json", out, 0600); err != nil {
		return errors.Wrap(err, fmt.Sprintf("unable to write cluster-patch.json"))
	}

	return nil
}

func patchOpenStackClusterFix(ctx context.Context, k8sClient client.Client, cluster capiv1beta1.Cluster, endpoint string, port int64) error {
	osc := unstructured.Unstructured{}

	osc.SetGroupVersionKind(cluster.Spec.InfrastructureRef.GroupVersionKind())
	osc.SetName(cluster.Spec.InfrastructureRef.Name)
	osc.SetNamespace(cluster.Spec.InfrastructureRef.Namespace)

	if err := k8sClient.Get(ctx, types.NamespacedName{Name: osc.GetName(), Namespace: osc.GetNamespace()}, &osc); err != nil {
		return errors.Wrap(err, fmt.Sprintf("cannot retrieve the %s resource", osc.GetKind()))
	}

	if err := unstructured.SetNestedField(osc.Object, endpoint, "spec", "apiServerFixedIP"); err != nil {
		return errors.Wrap(err, fmt.Sprintf("unable to set unstructured %s spec apiServerFixedIP", osc.GetKind()))
	}

	if err := unstructured.SetNestedField(osc.Object, port, "spec", "apiServerPort"); err != nil {
		return errors.Wrap(err, fmt.Sprintf("unable to set unstructured %s spec apiServerPort", osc.GetKind()))
	}

	out, err := osc.MarshalJSON()
	if err != nil {
		return errors.Wrap(err, fmt.Sprintf("unable to marshal unstructured %s spec patch", osc.GetKind()))
	}

	if err = os.WriteFile("cluster-patch-fix.json", out, 0600); err != nil {
		return errors.Wrap(err, fmt.Sprintf("unable to write cluster-patch.json"))
	}

	return nil
}

Comparing the 2 outputs I saw that the current implementation overwrites the spec with an object containing only the 2 fields. When looking at the fix I saw that it just modifies the fields as we would expect.

[prometherion]

LGTM, may I ask you to lint the commit message, please?

✖   body's lines must not be longer than 100 characters [body-max-line-length]

[nilpntr]

LGTM, may I ask you to lint the commit message, please?

✖   body's lines must not be longer than 100 characters [body-max-line-length]

Thanks, changed my commit message!

[prometherion]

LGTM, happy to have an additional contributor to Kamaji! 🚀

[nilpntr]

LGTM, happy to have an additional contributor to Kamaji! 🚀

Thanks, looking forward to contributing more often when needed on this project!