Merge branch 'main' into copilot/follow-up-pr-for-daemonset-checks

changelogs/unreleased/10353-PranjalManhgaye

@@ -0,0 +1 @@
+Reject velero backup delete when backup storage location is read-only

changelogs/unreleased/10362-opbot-xd

@@ -0,0 +1 @@
+Unify duplicate test harness structs across restore and delete tests into a shared pkg/test.Harness

internal/delete/delete_item_action_handler_test.go

@@ -26,9 +26,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/api/meta"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
@@ -154,9 +151,9 @@ func TestInvokeDeleteItemActionsRunForCorrectItems(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			// test harness contains the fake API server/discovery client
-			h := newHarness(t)
+			h, dh := newHarness(t)
 			for _, r := range tc.apiResources {
-				h.addResource(t, r)
+				h.AddResource(t, dh, r)
 			}
 
 			// Get the plugins out of the map in order to use them.
@@ -169,7 +166,7 @@ func TestInvokeDeleteItemActionsRunForCorrectItems(t *testing.T) {
 				Backup:          tc.backup,
 				BackupReader:    tc.tarball,
 				Filesystem:      fs,
-				DiscoveryHelper: h.discoveryHelper,
+				DiscoveryHelper: dh,
 				Actions:         actions,
 				Log:             log,
 			}
@@ -187,46 +184,15 @@ func TestInvokeDeleteItemActionsRunForCorrectItems(t *testing.T) {
 	}
 }
 
-// TODO: unify this with the test harness in pkg/restore/restore_test.go
-type harness struct {
-	*test.APIServer
-	discoveryHelper discovery.Helper
-}
-
-func newHarness(t *testing.T) *harness {
+func newHarness(t *testing.T) (*test.Harness, discovery.Helper) {
 	t.Helper()
 
 	apiServer := test.NewAPIServer(t)
 	log := logrus.StandardLogger()
-
-	discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, log)
+	dh, err := discovery.NewHelper(apiServer.DiscoveryClient, log)
 	require.NoError(t, err)
 
-	return &harness{
-		APIServer:       apiServer,
-		discoveryHelper: discoveryHelper,
-	}
-}
-
-// addResource adds an APIResource and it's items to a faked API server for testing.
-func (h *harness) addResource(t *testing.T, resource *test.APIResource) {
-	t.Helper()
-
-	h.DiscoveryClient.WithAPIResource(resource)
-	require.NoError(t, h.discoveryHelper.Refresh())
-
-	for _, item := range resource.Items {
-		obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(item)
-		require.NoError(t, err)
-
-		unstructuredObj := &unstructured.Unstructured{Object: obj}
-		if resource.Namespaced {
-			_, err = h.DynamicClient.Resource(resource.GVR()).Namespace(item.GetNamespace()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{})
-		} else {
-			_, err = h.DynamicClient.Resource(resource.GVR()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{})
-		}
-		require.NoError(t, err)
-	}
+	return test.NewHarness(t, apiServer), dh
 }
 
 // recordResourcesAction is a delete item action that can be configured to run
