From d7aa82d8ed02e5e72c584c32db0f65b5b10333b0 Mon Sep 17 00:00:00 2001
From: Daniel Jiang <jiangd@vmware.com>
Date: Tue, 21 Dec 2021 05:38:13 +0800
Subject: [PATCH] Return the error when getting backup store in backup deletion
 controller (#4465)

Per discussion in
https://github.com/vmware-tanzu/velero/issues/4260#issuecomment-947721686
https://github.com/vmware-tanzu/velero/issues/4260#issuecomment-951347384

return the error to avoid a panic when downloading the backup tarball

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
---
 changelogs/unreleased/4465-reasonerjt         |  1 +
 pkg/controller/backup_deletion_controller.go  |  2 +-
 .../backup_deletion_controller_test.go        | 33 ++++++++++++++++---
 pkg/controller/suite_test.go                  |  8 +++++
 4 files changed, 39 insertions(+), 5 deletions(-)
 create mode 100644 changelogs/unreleased/4465-reasonerjt

diff --git a/changelogs/unreleased/4465-reasonerjt b/changelogs/unreleased/4465-reasonerjt
new file mode 100644
index 000000000..f6f15ec62
--- /dev/null
+++ b/changelogs/unreleased/4465-reasonerjt
@@ -0,0 +1 @@
+Return the error when getting backup store in backup deletion controller
\ No newline at end of file
diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go
index 3e503e81a..90707be70 100644
--- a/pkg/controller/backup_deletion_controller.go
+++ b/pkg/controller/backup_deletion_controller.go
@@ -291,7 +291,7 @@ func (c *backupDeletionController) processRequest(req *velerov1api.DeleteBackupR
 
 	backupStore, err := c.backupStoreGetter.Get(location, pluginManager, log)
 	if err != nil {
-		errs = append(errs, err.Error())
+		return errors.Wrap(err, "error getting the backup store")
 	}
 
 	actions, err := pluginManager.GetDeleteItemActions()
diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go
index 98109d7fe..a03e98eb0 100644
--- a/pkg/controller/backup_deletion_controller_test.go
+++ b/pkg/controller/backup_deletion_controller_test.go
@@ -21,6 +21,7 @@ import (
 	"context"
 	"fmt"
 	"io/ioutil"
+	"strings"
 	"testing"
 	"time"
 
@@ -36,19 +37,20 @@ import (
 	core "k8s.io/client-go/testing"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	"github.com/vmware-tanzu/velero/pkg/builder"
+	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
+	"github.com/vmware-tanzu/velero/pkg/plugin/velero/mocks"
+	"github.com/vmware-tanzu/velero/pkg/volume"
+
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	pkgbackup "github.com/vmware-tanzu/velero/pkg/backup"
-	"github.com/vmware-tanzu/velero/pkg/builder"
 	"github.com/vmware-tanzu/velero/pkg/generated/clientset/versioned/fake"
 	informers "github.com/vmware-tanzu/velero/pkg/generated/informers/externalversions"
 	"github.com/vmware-tanzu/velero/pkg/metrics"
 	persistencemocks "github.com/vmware-tanzu/velero/pkg/persistence/mocks"
 	"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
 	pluginmocks "github.com/vmware-tanzu/velero/pkg/plugin/mocks"
-	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
-	"github.com/vmware-tanzu/velero/pkg/plugin/velero/mocks"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
-	"github.com/vmware-tanzu/velero/pkg/volume"
 )
 
 func TestBackupDeletionControllerProcessQueueItem(t *testing.T) {
@@ -183,6 +185,29 @@ func setupBackupDeletionControllerTest(t *testing.T, objects ...runtime.Object)
 }
 
 func TestBackupDeletionControllerProcessRequest(t *testing.T) {
+	t.Run("failed to get backup store", func(t *testing.T) {
+		backup := builder.ForBackup(velerov1api.DefaultNamespace, "foo").StorageLocation("default").Result()
+		location := &velerov1api.BackupStorageLocation{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: backup.Namespace,
+				Name:      backup.Spec.StorageLocation,
+			},
+			Spec: velerov1api.BackupStorageLocationSpec{
+				Provider: "objStoreProvider",
+				StorageType: velerov1api.StorageType{
+					ObjectStorage: &velerov1api.ObjectStorageLocation{
+						Bucket: "bucket",
+					},
+				},
+			},
+		}
+		td := setupBackupDeletionControllerTest(t, location, backup)
+		td.controller.backupStoreGetter = &fakeErrorBackupStoreGetter{}
+		err := td.controller.processRequest(td.req)
+		assert.NotNil(t, err)
+		assert.True(t, strings.HasPrefix(err.Error(), "error getting the backup store"))
+	})
+
 	t.Run("missing spec.backupName", func(t *testing.T) {
 		td := setupBackupDeletionControllerTest(t)
 		td.req.Spec.BackupName = ""
diff --git a/pkg/controller/suite_test.go b/pkg/controller/suite_test.go
index d4c07b995..b5c5b4d08 100644
--- a/pkg/controller/suite_test.go
+++ b/pkg/controller/suite_test.go
@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	"fmt"
 	"path/filepath"
 	"testing"
 	"time"
@@ -130,6 +131,13 @@ func (t *testEnvironment) stop() error {
 	return env.Stop()
 }
 
+type fakeErrorBackupStoreGetter struct {
+}
+
+func (f *fakeErrorBackupStoreGetter) Get(*velerov1api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) {
+	return nil, fmt.Errorf("some error")
+}
+
 type fakeSingleObjectBackupStoreGetter struct {
 	store persistence.BackupStore
 }