🐛 Harden deletion logic

api/v1alpha1/ionoscloudcluster_types.go

@@ -27,6 +27,10 @@ const (
 	// associated with the IonosCloudCluster before removing it from the API server.
 	ClusterFinalizer = "ionoscloudcluster.infrastructure.cluster.x-k8s.io"
 
+	// ClusterCredentialsFinalizer allows cleanup of resources, which are
+	// associated with the IonosCloudCluster credentials before removing it from the API server.
+	ClusterCredentialsFinalizer = ClusterFinalizer + "/credentials"
+
 	// IonosCloudClusterReady is the condition for the IonosCloudCluster, which indicates that the cluster is ready.
 	IonosCloudClusterReady clusterv1.ConditionType = "ClusterReady"
 

internal/controller/ionoscloudcluster_controller.go

@@ -171,8 +171,15 @@ func (r *IonosCloudClusterReconciler) reconcileDelete(
 		return ctrl.Result{RequeueAfter: defaultReconcileDuration}, nil
 	}
 
-	// TODO(lubedacht): check if there are any more machine CRs existing.
-	// If there are requeue with an offset.
+	machines, err := clusterScope.ListMachines(ctx, nil)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	if len(machines) > 0 {
+		log.Info("Waiting for all IonosCloudMachines to be deleted", "remaining", len(machines))
+		return ctrl.Result{RequeueAfter: defaultReconcileDuration}, nil
+	}
 
 	reconcileSequence := []serviceReconcileStep[scope.Cluster]{
 		{"ReconcileControlPlaneEndpointDeletion", cloudService.ReconcileControlPlaneEndpointDeletion},
@@ -186,6 +193,9 @@ func (r *IonosCloudClusterReconciler) reconcileDelete(
 			return ctrl.Result{RequeueAfter: defaultReconcileDuration}, err
 		}
 	}
+	if err := removeCredentialsFinalizer(ctx, r.Client, clusterScope.IonosCluster); err != nil {
+		return ctrl.Result{}, err
+	}
 	controllerutil.RemoveFinalizer(clusterScope.IonosCluster, infrav1.ClusterFinalizer)
 	return ctrl.Result{}, nil
 }

internal/controller/ionoscloudmachine_controller.go