@@ -306,14 +272,14 @@ func TestInvokeDeleteActionsReturnsPluginErrors(t *testing.T) {
 
 	action := &failingAction{err: errors.New("could not delete artifact")}
 
-	h := newHarness(t)
-	h.addResource(t, test.Pods())
+	h, dh := newHarness(t)
+	h.AddResource(t, dh, test.Pods())
 
 	c := &Context{
 		Backup:          builder.ForBackup("velero", "velero").Result(),
 		BackupReader:    tarball,
 		Filesystem:      fs,
-		DiscoveryHelper: h.discoveryHelper,
+		DiscoveryHelper: dh,
 		Actions:         []velero.DeleteItemAction{action},
 		Log:             log,
 	}

pkg/cmd/cli/backup/delete.go

@@ -121,7 +121,29 @@ func Run(o *cli.DeleteOptions) error {
 	}
 
 	// create a backup deletion request for each
+	bslCache := make(map[string]*velerov1api.BackupStorageLocation)
 	for _, b := range backups {
+		storageLocationName := b.Spec.StorageLocation
+		if storageLocationName == "" {
+			errs = append(errs, errors.Errorf("cannot delete backup %q because it does not have a backup storage location set", b.Name))
+			continue
+		}
+
+		location, ok := bslCache[storageLocationName]
+		if !ok {
+			location = &velerov1api.BackupStorageLocation{}
+			if err := o.Client.Get(context.TODO(), controllerclient.ObjectKey{Namespace: o.Namespace, Name: storageLocationName}, location); err != nil {
+				errs = append(errs, errors.Wrapf(err, "error getting backup storage location %q for backup %q", storageLocationName, b.Name))
+				continue
+			}
+			bslCache[storageLocationName] = location
+		}
+
+		if location.Spec.AccessMode == velerov1api.BackupStorageLocationAccessModeReadOnly {
+			errs = append(errs, errors.Errorf("cannot delete backup %q because backup storage location %q is currently in read-only mode", b.Name, location.Name))
+			continue
+		}
+
 		deleteRequest := builder.ForDeleteBackupRequest(o.Namespace, "").BackupName(b.Name).
 			ObjectMeta(builder.WithLabels(velerov1api.BackupNameLabel, label.GetValidName(b.Name),
 				velerov1api.BackupUIDLabel, string(b.UID)), builder.WithGenerateName(b.Name+"-")).Result()

pkg/cmd/cli/backup/delete_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/stretchr/testify/require"
 	controllerclient "sigs.k8s.io/controller-runtime/pkg/client"
 
+	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/builder"
 	factorymocks "github.com/vmware-tanzu/velero/pkg/client/mocks"
 	"github.com/vmware-tanzu/velero/pkg/cmd/cli"
@@ -82,3 +83,33 @@ func TestDeleteCommand(t *testing.T) {
 		require.Contains(t, stdout, fmt.Sprintf("backups.velero.io \"%s\" not found.", backup2))
 	}
 }
+
+func TestDeleteCommandReadOnlyBSL(t *testing.T) {
+	const (
+		backupName = "backup-readonly"
+		bslName    = "readonly-bsl"
+	)
+
+	client := velerotest.NewFakeControllerRuntimeClient(t)
+	require.NoError(t, client.Create(t.Context(), builder.ForBackupStorageLocation(cmdtest.VeleroNameSpace, bslName).
+		AccessMode(velerov1api.BackupStorageLocationAccessModeReadOnly).
+		Phase(velerov1api.BackupStorageLocationPhaseAvailable).
+		Result(), &controllerclient.CreateOptions{}))
+	require.NoError(t, client.Create(t.Context(), builder.ForBackup(cmdtest.VeleroNameSpace, backupName).
+		StorageLocation(bslName).
+		Result(), &controllerclient.CreateOptions{}))
+
+	o := cli.NewDeleteOptions("backup")
+	o.Client = client
+	o.Namespace = cmdtest.VeleroNameSpace
+	o.Confirm = true
+	o.Names = []string{backupName}
+
+	err := Run(o)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), fmt.Sprintf("cannot delete backup %q because backup storage location %q is currently in read-only mode", backupName, bslName))
+
+	deleteRequestList := new(velerov1api.DeleteBackupRequestList)
+	require.NoError(t, client.List(t.Context(), deleteRequestList, &controllerclient.ListOptions{Namespace: cmdtest.VeleroNameSpace}))
+	require.Empty(t, deleteRequestList.Items)
+}

pkg/restore/restore_test.go

