From 9a1d2e6eb0b06dd5e5631035c28f872b0e0238e4 Mon Sep 17 00:00:00 2001 From: AftAb-25 Date: Wed, 12 Aug 2026 12:42:58 +0530 Subject: [PATCH] Fix switch case ordering bug in `filterBackupOwnerReferences` (#10161) * Fix switch case ordering in filterBackupOwnerReferences (Issue #10160) When client.Get returns a transient (non-NotFound) error, the previous case ordering caused the UID mismatch case to fire against a zero-value struct, silently dropping the owner reference and logging a misleading 'mismatched UIDs' warning instead of the intended error log. Fix: move the general error handler before the UID mismatch check so it is evaluated while err is still relevant. The UID check now only runs when err == nil (i.e. the Schedule was successfully fetched). Also add a test case that injects a transient Get error via the fake client interceptor to verify the owner reference is preserved. Signed-off-by: aftab * Add changelog for #10160 Signed-off-by: aftab --------- Signed-off-by: aftab --- changelogs/unreleased/10161-AftAb-25 | 1 + pkg/controller/backup_sync_controller.go | 4 +- pkg/controller/backup_sync_controller_test.go | 44 +++++++++++++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/10161-AftAb-25 diff --git a/changelogs/unreleased/10161-AftAb-25 b/changelogs/unreleased/10161-AftAb-25 new file mode 100644 index 000000000..f960a13df --- /dev/null +++ b/changelogs/unreleased/10161-AftAb-25 @@ -0,0 +1 @@ +Fixed a bug in the backup sync controller where transient API errors could cause backups to incorrectly lose their schedule owner references. diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go index 07b5b460f..ce9af902f 100644 --- a/pkg/controller/backup_sync_controller.go +++ b/pkg/controller/backup_sync_controller.go @@ -271,11 +271,11 @@ func (b *backupSyncReconciler) filterBackupOwnerReferences(ctx context.Context, case err != nil && apierrors.IsNotFound(err): log.Warnf("Removing missing schedule ownership reference %s/%s from backup", backup.Namespace, v.Name) continue + case err != nil && !apierrors.IsNotFound(err): + log.WithError(errors.WithStack(err)).Error("Error finding schedule ownership reference, keeping schedule on backup") 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) diff --git a/pkg/controller/backup_sync_controller_test.go b/pkg/controller/backup_sync_controller_test.go index fe440ff09..fbfe65457 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/pkg/controller/backup_sync_controller_test.go @@ -36,6 +36,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" ctrlClient "sigs.k8s.io/controller-runtime/pkg/client" ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" @@ -914,4 +915,47 @@ var _ = Describe("Backup Sync Reconciler", func() { }) } }) + + It("filterBackupOwnerReferences preserves owner reference on transient API error", func() { + // This test verifies the fix for the switch case ordering bug: + // When client.Get returns a non-NotFound error (e.g. transient API failure), + // the owner reference must be kept on the backup rather than silently dropped + // due to an incorrect UID comparison against a zero-value struct. + scheduleUID := types.UID("schedule-uid-1") + backup := &velerov1api.Backup{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-backup", + Namespace: "test-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Schedule", + Name: "my-schedule", + UID: scheduleUID, + }, + }, + }, + } + + // Build a fake client that returns a generic (non-NotFound) error on Get, + // simulating a transient API server failure. + transientErr := fmt.Errorf("transient connection error") + fakeClient := ctrlfake.NewClientBuilder(). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, c ctrlClient.WithWatch, key ctrlClient.ObjectKey, obj ctrlClient.Object, opts ...ctrlClient.GetOption) error { + return transientErr + }, + }). + Build() + + b := backupSyncReconciler{ + client: fakeClient, + } + + logger := velerotest.NewLogger() + references := b.filterBackupOwnerReferences(context.Background(), backup, logger) + + // The owner reference must be preserved when a transient error occurs. + Expect(references).To(HaveLen(1)) + Expect(references[0].UID).To(Equal(scheduleUID)) + }) })