@@ -153,7 +153,7 @@ func (r *IonosCloudMachineReconciler) reconcileNormal(
 
 	if controllerutil.AddFinalizer(machineScope.IonosMachine, infrav1.MachineFinalizer) {
 		if err := machineScope.PatchObject(); err != nil {
-			log.Error(err, "unable to update finalizer on object")
+			err = fmt.Errorf("unable to update finalizer on object: %w", err)
 			return ctrl.Result{}, err
 		}
 	}

internal/controller/util.go

@@ -25,6 +25,7 @@ import (
 	sdk "github.com/ionos-cloud/sdk-go/v6"
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	infrav1 "github.com/ionos-cloud/cluster-api-provider-ionoscloud/api/v1alpha1"
 	icc "github.com/ionos-cloud/cluster-api-provider-ionoscloud/internal/ionoscloud/client"
@@ -80,6 +81,10 @@ func createServiceFromCluster(
 		return nil, err
 	}
 
+	if err := ensureSecretControlledByCluster(ctx, c, cluster, &authSecret); err != nil {
+		return nil, err
+	}
+
 	token := string(authSecret.Data["token"])
 	apiURL := string(authSecret.Data["apiURL"])
 	caBundle := authSecret.Data["caBundle"]
@@ -91,3 +96,43 @@ func createServiceFromCluster(
 
 	return cloud.NewService(ionosClient, log)
 }
+
+// ensureSecretControlledByCluster ensures that the secrets will contain a finalizer and a controller reference.
+// The secret should only be deleted when there are no resources left in the IONOS Cloud environment.
+func ensureSecretControlledByCluster(
+	ctx context.Context, c client.Client,
+	cluster *infrav1.IonosCloudCluster,
+	secret *corev1.Secret,
+) error {
+	requireUpdate := controllerutil.AddFinalizer(secret, infrav1.ClusterCredentialsFinalizer)
+
+	if !controllerutil.HasControllerReference(secret) {
+		if err := controllerutil.SetControllerReference(cluster, secret, c.Scheme()); err != nil {
+			return err
+		}
+		requireUpdate = true
+	}
+
+	if requireUpdate {
+		return c.Update(ctx, secret)
+	}
+
+	return nil
+}
+
+// removeCredentialsFinalizer removes the finalizer from the credential secret.
+func removeCredentialsFinalizer(ctx context.Context, c client.Client, cluster *infrav1.IonosCloudCluster) error {
+	secretKey := client.ObjectKey{
+		Namespace: cluster.Namespace,
+		Name:      cluster.Spec.CredentialsRef.Name,
+	}
+	var secret corev1.Secret
+
+	if err := c.Get(ctx, secretKey, &secret); err != nil {
+		// If the secret does not exist anymore, there is nothing we can do.
+		return client.IgnoreNotFound(err)
+	}
+
+	controllerutil.RemoveFinalizer(&secret, infrav1.ClusterCredentialsFinalizer)
+	return c.Update(ctx, &secret)
+}

internal/service/cloud/network.go

@@ -443,7 +443,7 @@ func (s *Service) getServerNICID(ctx context.Context, ms *scope.Machine) (string
 			log.Info("Server was not found or already deleted.")
 			return "", nil
 		}
-		log.Error(err, "Unable to retrieve server")
+		err = fmt.Errorf("unable to retrieve server %w", err)
 		return "", err
 	}
 

scope/cluster.go

@@ -44,6 +44,7 @@ type resolver interface {
 
 // Cluster defines a basic cluster context for primary use in IonosCloudClusterReconciler.
 type Cluster struct {
+	client       client.Client
 	patchHelper  *patch.Helper
 	resolver     resolver
 	Cluster      *clusterv1.Cluster
@@ -78,6 +79,7 @@ func NewCluster(params ClusterParams) (*Cluster, error) {
 	}
 
 	clusterScope := &Cluster{
+		client:       params.Client,
 		Cluster:      params.Cluster,
 		IonosCluster: params.IonosCluster,
 		patchHelper:  helper,
@@ -121,6 +123,26 @@ func (c *Cluster) SetControlPlaneEndpointIPBlockID(id string) {
 	c.IonosCluster.Status.ControlPlaneEndpointIPBlockID = id
 }
 
+// ListMachines returns a list of IonosCloudMachines in the same namespace and with the same cluster label.
+// With machineLabels, additional search labels can be provided.
+func (c *Cluster) ListMachines(
+	ctx context.Context,
+	machineLabels client.MatchingLabels,
+) ([]infrav1.IonosCloudMachine, error) {
+	if machineLabels == nil {
+		machineLabels = client.MatchingLabels{}
+	}
+
+	machineLabels[clusterv1.ClusterNameLabel] = c.Cluster.Name
+	listOpts := []client.ListOption{client.InNamespace(c.Cluster.Namespace), machineLabels}
+
+	machineList := &infrav1.IonosCloudMachineList{}
+	if err := c.client.List(ctx, machineList, listOpts...); err != nil {
+		return nil, err
+	}
+	return machineList.Items, nil
+}
+
 // Location is a shortcut for getting the location used by the IONOS Cloud cluster IP block.
 func (c *Cluster) Location() string {
 	return c.IonosCluster.Spec.Location

scope/cluster_test.go

@@ -23,8 +23,11 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	infrav1 "github.com/ionos-cloud/cluster-api-provider-ionoscloud/api/v1alpha1"
@@ -162,3 +165,107 @@ func TestCluster_GetControlPlaneEndpointIP(t *testing.T) {
 		})
 	}
 }
+
+func TestClusterListMachines(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, infrav1.AddToScheme(scheme))
+
+	const clusterName = "test-cluster"
+
+	makeLabels := func(clusterName string, additionalLabels map[string]string) map[string]string {
+		if additionalLabels == nil {
+			return map[string]string{clusterv1.ClusterNameLabel: clusterName}
+		}
+
+		additionalLabels[clusterv1.ClusterNameLabel] = clusterName
+		return additionalLabels
+	}
+
+	tests := []struct {
+		name           string
+		initialObjects []client.Object
+		searchLabels   client.MatchingLabels
+		expectedNames  sets.Set[string]
+	}{{
+		name: "List all machines for a cluster",
+		initialObjects: []client.Object{
+			buildMachineWithLabel("machine-1", makeLabels(clusterName, nil)),
+			buildMachineWithLabel("machine-2", makeLabels(clusterName, nil)),
+			buildMachineWithLabel("machine-3", makeLabels(clusterName, nil)),
+		},
+		searchLabels:  client.MatchingLabels{},
+		expectedNames: sets.New("machine-1", "machine-2", "machine-3"),
+	}, {
+		name: "List only machines with specific labels",
+		initialObjects: []client.Object{
+			buildMachineWithLabel("machine-1", makeLabels(clusterName, map[string]string{"foo": "bar"})),
+			buildMachineWithLabel("machine-2", makeLabels(clusterName, map[string]string{"foo": "bar"})),
+			buildMachineWithLabel("machine-3", makeLabels(clusterName, nil)),
+		},
+		searchLabels: client.MatchingLabels{
+			"foo": "bar",
+		},
+		expectedNames: sets.New("machine-1", "machine-2"),
+	}, {
+		name: "List no machines",
+		initialObjects: []client.Object{
+			buildMachineWithLabel("machine-1", makeLabels(clusterName, map[string]string{"foo": "notbar"})),
+			buildMachineWithLabel("machine-2", makeLabels(clusterName, map[string]string{"foo": "notbar"})),
+			buildMachineWithLabel("machine-3", makeLabels(clusterName, map[string]string{"foo": "notbar"})),
+		},
+		searchLabels:  makeLabels(clusterName, map[string]string{"foo": "bar"}),
+		expectedNames: sets.New[string](),
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			params := ClusterParams{
+				Cluster: &clusterv1.Cluster{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      clusterName,
+						Namespace: metav1.NamespaceDefault,
+						Labels: map[string]string{
+							clusterv1.ClusterNameLabel: clusterName,
+						},
+					},
+				},
+				IonosCluster: &infrav1.IonosCloudCluster{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test-ionos-cluster",
+						Namespace: metav1.NamespaceDefault,
+						Labels: map[string]string{
+							clusterv1.ClusterNameLabel: clusterName,
+						},
+					},
+					Status: infrav1.IonosCloudClusterStatus{},
+				},
+			}
+
+			cl := fake.NewClientBuilder().WithScheme(scheme).
+				WithObjects(test.initialObjects...).Build()
+
+			params.Client = cl
+			cs, err := NewCluster(params)
+			require.NoError(t, err)
+			require.NotNil(t, cs)
+
+			machines, err := cs.ListMachines(context.Background(), test.searchLabels)
+			require.NoError(t, err)
+			require.Len(t, machines, len(test.expectedNames))
+
+			for _, m := range machines {
+				require.Contains(t, test.expectedNames, m.Name)
+			}
+		})
+	}
+}
+
+func buildMachineWithLabel(name string, labels map[string]string) *infrav1.IonosCloudMachine {
+	return &infrav1.IonosCloudMachine{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: metav1.NamespaceDefault,
+			Labels:    labels,
+		},
+	}
+}