@@ -235,7 +235,7 @@ func TestRestorePVWithVolumeInfo(t *testing.T) {
 			for _, r := range tc.apiResources {
 				h.DiscoveryClient.WithAPIResource(r)
 			}
-			require.NoError(t, h.restorer.discoveryHelper.Refresh())
+			require.NoError(t, h.discoveryHelper.Refresh())
 
 			data := &Request{
 				Log:                 h.log,
@@ -788,7 +788,7 @@ func TestRestoreResourceFiltering(t *testing.T) {
 			for _, r := range tc.apiResources {
 				h.DiscoveryClient.WithAPIResource(r)
 			}
-			require.NoError(t, h.restorer.discoveryHelper.Refresh())
+			require.NoError(t, h.discoveryHelper.Refresh())
 
 			// We need to fetch the policies using the actual function
 			resPolicies, err := resourcepolicies.GetResourcePoliciesFromRestore(t.Context(), tc.restore, h.restorer.kbClient, h.log)
@@ -868,7 +868,7 @@ func TestRestoreMustHaveResourceNamespaceEnforcement(t *testing.T) {
 			for _, r := range tc.apiResources {
 				h.DiscoveryClient.WithAPIResource(r)
 			}
-			require.NoError(t, h.restorer.discoveryHelper.Refresh())
+			require.NoError(t, h.discoveryHelper.Refresh())
 
 			resPolicies, err := resourcepolicies.GetResourcePoliciesFromRestore(t.Context(), tc.restore, h.restorer.kbClient, h.log)
 			require.NoError(t, err)
@@ -954,7 +954,7 @@ func TestRestoreNamespaceMapping(t *testing.T) {
 			for _, r := range tc.apiResources {
 				h.DiscoveryClient.WithAPIResource(r)
 			}
-			require.NoError(t, h.restorer.discoveryHelper.Refresh())
+			require.NoError(t, h.discoveryHelper.Refresh())
 
 			data := &Request{
 				Log:              h.log,
@@ -1038,7 +1038,7 @@ func TestRestoreResourcePriorities(t *testing.T) {
 		for _, r := range tc.apiResources {
 			h.DiscoveryClient.WithAPIResource(r)
 		}
-		require.NoError(t, h.restorer.discoveryHelper.Refresh())
+		require.NoError(t, h.discoveryHelper.Refresh())
 
 		data := &Request{
 			Log:              h.log,
@@ -1115,7 +1115,7 @@ func TestInvalidTarballContents(t *testing.T) {
 			for _, r := range tc.apiResources {
 				h.DiscoveryClient.WithAPIResource(r)
 			}
-			require.NoError(t, h.restorer.discoveryHelper.Refresh())
+			require.NoError(t, h.discoveryHelper.Refresh())
 
 			data := &Request{
 				Log:              h.log,
@@ -4548,10 +4548,10 @@ func assertNonEmptyResults(t *testing.T, typeMsg string, res ...Result) {
 }
 
 type harness struct {
-	*test.APIServer
-
-	restorer *kubernetesRestorer
-	log      logrus.FieldLogger
+	*test.Harness
+	discoveryHelper discovery.Helper
+	restorer        *kubernetesRestorer
+	log             logrus.FieldLogger
 }
 
 func newHarness(t *testing.T) *harness {
@@ -4561,13 +4561,14 @@ func newHarness(t *testing.T) *harness {
 	log := logrus.StandardLogger()
 	kbClient := test.NewFakeControllerRuntimeClient(t)
 
-	discoveryHelper, err := discovery.NewHelper(apiServer.DiscoveryClient, log)
+	dh, err := discovery.NewHelper(apiServer.DiscoveryClient, log)
 	require.NoError(t, err)
 
 	return &harness{
-		APIServer: apiServer,
+		Harness:         test.NewHarness(t, apiServer),
+		discoveryHelper: dh,
 		restorer: &kubernetesRestorer{
-			discoveryHelper:            discoveryHelper,
+			discoveryHelper:            dh,
 			dynamicFactory:             client.NewDynamicFactory(apiServer.DynamicClient),
 			namespaceClient:            apiServer.KubeClient.CoreV1().Namespaces(),
 			resourceTerminatingTimeout: time.Minute,
@@ -4586,28 +4587,7 @@ func newHarness(t *testing.T) *harness {
 
 func (h *harness) AddItems(t *testing.T, resource *test.APIResource) {
 	t.Helper()
-
-	h.DiscoveryClient.WithAPIResource(resource)
-	require.NoError(t, h.restorer.discoveryHelper.Refresh())
-
-	for _, item := range resource.Items {
-		obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(item)
-		require.NoError(t, err)
-
-		unstructuredObj := &unstructured.Unstructured{Object: obj}
-
-		// These fields have non-nil zero values in the unstructured objects. We remove
-		// them to make comparison easier in our tests.
-		unstructured.RemoveNestedField(unstructuredObj.Object, "metadata", "creationTimestamp")
-		unstructured.RemoveNestedField(unstructuredObj.Object, "status")
-
-		if resource.Namespaced {
-			_, err = h.DynamicClient.Resource(resource.GVR()).Namespace(item.GetNamespace()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{})
-		} else {
-			_, err = h.DynamicClient.Resource(resource.GVR()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{})
-		}
-		require.NoError(t, err)
-	}
+	h.Harness.AddResource(t, h.discoveryHelper, resource)
 }
 
 func Test_resetVolumeBindingInfo(t *testing.T) {

pkg/test/harness.go

@@ -0,0 +1,73 @@
+/*
+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.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+type refreshable interface {
+	Refresh() error
+}
+
+// Harness wraps an APIServer and provides AddResource for seeding
+// the fake API server with resources and items.
+// It is shared across delete, restore, and backup test packages.
+type Harness struct {
+	*APIServer
+}
+
+// NewHarness creates a Harness from an existing APIServer.
+func NewHarness(t *testing.T, apiServer *APIServer) *Harness {
+	t.Helper()
+	require.NotNil(t, apiServer, "apiServer must not be nil")
+	return &Harness{APIServer: apiServer}
+}
+
+// AddResource registers an API resource with the discovery client,
+// refreshes the discovery helper, and creates all of the resource's
+// items in the fake dynamic client.
+func (h *Harness) AddResource(t *testing.T, dh refreshable, resource *APIResource) {
+	t.Helper()
+
+	h.DiscoveryClient.WithAPIResource(resource)
+	require.NoError(t, dh.Refresh())
+
+	for _, item := range resource.Items {
+		obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(item)
+		require.NoError(t, err)
+
+		unstructuredObj := &unstructured.Unstructured{Object: obj}
+
+		// These fields have non-nil zero values in the unstructured objects. We remove
+		// them to make comparison easier in our tests.
+		unstructured.RemoveNestedField(unstructuredObj.Object, "metadata", "creationTimestamp")
+		unstructured.RemoveNestedField(unstructuredObj.Object, "status")
+
+		if resource.Namespaced {
+			_, err = h.DynamicClient.Resource(resource.GVR()).Namespace(item.GetNamespace()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{})
+		} else {
+			_, err = h.DynamicClient.Resource(resource.GVR()).Create(t.Context(), unstructuredObj, metav1.CreateOptions{})
+		}
+		require.NoError(t, err)
+	}
+}