Convert DownloadRequest resource/controller to kubebuilder (#3004)

* Migrate DownloadRequest types to kubebuilder Signed-off-by: Carlisia <carlisia@vmware.com> * Migrate controller to kubebuilder Signed-off-by: Carlisia <carlisia@vmware.com> * Migrate download request cli to kubebuilder Signed-off-by: Carlisia <carlisia@vmware.com> * Format w make update Signed-off-by: Carlisia <carlisia@vmware.com> * Remove download file Signed-off-by: Carlisia <carlisia@vmware.com> * Remove kubebuilder from backup/restore apis Signed-off-by: Carlisia <carlisia@vmware.com> * Fix test description Signed-off-by: Carlisia <carlisia@vmware.com> * Import cleanups Signed-off-by: Carlisia <carlisia@vmware.com> * Refactor for controller runtime version update Signed-off-by: Carlisia <carlisia@vmware.com> * Remove year from the copyright Signed-off-by: Carlisia <carlisia@vmware.com> * Check for expiration regardless of phase Signed-off-by: Carlisia <carlisia@vmware.com> * Fix typos and godoc Signed-off-by: Carlisia <carlisia@vmware.com> * Fix test setup and fix a test case Signed-off-by: Carlisia <carlisia@vmware.com>
2026-01-07 05:46:37 +00:00 · 2021-03-01 10:28:46 -08:00
parent c80ad61bbc
commit 11bfe82342
23 changed files with 542 additions and 818 deletions
--- a/pkg/controller/backup_controller.go
+++ b/pkg/controller/backup_controller.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2020 the Velero contributors.
+Copyright the Velero contributors.

 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
--- a/pkg/controller/backup_storage_location_controller.go
+++ b/pkg/controller/backup_storage_location_controller.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2020 the Velero contributors.
+Copyright the Velero contributors.

 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -60,7 +60,7 @@ func (r *BackupStorageLocationReconciler) Reconcile(ctx context.Context, req ctr
 	locationList, err := storage.ListBackupStorageLocations(r.Ctx, r.Client, req.Namespace)
 	if err != nil {
 		log.WithError(err).Error("No backup storage locations found, at least one is required")
-		return ctrl.Result{Requeue: true}, err
+		return ctrl.Result{}, err
 	}

 	pluginManager := r.NewPluginManager(log)
--- a/pkg/controller/download_request_controller.go
+++ b/pkg/controller/download_request_controller.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2017 the Velero contributors.
+Copyright the Velero contributors.

 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -18,230 +18,154 @@ package controller

 import (
 	"context"
-	"encoding/json"
-	"time"

-	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/clock"
-	"k8s.io/client-go/tools/cache"
-	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/cluster-api/util/patch"
+	ctrl "sigs.k8s.io/controller-runtime"
+	kbclient "sigs.k8s.io/controller-runtime/pkg/client"

 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
-	velerov1client "github.com/vmware-tanzu/velero/pkg/generated/clientset/versioned/typed/velero/v1"
-	velerov1informers "github.com/vmware-tanzu/velero/pkg/generated/informers/externalversions/velero/v1"
-	velerov1listers "github.com/vmware-tanzu/velero/pkg/generated/listers/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/persistence"
 	"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
-	"github.com/vmware-tanzu/velero/pkg/util/kube"
 )

-type downloadRequestController struct {
-	*genericController
+// DownloadRequestReconciler reconciles a DownloadRequest object
+type DownloadRequestReconciler struct {
+	Scheme *runtime.Scheme
+	Client kbclient.Client
+	Clock  clock.Clock
+	// use variables to refer to these functions so they can be
+	// replaced with fakes for testing.
+	NewPluginManager  func(logrus.FieldLogger) clientmgmt.Manager
+	BackupStoreGetter persistence.ObjectBackupStoreGetter

-	downloadRequestClient velerov1client.DownloadRequestsGetter
-	downloadRequestLister velerov1listers.DownloadRequestLister
-	restoreLister         velerov1listers.RestoreLister
-	clock                 clock.Clock
-	kbClient              client.Client
-	backupLister          velerov1listers.BackupLister
-	newPluginManager      func(logrus.FieldLogger) clientmgmt.Manager
-	backupStoreGetter     persistence.ObjectBackupStoreGetter
+	Log logrus.FieldLogger
 }

-// NewDownloadRequestController creates a new DownloadRequestController.
-func NewDownloadRequestController(
-	downloadRequestClient velerov1client.DownloadRequestsGetter,
-	downloadRequestInformer velerov1informers.DownloadRequestInformer,
-	restoreLister velerov1listers.RestoreLister,
-	kbClient client.Client,
-	backupLister velerov1listers.BackupLister,
-	newPluginManager func(logrus.FieldLogger) clientmgmt.Manager,
-	backupStoreGetter persistence.ObjectBackupStoreGetter,
-	logger logrus.FieldLogger,
-) Interface {
-	c := &downloadRequestController{
-		genericController:     newGenericController(DownloadRequest, logger),
-		downloadRequestClient: downloadRequestClient,
-		downloadRequestLister: downloadRequestInformer.Lister(),
-		restoreLister:         restoreLister,
-		kbClient:              kbClient,
-		backupLister:          backupLister,
+// +kubebuilder:rbac:groups=velero.io,resources=downloadrequests,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=velero.io,resources=downloadrequests/status,verbs=get;update;patch
+func (r *DownloadRequestReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	log := r.Log.WithFields(logrus.Fields{
+		"controller":      "download-request",
+		"downloadRequest": req.NamespacedName,
+	})

-		// use variables to refer to these functions so they can be
-		// replaced with fakes for testing.
-		newPluginManager:  newPluginManager,
-		backupStoreGetter: backupStoreGetter,
-
-		clock: &clock.RealClock{},
-	}
-
-	c.syncHandler = c.processDownloadRequest
-
-	downloadRequestInformer.Informer().AddEventHandler(
-		cache.ResourceEventHandlerFuncs{
-			AddFunc: func(obj interface{}) {
-				key, err := cache.MetaNamespaceKeyFunc(obj)
-				if err != nil {
-					downloadRequest := obj.(*velerov1api.DownloadRequest)
-					c.logger.WithError(errors.WithStack(err)).
-						WithField(DownloadRequest, downloadRequest.Name).
-						Error("Error creating queue key, item not added to queue")
-					return
-				}
-				c.queue.Add(key)
-			},
-		},
-	)
-
-	return c
-}
-
-// processDownloadRequest is the default per-item sync handler. It generates a pre-signed URL for
-// a new DownloadRequest or deletes the DownloadRequest if it has expired.
-func (c *downloadRequestController) processDownloadRequest(key string) error {
-	log := c.logger.WithField("key", key)
-
-	log.Debug("Running processDownloadRequest")
-	ns, name, err := cache.SplitMetaNamespaceKey(key)
-	if err != nil {
-		log.WithError(err).Error("error splitting queue key")
-		return nil
-	}
-
-	downloadRequest, err := c.downloadRequestLister.DownloadRequests(ns).Get(name)
-	if apierrors.IsNotFound(err) {
-		log.Debug("Unable to find DownloadRequest")
-		return nil
-	}
-	if err != nil {
-		return errors.Wrap(err, "error getting DownloadRequest")
-	}
-
-	switch downloadRequest.Status.Phase {
-	case "", velerov1api.DownloadRequestPhaseNew:
-		return c.generatePreSignedURL(downloadRequest, log)
-	case velerov1api.DownloadRequestPhaseProcessed:
-		return c.deleteIfExpired(downloadRequest)
-	}
-
-	return nil
-}
-
-const signedURLTTL = 10 * time.Minute
-
-// generatePreSignedURL generates a pre-signed URL for downloadRequest, changes the phase to
-// Processed, and persists the changes to storage.
-func (c *downloadRequestController) generatePreSignedURL(downloadRequest *velerov1api.DownloadRequest, log logrus.FieldLogger) error {
-	update := downloadRequest.DeepCopy()
-
-	var (
-		backupName string
-		err        error
-	)
-
-	switch downloadRequest.Spec.Target.Kind {
-	case velerov1api.DownloadTargetKindRestoreLog, velerov1api.DownloadTargetKindRestoreResults:
-		restore, err := c.restoreLister.Restores(downloadRequest.Namespace).Get(downloadRequest.Spec.Target.Name)
-		if err != nil {
-			return errors.Wrap(err, "error getting Restore")
+	// Fetch the DownloadRequest instance.
+	log.Debug("Getting DownloadRequest")
+	downloadRequest := &velerov1api.DownloadRequest{}
+	if err := r.Client.Get(ctx, req.NamespacedName, downloadRequest); err != nil {
+		if apierrors.IsNotFound(err) {
+			log.Debug("Unable to find DownloadRequest")
+			return ctrl.Result{}, nil
 		}

-		backupName = restore.Spec.BackupName
-	default:
-		backupName = downloadRequest.Spec.Target.Name
+		log.WithError(err).Error("Error getting DownloadRequest")
+		return ctrl.Result{}, errors.WithStack(err)
 	}

-	backup, err := c.backupLister.Backups(downloadRequest.Namespace).Get(backupName)
+	// Initialize the patch helper.
+	patchHelper, err := patch.NewHelper(downloadRequest, r.Client)
 	if err != nil {
-		return errors.WithStack(err)
+		log.WithError(err).Error("Error getting a patch helper to update this resource")
+		return ctrl.Result{}, errors.WithStack(err)
 	}

-	backupLocation := &velerov1api.BackupStorageLocation{}
-	if err := c.kbClient.Get(context.Background(), client.ObjectKey{
-		Namespace: backup.Namespace,
-		Name:      backup.Spec.StorageLocation,
-	}, backupLocation); err != nil {
-		return errors.WithStack(err)
+	defer func() {
+		// Always attempt to Patch the downloadRequest object and status after each reconciliation.
+		if err := patchHelper.Patch(ctx, downloadRequest); err != nil {
+			log.WithError(err).Error("Error updating download request")
+			return
+		}
+	}()
+
+	if downloadRequest.Status != (velerov1api.DownloadRequestStatus{}) && downloadRequest.Status.Expiration != nil {
+		if downloadRequest.Status.Expiration.Time.Before(r.Clock.Now()) {
+
+			// Delete any request that is expired, regardless of the phase: it is not
+			// worth proceeding and trying/retrying to find it.
+			log.Debug("DownloadRequest has expired - deleting")
+			if err := r.Client.Delete(ctx, downloadRequest); err != nil {
+				log.WithError(err).Error("Error deleting an expired download request")
+				return ctrl.Result{}, errors.WithStack(err)
+			}
+			return ctrl.Result{Requeue: false}, nil
+
+		} else if downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseProcessed {
+
+			// Requeue the request if is not yet expired and has already been processed before,
+			// since it might still be in use by the logs streaming and shouldn't
+			// be deleted until after its expiration.
+			log.Debug("DownloadRequest has not yet expired - requeueing")
+			return ctrl.Result{Requeue: true}, nil
+		}
 	}

-	pluginManager := c.newPluginManager(log)
-	defer pluginManager.CleanupClients()
+	// Process a brand new request.
+	backupName := downloadRequest.Spec.Target.Name
+	if downloadRequest.Status.Phase == "" || downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseNew {

-	backupStore, err := c.backupStoreGetter.Get(backupLocation, pluginManager, log)
-	if err != nil {
-		return errors.WithStack(err)
-	}
+		// Update the expiration.
+		downloadRequest.Status.Expiration = &metav1.Time{Time: r.Clock.Now().Add(persistence.DownloadURLTTL)}

-	if update.Status.DownloadURL, err = backupStore.GetDownloadURL(downloadRequest.Spec.Target); err != nil {
-		return err
-	}
-
-	update.Status.Phase = velerov1api.DownloadRequestPhaseProcessed
-	update.Status.Expiration = &metav1.Time{Time: c.clock.Now().Add(persistence.DownloadURLTTL)}
-
-	_, err = patchDownloadRequest(downloadRequest, update, c.downloadRequestClient)
-	return errors.WithStack(err)
-}
-
-// deleteIfExpired deletes downloadRequest if it has expired.
-func (c *downloadRequestController) deleteIfExpired(downloadRequest *velerov1api.DownloadRequest) error {
-	log := c.logger.WithField("key", kube.NamespaceAndName(downloadRequest))
-	log.Info("checking for expiration of DownloadRequest")
-	if downloadRequest.Status.Expiration.Time.After(c.clock.Now()) {
-		log.Debug("DownloadRequest has not expired")
-		return nil
-	}
-
-	log.Debug("DownloadRequest has expired - deleting")
-	return errors.WithStack(c.downloadRequestClient.DownloadRequests(downloadRequest.Namespace).Delete(context.TODO(), downloadRequest.Name, metav1.DeleteOptions{}))
-}
-
-// resync requeues all the DownloadRequests in the lister's cache. This is mostly to handle deleting
-// any expired requests that were not deleted as part of the normal client flow for whatever reason.
-func (c *downloadRequestController) resync() {
-	list, err := c.downloadRequestLister.List(labels.Everything())
-	if err != nil {
-		c.logger.WithError(errors.WithStack(err)).Error("error listing download requests")
-		return
-	}
-
-	for _, dr := range list {
-		key, err := cache.MetaNamespaceKeyFunc(dr)
-		if err != nil {
-			c.logger.WithError(errors.WithStack(err)).WithField(DownloadRequest, dr.Name).Error("error generating key for download request")
-			continue
+		if downloadRequest.Spec.Target.Kind == velerov1api.DownloadTargetKindRestoreLog ||
+			downloadRequest.Spec.Target.Kind == velerov1api.DownloadTargetKindRestoreResults {
+			restore := &velerov1api.Restore{}
+			if err := r.Client.Get(ctx, kbclient.ObjectKey{
+				Namespace: downloadRequest.Namespace,
+				Name:      downloadRequest.Spec.Target.Name,
+			}, restore); err != nil {
+				return ctrl.Result{}, errors.WithStack(err)
+			}
+			backupName = restore.Spec.BackupName
 		}

-		c.queue.Add(key)
+		backup := &velerov1api.Backup{}
+		if err := r.Client.Get(ctx, kbclient.ObjectKey{
+			Namespace: downloadRequest.Namespace,
+			Name:      backupName,
+		}, backup); err != nil {
+			return ctrl.Result{}, errors.WithStack(err)
+		}
+
+		location := &velerov1api.BackupStorageLocation{}
+		if err := r.Client.Get(ctx, kbclient.ObjectKey{
+			Namespace: backup.Namespace,
+			Name:      backup.Spec.StorageLocation,
+		}, location); err != nil {
+			return ctrl.Result{}, errors.WithStack(err)
+		}
+
+		pluginManager := r.NewPluginManager(log)
+		defer pluginManager.CleanupClients()
+
+		backupStore, err := r.BackupStoreGetter.Get(location, pluginManager, log)
+		if err != nil {
+			log.WithError(err).Error("Error getting a backup store")
+			return ctrl.Result{}, errors.WithStack(err)
+		}
+
+		if downloadRequest.Status.DownloadURL, err = backupStore.GetDownloadURL(downloadRequest.Spec.Target); err != nil {
+			return ctrl.Result{Requeue: true}, errors.WithStack(err)
+		}
+
+		downloadRequest.Status.Phase = velerov1api.DownloadRequestPhaseProcessed
+
+		// Update the expiration again to extend the time we wait (the TTL) to start after successfully processing the URL.
+		downloadRequest.Status.Expiration = &metav1.Time{Time: r.Clock.Now().Add(persistence.DownloadURLTTL)}
 	}
+
+	// Requeue is mostly to handle deleting any expired requests that were not
+	// deleted as part of the normal client flow for whatever reason.
+	return ctrl.Result{Requeue: true}, nil
 }

-func patchDownloadRequest(original, updated *velerov1api.DownloadRequest, client velerov1client.DownloadRequestsGetter) (*velerov1api.DownloadRequest, error) {
-	origBytes, err := json.Marshal(original)
-	if err != nil {
-		return nil, errors.Wrap(err, "error marshalling original download request")
-	}
-
-	updatedBytes, err := json.Marshal(updated)
-	if err != nil {
-		return nil, errors.Wrap(err, "error marshalling updated download request")
-	}
-
-	patchBytes, err := jsonpatch.CreateMergePatch(origBytes, updatedBytes)
-	if err != nil {
-		return nil, errors.Wrap(err, "error creating json merge patch for download request")
-	}
-
-	res, err := client.DownloadRequests(original.Namespace).Patch(context.TODO(), original.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{})
-	if err != nil {
-		return nil, errors.Wrap(err, "error patching download request")
-	}
-
-	return res, nil
+func (r *DownloadRequestReconciler) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&velerov1api.DownloadRequest{}).
+		Complete(r)
 }
--- a/pkg/controller/download_request_controller_test.go
+++ b/pkg/controller/download_request_controller_test.go
@@ -18,306 +18,249 @@ package controller

 import (
 	"context"
-	"testing"
 	"time"

+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
+	. "github.com/onsi/gomega"
+
 	"github.com/sirupsen/logrus"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/clock"
+	"k8s.io/client-go/kubernetes/scheme"

-	"sigs.k8s.io/controller-runtime/pkg/client"
+	ctrl "sigs.k8s.io/controller-runtime"
+
+	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"

 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"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"
 	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"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
-	kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
 )

-type downloadRequestTestHarness struct {
-	client          *fake.Clientset
-	informerFactory informers.SharedInformerFactory
-	pluginManager   *pluginmocks.Manager
-	backupStore     *persistencemocks.BackupStore
-
-	controller *downloadRequestController
-}
-
-func newDownloadRequestTestHarness(t *testing.T, fakeClient client.Client) *downloadRequestTestHarness {
-	var (
-		client          = fake.NewSimpleClientset()
-		informerFactory = informers.NewSharedInformerFactory(client, 0)
-		pluginManager   = new(pluginmocks.Manager)
-		backupStore     = new(persistencemocks.BackupStore)
-		controller      = NewDownloadRequestController(
-			client.VeleroV1(),
-			informerFactory.Velero().V1().DownloadRequests(),
-			informerFactory.Velero().V1().Restores().Lister(),
-			fakeClient,
-			informerFactory.Velero().V1().Backups().Lister(),
-			func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
-			NewFakeSingleObjectBackupStoreGetter(backupStore),
-			velerotest.NewLogger(),
-		).(*downloadRequestController)
-	)
-
-	clockTime, err := time.Parse(time.RFC1123, time.RFC1123)
-	require.NoError(t, err)
-	controller.clock = clock.NewFakeClock(clockTime)
-
-	pluginManager.On("CleanupClients").Return()
-
-	return &downloadRequestTestHarness{
-		client:          client,
-		informerFactory: informerFactory,
-		pluginManager:   pluginManager,
-		backupStore:     backupStore,
-		controller:      controller,
+var _ = Describe("Download Request Reconciler", func() {
+	type request struct {
+		downloadRequest      *velerov1api.DownloadRequest
+		backup               *velerov1api.Backup
+		restore              *velerov1api.Restore
+		backupLocation       *velerov1api.BackupStorageLocation
+		expired              bool
+		expectedReconcileErr string
+		expectGetsURL        bool
+		expectedRequeue      ctrl.Result
 	}
-}
-
-func newDownloadRequest(phase velerov1api.DownloadRequestPhase, targetKind velerov1api.DownloadTargetKind, targetName string) *velerov1api.DownloadRequest {
-	return &velerov1api.DownloadRequest{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "a-download-request",
-			Namespace: velerov1api.DefaultNamespace,
-		},
-		Spec: velerov1api.DownloadRequestSpec{
-			Target: velerov1api.DownloadTarget{
-				Kind: targetKind,
-				Name: targetName,
-			},
-		},
-		Status: velerov1api.DownloadRequestStatus{
-			Phase: phase,
-		},
-	}
-}
-
-func newBackupLocation(name, provider, bucket string) *velerov1api.BackupStorageLocation {
-	return &velerov1api.BackupStorageLocation{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      name,
-			Namespace: velerov1api.DefaultNamespace,
-		},
-		Spec: velerov1api.BackupStorageLocationSpec{
-			Provider: provider,
-			StorageType: velerov1api.StorageType{
-				ObjectStorage: &velerov1api.ObjectStorageLocation{
-					Bucket: bucket,
-				},
-			},
-		},
-	}
-}
-
-func TestProcessDownloadRequest(t *testing.T) {

 	defaultBackup := func() *velerov1api.Backup {
 		return builder.ForBackup(velerov1api.DefaultNamespace, "a-backup").StorageLocation("a-location").Result()
 	}

-	tests := []struct {
-		name            string
-		key             string
-		downloadRequest *velerov1api.DownloadRequest
-		backup          *velerov1api.Backup
-		restore         *velerov1api.Restore
-		backupLocation  *velerov1api.BackupStorageLocation
-		expired         bool
-		expectedErr     string
-		expectGetsURL   bool
-	}{
-		{
-			name: "empty key returns without error",
-			key:  "",
+	DescribeTable("a Download request",
+		func(test request) {
+			// now will be used to set the fake clock's time; capture
+			// it here so it can be referenced in the test case defs.
+			now, err := time.Parse(time.RFC1123, time.RFC1123)
+			Expect(err).To(BeNil())
+			now = now.Local()
+
+			rClock := clock.NewFakeClock(now)
+
+			const signedURLTTL = 10 * time.Minute
+
+			var (
+				pluginManager = &pluginmocks.Manager{}
+				backupStores  = make(map[string]*persistencemocks.BackupStore)
+			)
+			pluginManager.On("CleanupClients").Return(nil)
+
+			Expect(test.downloadRequest).ToNot(BeNil())
+
+			// Set .status.expiration properly for all requests test cases that are
+			// meant to be expired. Since "expired" is relative to the controller's
+			// clock time, it's easier to do this here than as part of the test case definitions.
+			if test.expired {
+				test.downloadRequest.Status.Expiration = &metav1.Time{Time: rClock.Now().Add(-1 * time.Minute)}
+			}
+
+			fakeClient := fake.NewFakeClientWithScheme(scheme.Scheme)
+			err = fakeClient.Create(context.TODO(), test.downloadRequest)
+			Expect(err).To(BeNil())
+
+			if test.backup != nil {
+				err := fakeClient.Create(context.TODO(), test.backup)
+				Expect(err).To(BeNil())
+			}
+
+			if test.backupLocation != nil {
+				err := fakeClient.Create(context.TODO(), test.backupLocation)
+				Expect(err).To(BeNil())
+				backupStores[test.backupLocation.Name] = &persistencemocks.BackupStore{}
+			}
+
+			if test.restore != nil {
+				err := fakeClient.Create(context.TODO(), test.restore)
+				Expect(err).To(BeNil())
+			}
+
+			// Setup reconciler
+			Expect(velerov1api.AddToScheme(scheme.Scheme)).To(Succeed())
+			r := DownloadRequestReconciler{
+				Client:            fakeClient,
+				Clock:             rClock,
+				NewPluginManager:  func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
+				BackupStoreGetter: NewFakeObjectBackupStoreGetter(backupStores),
+				Log:               velerotest.NewLogger(),
+			}
+
+			if test.backupLocation != nil && test.expectGetsURL {
+				backupStores[test.backupLocation.Name].On("GetDownloadURL", test.downloadRequest.Spec.Target).Return("a-url", nil)
+			}
+
+			actualResult, err := r.Reconcile(context.Background(), ctrl.Request{
+				NamespacedName: types.NamespacedName{
+					Namespace: velerov1api.DefaultNamespace,
+					Name:      test.downloadRequest.Name,
+				},
+			})
+
+			Expect(actualResult).To(BeEquivalentTo(test.expectedRequeue))
+			if test.expectedReconcileErr == "" {
+				Expect(err).To(BeNil())
+			} else {
+				Expect(err.Error()).To(Equal(test.expectedReconcileErr))
+			}
+
+			instance := &velerov1api.DownloadRequest{}
+			err = r.Client.Get(ctx, kbclient.ObjectKey{Name: test.downloadRequest.Name, Namespace: test.downloadRequest.Namespace}, instance)
+
+			if test.expired {
+				Expect(instance).ToNot(Equal(test.downloadRequest))
+				Expect(apierrors.IsNotFound(err)).To(BeTrue())
+			} else {
+				if test.downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseProcessed {
+					Expect(instance).To(Equal(test.downloadRequest))
+				} else {
+					Expect(instance).ToNot(Equal(test.downloadRequest))
+				}
+				Expect(err).To(BeNil())
+			}
+
+			if test.expectGetsURL {
+				Expect(string(instance.Status.Phase)).To(Equal(string(velerov1api.DownloadRequestPhaseProcessed)))
+				Expect(instance.Status.DownloadURL).To(Equal("a-url"))
+				Expect(velerotest.TimesAreEqual(instance.Status.Expiration.Time, r.Clock.Now().Add(signedURLTTL))).To(BeTrue())
+			}
 		},
-		{
-			name: "bad key format returns without error",
-			key:  "a/b/c",
-		},
-		{
-			name: "no download request for key returns without error",
-			key:  "nonexistent/key",
-		},
-		{
-			name:            "backup contents request for nonexistent backup returns an error",
-			downloadRequest: newDownloadRequest("", velerov1api.DownloadTargetKindBackupContents, "a-backup"),
-			backup:          builder.ForBackup(velerov1api.DefaultNamespace, "non-matching-backup").StorageLocation("a-location").Result(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
-			expectedErr:     "backup.velero.io \"a-backup\" not found",
-		},
-		{
-			name:            "restore log request for nonexistent restore returns an error",
-			downloadRequest: newDownloadRequest("", velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214"),
-			restore:         builder.ForRestore(velerov1api.DefaultNamespace, "non-matching-restore").Phase(velerov1api.RestorePhaseCompleted).Backup("a-backup").Result(),
+
+		Entry("backup contents request for nonexistent backup returns an error", request{
+			downloadRequest:      builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupContents, "a1-backup").Result(),
+			backup:               builder.ForBackup(velerov1api.DefaultNamespace, "non-matching-backup").StorageLocation("a-location").Result(),
+			backupLocation:       builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
+			expectedReconcileErr: "backups.velero.io \"a1-backup\" not found",
+			expectedRequeue:      ctrl.Result{Requeue: false},
+		}),
+		Entry("restore log request for nonexistent restore returns an error", request{
+			downloadRequest:      builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214").Result(),
+			restore:              builder.ForRestore(velerov1api.DefaultNamespace, "non-matching-restore").Phase(velerov1api.RestorePhaseCompleted).Backup("a-backup").Result(),
+			backup:               defaultBackup(),
+			backupLocation:       builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
+			expectedReconcileErr: "restores.velero.io \"a-backup-20170912150214\" not found",
+			expectedRequeue:      ctrl.Result{Requeue: false},
+		}),
+		Entry("backup contents request for backup with nonexistent location returns an error", request{
+			downloadRequest:      builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupContents, "a-backup").Result(),
+			backup:               defaultBackup(),
+			backupLocation:       builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "non-matching-location").Provider("a-provider").Bucket("a-bucket").Result(),
+			expectedReconcileErr: "backupstoragelocations.velero.io \"a-location\" not found",
+			expectedRequeue:      ctrl.Result{Requeue: false},
+		}),
+		Entry("backup contents request with phase '' gets a url", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupContents, "a-backup").Result(),
 			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
-			expectedErr:     "error getting Restore: restore.velero.io \"a-backup-20170912150214\" not found",
-		},
-		{
-			name:            "backup contents request for backup with nonexistent location returns an error",
-			downloadRequest: newDownloadRequest("", velerov1api.DownloadTargetKindBackupContents, "a-backup"),
-			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("non-matching-location", "a-provider", "a-bucket"),
-			expectedErr:     "backupstoragelocations.velero.io \"a-location\" not found",
-		},
-		{
-			name:            "backup contents request with phase '' gets a url",
-			downloadRequest: newDownloadRequest("", velerov1api.DownloadTargetKindBackupContents, "a-backup"),
-			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
+			backupLocation:  builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
 			expectGetsURL:   true,
-		},
-		{
-			name:            "backup contents request with phase 'New' gets a url",
-			downloadRequest: newDownloadRequest(velerov1api.DownloadRequestPhaseNew, velerov1api.DownloadTargetKindBackupContents, "a-backup"),
+			expectedRequeue: ctrl.Result{Requeue: true},
+		}),
+		Entry("backup contents request with phase 'New' gets a url", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindBackupContents, "a-backup").Result(),
 			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
+			backupLocation:  builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
 			expectGetsURL:   true,
-		},
-		{
-			name:            "backup log request with phase '' gets a url",
-			downloadRequest: newDownloadRequest("", velerov1api.DownloadTargetKindBackupLog, "a-backup"),
+			expectedRequeue: ctrl.Result{Requeue: true},
+		}),
+		Entry("backup log request with phase '' gets a url", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupLog, "a-backup").Result(),
 			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
+			backupLocation:  builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
 			expectGetsURL:   true,
-		},
-		{
-			name:            "backup log request with phase 'New' gets a url",
-			downloadRequest: newDownloadRequest(velerov1api.DownloadRequestPhaseNew, velerov1api.DownloadTargetKindBackupLog, "a-backup"),
+			expectedRequeue: ctrl.Result{Requeue: true},
+		}),
+		Entry("backup log request with phase 'New' gets a url", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindBackupLog, "a-backup").Result(),
 			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
+			backupLocation:  builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
 			expectGetsURL:   true,
-		},
-		{
-			name:            "restore log request with phase '' gets a url",
-			downloadRequest: newDownloadRequest("", velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214"),
+			expectedRequeue: ctrl.Result{Requeue: true},
+		}),
+		Entry("restore log request with phase '' gets a url", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214").Result(),
 			restore:         builder.ForRestore(velerov1api.DefaultNamespace, "a-backup-20170912150214").Phase(velerov1api.RestorePhaseCompleted).Backup("a-backup").Result(),
 			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
+			backupLocation:  builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
 			expectGetsURL:   true,
-		},
-		{
-			name:            "restore log request with phase 'New' gets a url",
-			downloadRequest: newDownloadRequest(velerov1api.DownloadRequestPhaseNew, velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214"),
+			expectedRequeue: ctrl.Result{Requeue: true},
+		}),
+		Entry("restore log request with phase 'New' gets a url", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindRestoreLog, "a-backup-20170912150214").Result(),
+			backup:          defaultBackup(),
+			restore:         builder.ForRestore(velerov1api.DefaultNamespace, "a-backup-20170912150214").Phase(velerov1api.RestorePhaseCompleted).Backup("a-backup").Result(),
+			backupLocation:  builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
+			expectGetsURL:   true,
+			expectedRequeue: ctrl.Result{Requeue: true},
+		}),
+		Entry("restore results request with phase '' gets a url", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindRestoreResults, "a-backup-20170912150214").Result(),
 			restore:         builder.ForRestore(velerov1api.DefaultNamespace, "a-backup-20170912150214").Phase(velerov1api.RestorePhaseCompleted).Backup("a-backup").Result(),
 			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
+			backupLocation:  builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
 			expectGetsURL:   true,
-		},
-		{
-			name:            "restore results request with phase '' gets a url",
-			downloadRequest: newDownloadRequest("", velerov1api.DownloadTargetKindRestoreResults, "a-backup-20170912150214"),
+			expectedRequeue: ctrl.Result{Requeue: true},
+		}),
+		Entry("restore results request with phase 'New' gets a url", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindRestoreResults, "a-backup-20170912150214").Result(),
 			restore:         builder.ForRestore(velerov1api.DefaultNamespace, "a-backup-20170912150214").Phase(velerov1api.RestorePhaseCompleted).Backup("a-backup").Result(),
 			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
+			backupLocation:  builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "a-location").Provider("a-provider").Bucket("a-bucket").Result(),
 			expectGetsURL:   true,
-		},
-		{
-			name:            "restore results request with phase 'New' gets a url",
-			downloadRequest: newDownloadRequest(velerov1api.DownloadRequestPhaseNew, velerov1api.DownloadTargetKindRestoreResults, "a-backup-20170912150214"),
-			restore:         builder.ForRestore(velerov1api.DefaultNamespace, "a-backup-20170912150214").Phase(velerov1api.RestorePhaseCompleted).Backup("a-backup").Result(),
+			expectedRequeue: ctrl.Result{Requeue: true},
+		}),
+		Entry("request with phase 'Processed' and not expired is not deleted", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseProcessed).Target(velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214").Result(),
 			backup:          defaultBackup(),
-			backupLocation:  newBackupLocation("a-location", "a-provider", "a-bucket"),
-			expectGetsURL:   true,
-		},
-		{
-			name:            "request with phase 'Processed' is not deleted if not expired",
-			downloadRequest: newDownloadRequest(velerov1api.DownloadRequestPhaseProcessed, velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214"),
-			backup:          defaultBackup(),
-		},
-		{
-			name:            "request with phase 'Processed' is deleted if expired",
-			downloadRequest: newDownloadRequest(velerov1api.DownloadRequestPhaseProcessed, velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214"),
+			expectedRequeue: ctrl.Result{Requeue: true},
+		}),
+		Entry("request with phase 'Processed' and expired is deleted", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseProcessed).Target(velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214").Result(),
 			backup:          defaultBackup(),
 			expired:         true,
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			var fakeClient client.Client
-			if tc.backupLocation != nil {
-				fakeClient = velerotest.NewFakeControllerRuntimeClient(t, tc.backupLocation)
-			} else {
-				fakeClient = velerotest.NewFakeControllerRuntimeClient(t)
-			}
-
-			harness := newDownloadRequestTestHarness(t, fakeClient)
-
-			// set up test case data
-
-			// Set .status.expiration properly for processed requests. Since "expired" is relative to the controller's
-			// clock time, it's easier to do this here than as part of the test case definitions.
-			if tc.downloadRequest != nil && tc.downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseProcessed {
-				if tc.expired {
-					tc.downloadRequest.Status.Expiration = &metav1.Time{Time: harness.controller.clock.Now().Add(-1 * time.Minute)}
-				} else {
-					tc.downloadRequest.Status.Expiration = &metav1.Time{Time: harness.controller.clock.Now().Add(time.Minute)}
-				}
-			}
-
-			if tc.downloadRequest != nil {
-				require.NoError(t, harness.informerFactory.Velero().V1().DownloadRequests().Informer().GetStore().Add(tc.downloadRequest))
-
-				_, err := harness.client.VeleroV1().DownloadRequests(tc.downloadRequest.Namespace).Create(context.TODO(), tc.downloadRequest, metav1.CreateOptions{})
-				require.NoError(t, err)
-			}
-
-			if tc.restore != nil {
-				require.NoError(t, harness.informerFactory.Velero().V1().Restores().Informer().GetStore().Add(tc.restore))
-			}
-
-			if tc.backup != nil {
-				require.NoError(t, harness.informerFactory.Velero().V1().Backups().Informer().GetStore().Add(tc.backup))
-			}
-
-			if tc.expectGetsURL {
-				harness.backupStore.On("GetDownloadURL", tc.downloadRequest.Spec.Target).Return("a-url", nil)
-			}
-
-			// exercise method under test
-			key := tc.key
-			if key == "" && tc.downloadRequest != nil {
-				key = kubeutil.NamespaceAndName(tc.downloadRequest)
-			}
-
-			err := harness.controller.processDownloadRequest(key)
-
-			// verify results
-			if tc.expectedErr != "" {
-				require.Equal(t, tc.expectedErr, err.Error())
-			} else {
-				assert.Nil(t, err)
-			}
-
-			if tc.expectGetsURL {
-				output, err := harness.client.VeleroV1().DownloadRequests(tc.downloadRequest.Namespace).Get(context.TODO(), tc.downloadRequest.Name, metav1.GetOptions{})
-				require.NoError(t, err)
-
-				assert.Equal(t, string(velerov1api.DownloadRequestPhaseProcessed), string(output.Status.Phase))
-				assert.Equal(t, "a-url", output.Status.DownloadURL)
-				assert.True(t, velerotest.TimesAreEqual(harness.controller.clock.Now().Add(signedURLTTL), output.Status.Expiration.Time), "expiration does not match")
-			}
-
-			if tc.downloadRequest != nil && tc.downloadRequest.Status.Phase == velerov1api.DownloadRequestPhaseProcessed {
-				res, err := harness.client.VeleroV1().DownloadRequests(tc.downloadRequest.Namespace).Get(context.TODO(), tc.downloadRequest.Name, metav1.GetOptions{})
-
-				if tc.expired {
-					assert.True(t, apierrors.IsNotFound(err))
-				} else {
-					assert.NoError(t, err)
-					assert.Equal(t, tc.downloadRequest, res)
-				}
-			}
-		})
-	}
-}
+			expectedRequeue: ctrl.Result{Requeue: false},
+		}),
+		Entry("request with phase '' and expired is deleted", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase("").Target(velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214").Result(),
+			backup:          defaultBackup(),
+			expired:         true,
+			expectedRequeue: ctrl.Result{Requeue: false},
+		}),
+		Entry("request with phase 'New' and expired is deleted", request{
+			downloadRequest: builder.ForDownloadRequest(velerov1api.DefaultNamespace, "a-download-request").Phase(velerov1api.DownloadRequestPhaseNew).Target(velerov1api.DownloadTargetKindBackupLog, "a-backup-20170912150214").Result(),
+			backup:          defaultBackup(),
+			expired:         true,
+			expectedRequeue: ctrl.Result{Requeue: false},
+		}),
+	)
+})
--- a/pkg/controller/server_status_request_controller.go
+++ b/pkg/controller/server_status_request_controller.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2018 the Velero contributors.
+Copyright the Velero contributors.

 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -71,12 +71,11 @@ func (r *ServerStatusRequestReconciler) Reconcile(ctx context.Context, req ctrl.
 	statusRequest := &velerov1api.ServerStatusRequest{}
 	if err := r.Client.Get(r.Ctx, req.NamespacedName, statusRequest); err != nil {
 		if apierrors.IsNotFound(err) {
-			log.WithError(err).Error("ServerStatusRequest not found")
+			log.Debug("Unable to find ServerStatusRequest")
 			return ctrl.Result{}, nil
 		}

 		log.WithError(err).Error("Error getting ServerStatusRequest")
-		// Error reading the object - requeue the request.
 		return ctrl.Result{}, err
 	}