scope/machine.go

@@ -122,24 +122,12 @@ func (m *Machine) CountMachines(ctx context.Context, machineLabels client.Matchi
 	return len(machines), err
 }
 
-// ListMachines returns a list of IonosCloudMachines in the same namespace and with the same cluster label.
-// With machineLabels, additional search labels can be provided.
+// ListMachines is a convenience wrapper function for the Cluster.ListMachines function.
 func (m *Machine) ListMachines(
 	ctx context.Context,
 	machineLabels client.MatchingLabels,
 ) ([]infrav1.IonosCloudMachine, error) {
-	if machineLabels == nil {
-		machineLabels = client.MatchingLabels{}
-	}
-
-	machineLabels[clusterv1.ClusterNameLabel] = m.ClusterScope.Cluster.Name
-	listOpts := []client.ListOption{client.InNamespace(m.IonosMachine.Namespace), machineLabels}
-
-	machineList := &infrav1.IonosCloudMachineList{}
-	if err := m.client.List(ctx, machineList, listOpts...); err != nil {
-		return nil, err
-	}
-	return machineList.Items, nil
+	return m.ClusterScope.ListMachines(ctx, machineLabels)
 }
 
 // FindLatestMachine returns the latest IonosCloudMachine in the same namespace
@@ -151,23 +139,17 @@ func (m *Machine) FindLatestMachine(
 	ctx context.Context,
 	matchLabels client.MatchingLabels,
 ) (*infrav1.IonosCloudMachine, error) {
-	if matchLabels == nil {
-		matchLabels = client.MatchingLabels{}
-	}
-
-	matchLabels[clusterv1.ClusterNameLabel] = m.ClusterScope.Cluster.Name
-	listOpts := []client.ListOption{client.InNamespace(m.IonosMachine.Namespace), matchLabels}
-
-	machineList := &infrav1.IonosCloudMachineList{}
-	if err := m.client.List(ctx, machineList, listOpts...); err != nil {
+	machines, err := m.ClusterScope.ListMachines(ctx, matchLabels)
+	if err != nil {
 		return nil, err
 	}
-	if len(machineList.Items) <= 1 {
+
+	if len(machines) <= 1 {
 		return nil, nil
 	}
 
-	latestMachine := machineList.Items[0]
-	for _, machine := range machineList.Items {
+	latestMachine := machines[0]
+	for _, machine := range machines {
 		if !machine.CreationTimestamp.Before(&latestMachine.CreationTimestamp) && machine.Name != m.IonosMachine.Name {
 			latestMachine = machine
 		}

scope/machine_test.go

@@ -37,10 +37,12 @@ func exampleParams(t *testing.T) MachineParams {
 	if err := infrav1.AddToScheme(scheme.Scheme); err != nil {
 		require.NoError(t, err, "could not construct params")
 	}
+	cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()
 	return MachineParams{
-		Client:  fake.NewClientBuilder().WithScheme(scheme.Scheme).Build(),
+		Client:  cl,
 		Machine: &clusterv1.Machine{},
 		ClusterScope: &Cluster{
+			client:  cl,
 			Cluster: &clusterv1.Cluster{},
 		},
 		IonosMachine: &infrav1.IonosCloudMachine{},