Skip to content

Commit

Permalink
Merge pull request #1478 from Nordix/metal3data-deleteable/max
Browse files Browse the repository at this point in the history 
🐛 Protect Metal3Data and Metal3DataClaim with Finalizers
  • Loading branch information
@metal3-io-bot
metal3-io-bot authored May 28, 2024
2 parents 69ea23a + acb35f3 commit ee02e92
Show file tree
Hide file tree
Showing 6 changed files with 76 additions and 46 deletions.
2 changes: 1 addition & 1 deletion baremetal/metal3data_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1437,7 +1437,7 @@ func (m *DataManager) getM3Machine(ctx context.Context, m3dt *infrav1.Metal3Data
}
// not matching on UID since when pivoting it might change
// Not matching on API version as this might change
if ownerRef.Kind == "Metal3Machine" &&
if ownerRef.Kind == metal3MachineKind &&
oGV.Group == infrav1.GroupVersion.Group {
metal3MachineName = ownerRef.Name
break
Expand Down
40 changes: 25 additions & 15 deletions baremetal/metal3datatemplate_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package baremetal

import (
"context"
"fmt"
"strconv"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -274,7 +273,7 @@ func (m *DataTemplateManager) createData(ctx context.Context,
if err != nil {
return indexes, err
}
if ownerRef.Kind == "Metal3Machine" &&
if ownerRef.Kind == metal3MachineKind &&
aGV.Group == infrav1.GroupVersion.Group {
m3mUID = ownerRef.UID
m3mName = ownerRef.Name
Expand Down Expand Up @@ -308,16 +307,17 @@ func (m *DataTemplateManager) createData(ctx context.Context,
m.Log.Info("Index", "Claim", dataClaim.Name, "index", claimIndex)

// Create the Metal3Data object, with an Owner ref to the Metal3Machine
// (curOwnerRef) and to the Metal3DataTemplate
// (curOwnerRef) and to the Metal3DataTemplate. Also add a finalizer.
dataObject := &infrav1.Metal3Data{
TypeMeta: metav1.TypeMeta{
Kind: "Metal3Data",
APIVersion: infrav1.GroupVersion.String(),
},
ObjectMeta: metav1.ObjectMeta{
Name: dataName,
Namespace: m.DataTemplate.Namespace,
Labels: dataClaim.Labels,
Name: dataName,
Namespace: m.DataTemplate.Namespace,
Finalizers: []string{infrav1.DataClaimFinalizer},
Labels: dataClaim.Labels,
OwnerReferences: []metav1.OwnerReference{
{
Controller: ptr.To(true),
Expand All @@ -334,7 +334,7 @@ func (m *DataTemplateManager) createData(ctx context.Context,
},
{
APIVersion: dataClaim.APIVersion,
Kind: "Metal3Machine",
Kind: metal3MachineKind,
Name: m3mName,
UID: m3mUID,
},
Expand Down Expand Up @@ -376,18 +376,18 @@ func (m *DataTemplateManager) createData(ctx context.Context,
return indexes, nil
}

// DeleteDatas deletes old secrets.
// deleteData deletes the Metal3DataClaim and marks the Metal3Data for deletion.
func (m *DataTemplateManager) deleteData(ctx context.Context,
dataClaim *infrav1.Metal3DataClaim, indexes map[int]string,
) (map[int]string, error) {
var dataName string
m.Log.Info("Deleting Claim", "Metal3DataClaim", dataClaim.Name)
m.Log.Info("Deleting Metal3DataClaim", "Metal3DataClaim", dataClaim.Name)

dataClaimIndex, ok := m.DataTemplate.Status.Indexes[dataClaim.Name]
if ok {
// Try to get the Metal3Data. if it succeeds, delete it
tmpM3Data := &infrav1.Metal3Data{}

var dataName string
if m.DataTemplate.Spec.TemplateReference != "" {
dataName = m.DataTemplate.Spec.TemplateReference + "-" + strconv.Itoa(dataClaimIndex)
} else {
Expand All @@ -403,26 +403,36 @@ func (m *DataTemplateManager) deleteData(ctx context.Context,
dataClaim.Status.ErrorMessage = ptr.To("Failed to get associated Metal3Data object")
return indexes, err
} else if err == nil {
// Delete the secret with metadata
fmt.Println(tmpM3Data.Name)
err = m.client.Delete(ctx, tmpM3Data)
// Remove the finalizer
tmpM3Data.Finalizers = Filter(tmpM3Data.Finalizers,
infrav1.DataClaimFinalizer,
)
err = updateObject(ctx, m.client, tmpM3Data)
if err != nil && !apierrors.IsNotFound(err) {
m.Log.Info("Unable to remove finalizer from Metal3Data", "Metal3Data", tmpM3Data.Name)
return indexes, err
}
// Delete the Metal3Data
err = deleteObject(ctx, m.client, tmpM3Data)
if err != nil && !apierrors.IsNotFound(err) {
dataClaim.Status.ErrorMessage = ptr.To("Failed to delete associated Metal3Data object")
return indexes, err
}
m.Log.Info("Deleted Metal3Data", "Metal3Data", tmpM3Data.Name)
}
}

dataClaim.Status.RenderedData = nil
dataClaim.Finalizers = Filter(dataClaim.Finalizers,
infrav1.DataClaimFinalizer,
)

m.Log.Info("Deleted Claim", "Metal3DataClaim", dataClaim.Name)

if ok {
delete(m.DataTemplate.Status.Indexes, dataClaim.Name)
delete(indexes, dataClaimIndex)
}

m.Log.Info("Deleted Metal3DataClaim", "Metal3DataClaim", dataClaim.Name)
m.updateStatusTimestamp()
return indexes, nil
}
34 changes: 17 additions & 17 deletions baremetal/metal3datatemplate_manager_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,24 +299,24 @@ var _ = Describe("Metal3DataTemplate manager", func() {
dataClaims: []*infrav1.Metal3DataClaim{
{
ObjectMeta: metav1.ObjectMeta{
Name: "abc",
Name: "claim-without-status",
Namespace: namespaceName,
},
Spec: infrav1.Metal3DataClaimSpec{
Template: corev1.ObjectReference{
Name: "abc",
Name: templateMeta.Name,
Namespace: namespaceName,
},
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "abcd",
Name: "orphaned-claim",
Namespace: namespaceName,
},
Spec: infrav1.Metal3DataClaimSpec{
Template: corev1.ObjectReference{
Name: "abcd",
Name: "other-template",
Namespace: namespaceName,
},
},
Expand All @@ -329,12 +329,12 @@ var _ = Describe("Metal3DataTemplate manager", func() {
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "abce",
Name: "claim-with-status",
Namespace: namespaceName,
},
Spec: infrav1.Metal3DataClaimSpec{
Template: corev1.ObjectReference{
Name: "abc",
Name: templateMeta.Name,
Namespace: namespaceName,
},
},
Expand All @@ -347,14 +347,14 @@ var _ = Describe("Metal3DataTemplate manager", func() {
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "abcf",
Name: "deleting-claim",
Namespace: namespaceName,
DeletionTimestamp: &timeNow,
Finalizers: []string{"ipclaim.ipam.metal3.io"},
},
Spec: infrav1.Metal3DataClaimSpec{
Template: corev1.ObjectReference{
Name: "abc",
Name: templateMeta.Name,
Namespace: namespaceName,
},
},
Expand All @@ -374,11 +374,11 @@ var _ = Describe("Metal3DataTemplate manager", func() {
},
Spec: infrav1.Metal3DataSpec{
Template: corev1.ObjectReference{
Name: "abc",
Name: templateMeta.Name,
Namespace: namespaceName,
},
Claim: corev1.ObjectReference{
Name: "abc",
Name: "claim-without-status",
Namespace: namespaceName,
},
Index: 0,
Expand All @@ -391,37 +391,37 @@ var _ = Describe("Metal3DataTemplate manager", func() {
},
Spec: infrav1.Metal3DataSpec{
Template: corev1.ObjectReference{
Name: "abc",
Name: templateMeta.Name,
Namespace: namespaceName,
},
Claim: corev1.ObjectReference{
Name: "abce",
Name: "claim-with-status",
Namespace: namespaceName,
},
Index: 1,
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "abc-3",
Name: "data-to-delete",
Namespace: namespaceName,
},
Spec: infrav1.Metal3DataSpec{
Template: corev1.ObjectReference{
Name: "abc",
Name: templateMeta.Name,
Namespace: namespaceName,
},
Claim: corev1.ObjectReference{
Name: "abcf",
Name: "deleting-claim",
Namespace: namespaceName,
},
Index: 3,
},
},
},
expectedIndexes: map[string]int{
"abc": 0,
"abce": 1,
"claim-without-status": 0,
"claim-with-status": 1,
},
expectedNbIndexes: 2,
}),
Expand Down
12 changes: 12 additions & 0 deletions baremetal/metal3machine_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1534,6 +1534,9 @@ func (m *MachineManager) AssociateM3Metadata(ctx context.Context) error {
ObjectMeta: metav1.ObjectMeta{
Name: m.Metal3Machine.Name,
Namespace: m.Metal3Machine.Namespace,
Finalizers: []string{
infrav1.MachineFinalizer,
},
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: m.Metal3Machine.APIVersion,
Expand Down Expand Up @@ -1663,6 +1666,15 @@ func (m *MachineManager) DissociateM3Metadata(ctx context.Context) error {
return nil
}

metal3DataClaim.Finalizers = Filter(metal3DataClaim.Finalizers,
infrav1.MachineFinalizer,
)
err = updateObject(ctx, m.client, metal3DataClaim)
if err != nil && !apierrors.IsNotFound(err) {
m.Log.Info("Unable to remove finalizers from Metal3DataClaim", "Metal3DataClaim", metal3DataClaim.Name)
return err
}

return deleteObject(ctx, m.client, metal3DataClaim)
}

Expand Down
4 changes: 3 additions & 1 deletion baremetal/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@ import (
const (
// metal3SecretType defines the type of secret created by metal3.
metal3SecretType corev1.SecretType = "infrastructure.cluster.x-k8s.io/secret"
// metal3MachineKind is the Kind of the Metal3Machine.
metal3MachineKind = "Metal3Machine"
)

// Filter filters a list for a string.
// Filter filters out occurrences of strToFilter from list and returns the new list.
func Filter(list []string, strToFilter string) (newList []string) {
for _, item := range list {
if item != strToFilter {
Expand Down
30 changes: 18 additions & 12 deletions controllers/metal3data_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,29 +60,29 @@ func (r *Metal3DataReconciler) Reconcile(ctx context.Context, req ctrl.Request)
metadataLog := r.Log.WithName(dataControllerName).WithValues("metal3-data", req.NamespacedName)

// Fetch the Metal3Data instance.
capm3Metadata := &infrav1.Metal3Data{}
metal3Data := &infrav1.Metal3Data{}

if err := r.Client.Get(ctx, req.NamespacedName, capm3Metadata); err != nil {
if err := r.Client.Get(ctx, req.NamespacedName, metal3Data); err != nil {
if apierrors.IsNotFound(err) {
return ctrl.Result{}, nil
}
return ctrl.Result{}, err
}
helper, err := patch.NewHelper(capm3Metadata, r.Client)
helper, err := patch.NewHelper(metal3Data, r.Client)
if err != nil {
return ctrl.Result{}, errors.Wrap(err, "failed to init patch helper")
}
// Always patch capm3Data exiting this function so we can persist any Metal3Data changes.
// Always patch Metal3Data exiting this function so we can persist any changes.
defer func() {
err := helper.Patch(ctx, capm3Metadata)
err := helper.Patch(ctx, metal3Data)
if err != nil {
metadataLog.Info("failed to Patch Metal3Data")
}
}()

// Fetch the Cluster.
cluster, err := util.GetClusterFromMetadata(ctx, r.Client, capm3Metadata.ObjectMeta)
if capm3Metadata.ObjectMeta.DeletionTimestamp.IsZero() {
cluster, err := util.GetClusterFromMetadata(ctx, r.Client, metal3Data.ObjectMeta)
if metal3Data.ObjectMeta.DeletionTimestamp.IsZero() {
if err != nil {
metadataLog.Info("Metal3Data is missing cluster label or cluster does not exist")
return ctrl.Result{}, nil
Expand All @@ -97,21 +97,27 @@ func (r *Metal3DataReconciler) Reconcile(ctx context.Context, req ctrl.Request)
metadataLog = metadataLog.WithValues("cluster", cluster.Name)

// Return early if the Metadata or Cluster is paused.
if annotations.IsPaused(cluster, capm3Metadata) {
if annotations.IsPaused(cluster, metal3Data) {
metadataLog.Info("reconciliation is paused for this object")
return ctrl.Result{Requeue: true, RequeueAfter: requeueAfter}, nil
}
}

// Create a helper for managing the metadata object.
metadataMgr, err := r.ManagerFactory.NewDataManager(capm3Metadata, metadataLog)
metadataMgr, err := r.ManagerFactory.NewDataManager(metal3Data, metadataLog)
if err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to create helper for managing the Metal3Data")
}

// Handle deleted metadata
if !capm3Metadata.ObjectMeta.DeletionTimestamp.IsZero() {
return r.reconcileDelete(ctx, metadataMgr)
// Handle deletion of Metal3Data
if !metal3Data.ObjectMeta.DeletionTimestamp.IsZero() {
// Check if the Metal3DataClaim is gone. We cannot clean up until it is.
err := r.Client.Get(ctx, types.NamespacedName{Name: metal3Data.Spec.Claim.Name, Namespace: metal3Data.Spec.Claim.Namespace}, &infrav1.Metal3DataClaim{})
if err != nil && apierrors.IsNotFound(err) {
return r.reconcileDelete(ctx, metadataMgr)
}
// We were unable to determine if the Metal3DataClaim is gone
return ctrl.Result{}, err
}

// Handle non-deleted machines
Expand Down

0 comments on commit ee02e92

Please sign in to comment.