From 929af4f73496c3e091a66f5db746f2e63b56b374 Mon Sep 17 00:00:00 2001 From: Jeffrey Koehler Date: Sun, 29 Oct 2023 22:06:14 -0500 Subject: [PATCH 1/3] Add check for owner reference in backup sync, removing if missing Signed-off-by: Jeffrey Koehler --- changelogs/unreleased/7031-deefdragon | 1 + pkg/controller/backup_sync_controller.go | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 changelogs/unreleased/7031-deefdragon diff --git a/changelogs/unreleased/7031-deefdragon b/changelogs/unreleased/7031-deefdragon new file mode 100644 index 0000000000..430b8c0560 --- /dev/null +++ b/changelogs/unreleased/7031-deefdragon @@ -0,0 +1 @@ +Fix #7031. Added check for Owner References when synchronizing backups, removing references that are not found. \ No newline at end of file diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go index 2fb61d6032..3b2c758ba3 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/pkg/controller/backup_sync_controller.go @@ -29,6 +29,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -171,6 +172,27 @@ func (b *backupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) } backup.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(backup.Spec.StorageLocation) + //check for the backup schedule. If it does not exist, remove it. + listedReferences := backup.ObjectMeta.OwnerReferences + foundReferences := make([]metav1.OwnerReference, 0) + for _, v := range listedReferences { + schedule := new(velerov1api.Schedule) + err := b.client.Get(ctx, types.NamespacedName{ + Name: v.Name, + Namespace: backup.Namespace, + }, schedule) + switch { + case err != nil && apierrors.IsNotFound(err): + log.Debug("Removing missing schedule ownership reference from backup") + case err != nil && !apierrors.IsNotFound(err): + log.WithError(errors.WithStack(err)).Error("Error finding ownership reference schedule") + fallthrough + default: + foundReferences = append(foundReferences, v) + } + } + backup.ObjectMeta.OwnerReferences = foundReferences + // attempt to create backup custom resource via API err = b.client.Create(ctx, backup, &client.CreateOptions{}) switch { From 8eec6865d171da06e9e4911e686246e10ea732bf Mon Sep 17 00:00:00 2001 From: Jeffrey Koehler Date: Tue, 31 Oct 2023 23:01:50 -0500 Subject: [PATCH 2/3] Check only schedules, and verify UIDs are the same Signed-off-by: Jeffrey Koehler --- changelogs/unreleased/7031-deefdragon | 1 - changelogs/unreleased/7032-deefdragon | 1 + pkg/controller/backup_sync_controller.go | 35 +++++++++++++++--------- 3 files changed, 23 insertions(+), 14 deletions(-) delete mode 100644 changelogs/unreleased/7031-deefdragon create mode 100644 changelogs/unreleased/7032-deefdragon diff --git a/changelogs/unreleased/7031-deefdragon b/changelogs/unreleased/7031-deefdragon deleted file mode 100644 index 430b8c0560..0000000000 --- a/changelogs/unreleased/7031-deefdragon +++ /dev/null @@ -1 +0,0 @@ -Fix #7031. Added check for Owner References when synchronizing backups, removing references that are not found. \ No newline at end of file diff --git a/changelogs/unreleased/7032-deefdragon b/changelogs/unreleased/7032-deefdragon new file mode 100644 index 0000000000..710ed3a110 --- /dev/null +++ b/changelogs/unreleased/7032-deefdragon @@ -0,0 +1 @@ +Fix #6857. Added check for matching Owner References when synchronizing backups, removing references that are not found/have mismatched uid. \ No newline at end of file diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go index 3b2c758ba3..e199390388 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/pkg/controller/backup_sync_controller.go @@ -172,24 +172,33 @@ func (b *backupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) } backup.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(backup.Spec.StorageLocation) - //check for the backup schedule. If it does not exist, remove it. + //check for the ownership references. If they do not exist, remove them. listedReferences := backup.ObjectMeta.OwnerReferences foundReferences := make([]metav1.OwnerReference, 0) for _, v := range listedReferences { - schedule := new(velerov1api.Schedule) - err := b.client.Get(ctx, types.NamespacedName{ - Name: v.Name, - Namespace: backup.Namespace, - }, schedule) - switch { - case err != nil && apierrors.IsNotFound(err): - log.Debug("Removing missing schedule ownership reference from backup") - case err != nil && !apierrors.IsNotFound(err): - log.WithError(errors.WithStack(err)).Error("Error finding ownership reference schedule") - fallthrough + switch v.Kind { + case "Schedule": + + schedule := new(velerov1api.Schedule) + err := b.client.Get(ctx, types.NamespacedName{ + Name: v.Name, + Namespace: backup.Namespace, + }, schedule) + switch { + case err != nil && apierrors.IsNotFound(err): + log.Warn("Removing missing schedule ownership reference from backup") + continue + case schedule.UID != v.UID: + log.Warnf("Removing schedule ownership reference with mismatched UIDs. Expected %s, got %s", v.UID, schedule.UID) + continue + case err != nil && !apierrors.IsNotFound(err): + log.WithError(errors.WithStack(err)).Error("Error finding schedule ownership reference, keeping schedule on backup") + } default: - foundReferences = append(foundReferences, v) + log.Warnf("Unable to check ownership reference for unknown kind, %s", v.Kind) } + + foundReferences = append(foundReferences, v) } backup.ObjectMeta.OwnerReferences = foundReferences From 292aa34a48de3513af2d18022b50c3389fa4e687 Mon Sep 17 00:00:00 2001 From: Jeffrey Koehler Date: Thu, 16 Nov 2023 03:57:36 -0600 Subject: [PATCH 3/3] move filtering code to seperate method, add tests Signed-off-by: Jeffrey Koehler --- pkg/controller/backup_sync_controller.go | 58 +++--- pkg/controller/backup_sync_controller_test.go | 170 ++++++++++++++++++ 2 files changed, 200 insertions(+), 28 deletions(-) diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go index e199390388..91b519b572 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/pkg/controller/backup_sync_controller.go @@ -173,34 +173,7 @@ func (b *backupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) backup.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(backup.Spec.StorageLocation) //check for the ownership references. If they do not exist, remove them. - listedReferences := backup.ObjectMeta.OwnerReferences - foundReferences := make([]metav1.OwnerReference, 0) - for _, v := range listedReferences { - switch v.Kind { - case "Schedule": - - schedule := new(velerov1api.Schedule) - err := b.client.Get(ctx, types.NamespacedName{ - Name: v.Name, - Namespace: backup.Namespace, - }, schedule) - switch { - case err != nil && apierrors.IsNotFound(err): - log.Warn("Removing missing schedule ownership reference from backup") - continue - case schedule.UID != v.UID: - log.Warnf("Removing schedule ownership reference with mismatched UIDs. Expected %s, got %s", v.UID, schedule.UID) - continue - case err != nil && !apierrors.IsNotFound(err): - log.WithError(errors.WithStack(err)).Error("Error finding schedule ownership reference, keeping schedule on backup") - } - default: - log.Warnf("Unable to check ownership reference for unknown kind, %s", v.Kind) - } - - foundReferences = append(foundReferences, v) - } - backup.ObjectMeta.OwnerReferences = foundReferences + backup.ObjectMeta.OwnerReferences = b.filterBackupOwnerReferences(ctx, backup, log) // attempt to create backup custom resource via API err = b.client.Create(ctx, backup, &client.CreateOptions{}) @@ -317,6 +290,35 @@ func (b *backupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil } +func (b *backupSyncReconciler) filterBackupOwnerReferences(ctx context.Context, backup *velerov1api.Backup, log logrus.FieldLogger) []metav1.OwnerReference { + listedReferences := backup.ObjectMeta.OwnerReferences + foundReferences := make([]metav1.OwnerReference, 0) + for _, v := range listedReferences { + switch v.Kind { + case "Schedule": + schedule := new(velerov1api.Schedule) + err := b.client.Get(ctx, types.NamespacedName{ + Name: v.Name, + Namespace: backup.Namespace, + }, schedule) + switch { + case err != nil && apierrors.IsNotFound(err): + log.Warnf("Removing missing schedule ownership reference %s/%s from backup", backup.Namespace, v.Name) + continue + case schedule.UID != v.UID: + log.Warnf("Removing schedule ownership reference with mismatched UIDs. Expected %s, got %s", v.UID, schedule.UID) + continue + case err != nil && !apierrors.IsNotFound(err): + log.WithError(errors.WithStack(err)).Error("Error finding schedule ownership reference, keeping schedule on backup") + } + default: + log.Warnf("Unable to check ownership reference for unknown kind, %s", v.Kind) + } + foundReferences = append(foundReferences, v) + } + return foundReferences +} + // SetupWithManager is used to setup controller and its watching sources. func (b *backupSyncReconciler) SetupWithManager(mgr ctrl.Manager) error { backupSyncSource := kube.NewPeriodicalEnqueueSource( diff --git a/pkg/controller/backup_sync_controller_test.go b/pkg/controller/backup_sync_controller_test.go index 86ca3227b1..2fcbc94a70 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/pkg/controller/backup_sync_controller_test.go @@ -724,4 +724,174 @@ var _ = Describe("Backup Sync Reconciler", func() { Expect(testObjList).To(BeEquivalentTo(locationList)) }) + + When("testing validateOwnerReferences", func() { + + testCases := []struct { + name string + backup *velerov1api.Backup + toCreate []ctrlClient.Object + expectedReferences []metav1.OwnerReference + }{ + { + name: "handles empty owner references", + backup: &velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{}, + }, + }, + expectedReferences: []metav1.OwnerReference{}, + }, + { + name: "handles missing schedule", + backup: &velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Schedule", + Name: "some name", + }, + }, + }, + }, + expectedReferences: []metav1.OwnerReference{}, + }, + { + name: "handles existing reference", + backup: &velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Schedule", + Name: "existing-schedule", + }, + }, + Namespace: "test-namespace", + }, + }, + toCreate: []ctrlClient.Object{ + &velerov1api.Schedule{ + ObjectMeta: metav1.ObjectMeta{ + Name: "existing-schedule", + Namespace: "test-namespace", + }, + }, + }, + expectedReferences: []metav1.OwnerReference{ + { + Kind: "Schedule", + Name: "existing-schedule", + }, + }, + }, + { + name: "handles existing mismatched UID", + backup: &velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Schedule", + Name: "existing-schedule", + UID: "backup-UID", + }, + }, + Namespace: "test-namespace", + }, + }, + toCreate: []ctrlClient.Object{ + &velerov1api.Schedule{ + ObjectMeta: metav1.ObjectMeta{ + Name: "existing-schedule", + Namespace: "test-namespace", + UID: "schedule-UID", + }, + }, + }, + expectedReferences: []metav1.OwnerReference{}, + }, + { + name: "handles multiple references", + backup: &velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Schedule", + Name: "existing-schedule", + UID: "1", + }, + { + Kind: "Schedule", + Name: "missing-schedule", + UID: "2", + }, + { + Kind: "Schedule", + Name: "mismatched-uid-schedule", + UID: "3", + }, + { + Kind: "Schedule", + Name: "another-existing-schedule", + UID: "4", + }, + }, + Namespace: "test-namespace", + }, + }, + toCreate: []ctrlClient.Object{ + &velerov1api.Schedule{ + ObjectMeta: metav1.ObjectMeta{ + Name: "existing-schedule", + Namespace: "test-namespace", + UID: "1", + }, + }, + &velerov1api.Schedule{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mismatched-uid-schedule", + Namespace: "test-namespace", + UID: "not-3", + }, + }, + &velerov1api.Schedule{ + ObjectMeta: metav1.ObjectMeta{ + Name: "another-existing-schedule", + Namespace: "test-namespace", + UID: "4", + }, + }, + }, + expectedReferences: []metav1.OwnerReference{ + { + Kind: "Schedule", + Name: "existing-schedule", + UID: "1", + }, + { + Kind: "Schedule", + Name: "another-existing-schedule", + UID: "4", + }, + }, + }, + } + for _, test := range testCases { + test := test + It(test.name, func() { + logger := velerotest.NewLogger() + b := backupSyncReconciler{ + client: ctrlfake.NewClientBuilder().Build(), + } + + //create all required schedules as needed. + for _, creatable := range test.toCreate { + err := b.client.Create(context.Background(), creatable) + Expect(err).ShouldNot(HaveOccurred()) + } + + references := b.filterBackupOwnerReferences(context.Background(), test.backup, logger) + Expect(references).To(BeEquivalentTo(test.expectedReferences)) + }) + } + }) })