diff --git a/changelogs/unreleased/10353-PranjalManhgaye b/changelogs/unreleased/10353-PranjalManhgaye new file mode 100644 index 000000000..46e5d5eed --- /dev/null +++ b/changelogs/unreleased/10353-PranjalManhgaye @@ -0,0 +1 @@ +Reject velero backup delete when backup storage location is read-only diff --git a/changelogs/unreleased/10362-opbot-xd b/changelogs/unreleased/10362-opbot-xd new file mode 100644 index 000000000..551b7985b --- /dev/null +++ b/changelogs/unreleased/10362-opbot-xd @@ -0,0 +1 @@ +Unify duplicate test harness structs across restore and delete tests into a shared pkg/test.Harness diff --git a/internal/delete/delete_item_action_handler_test.go b/internal/delete/delete_item_action_handler_test.go index b7d4e6e6a..1ad978bca 100644 --- a/internal/delete/delete_item_action_handler_test.go +++ b/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, } diff --git a/pkg/cmd/cli/backup/delete.go b/pkg/cmd/cli/backup/delete.go index ba5a4954b..f76e4fcf2 100644 --- a/pkg/cmd/cli/backup/delete.go +++ b/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() diff --git a/pkg/cmd/cli/backup/delete_test.go b/pkg/cmd/cli/backup/delete_test.go index 3278e8153..1e745e265 100644 --- a/pkg/cmd/cli/backup/delete_test.go +++ b/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) +} diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 5c75fff42..074053444 100644 --- a/pkg/restore/restore_test.go +++ b/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) { diff --git a/pkg/test/harness.go b/pkg/test/harness.go new file mode 100644 index 000000000..6fe1cf010 --- /dev/null +++ b/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) + } +}