diff --git a/api/v1alpha1/ionoscloudcluster_types.go b/api/v1alpha1/ionoscloudcluster_types.go index 3709855b..103118a1 100644 --- a/api/v1alpha1/ionoscloudcluster_types.go +++ b/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" diff --git a/internal/controller/ionoscloudcluster_controller.go b/internal/controller/ionoscloudcluster_controller.go index 5f47387c..1d0c317e 100644 --- a/internal/controller/ionoscloudcluster_controller.go +++ b/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 } diff --git a/internal/controller/ionoscloudmachine_controller.go b/internal/controller/ionoscloudmachine_controller.go index e9dd5580..a3ba2961 100644 --- a/internal/controller/ionoscloudmachine_controller.go +++ b/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 } } diff --git a/internal/controller/util.go b/internal/controller/util.go index 1c091d37..7a60a22e 100644 --- a/internal/controller/util.go +++ b/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) +} diff --git a/internal/service/cloud/network.go b/internal/service/cloud/network.go index 726068f8..e2e25ad7 100644 --- a/internal/service/cloud/network.go +++ b/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 } diff --git a/scope/cluster.go b/scope/cluster.go index b9dc68c3..daf6a16e 100644 --- a/scope/cluster.go +++ b/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 diff --git a/scope/cluster_test.go b/scope/cluster_test.go index 36602920..45699c32 100644 --- a/scope/cluster_test.go +++ b/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, + }, + } +} diff --git a/scope/machine.go b/scope/machine.go index f714460f..6c681295 100644 --- a/scope/machine.go +++ b/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 } diff --git a/scope/machine_test.go b/scope/machine_test.go index 71b2add4..68a59af1 100644 --- a/scope/machine_test.go +++ b/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{},