diff --git a/.github/workflows/e2e-test-kind.yaml b/.github/workflows/e2e-test-kind.yaml index fcf0d37c2..e3f220353 100644 --- a/.github/workflows/e2e-test-kind.yaml +++ b/.github/workflows/e2e-test-kind.yaml @@ -192,7 +192,7 @@ jobs: - name: Install MinIO run: | docker run -d --rm -p 9000:9000 -e "MINIO_ROOT_USER=minio" -e "MINIO_ROOT_PASSWORD=minio123" -e "MINIO_DEFAULT_BUCKETS=bucket,additional-bucket" bitnami/minio:local - - uses: helm/kind-action@7a97ed793754775518f9db3a8151ee7461dc9c31 # v1 + fix: add curl retry flags (https://github.com/helm/kind-action/pull/165) + - uses: helm/kind-action@c72b4750145dbfb1c71734c3782a4db35a1c65c0 # v1 + fix: add curl retry flags (https://github.com/helm/kind-action/pull/165) with: cluster_name: "kind" version: "v0.32.0" diff --git a/.github/workflows/nightly-trivy-scan.yml b/.github/workflows/nightly-trivy-scan.yml index d3f2e6062..09dcdbf4f 100644 --- a/.github/workflows/nightly-trivy-scan.yml +++ b/.github/workflows/nightly-trivy-scan.yml @@ -31,6 +31,6 @@ jobs: output: 'trivy-results.sarif' - name: Upload Trivy scan results to GitHub Security tab - uses: github/codeql-action/upload-sarif@v4.37.7 + uses: github/codeql-action/upload-sarif@v4.37.9 with: sarif_file: 'trivy-results.sarif' \ No newline at end of file diff --git a/changelogs/unreleased/10167-samay43 b/changelogs/unreleased/10167-samay43 new file mode 100644 index 000000000..0955fcc99 --- /dev/null +++ b/changelogs/unreleased/10167-samay43 @@ -0,0 +1 @@ +Validate backup name format before contacting the API server diff --git a/changelogs/unreleased/10198-samay43 b/changelogs/unreleased/10198-samay43 new file mode 100644 index 000000000..ab4c0c7f9 --- /dev/null +++ b/changelogs/unreleased/10198-samay43 @@ -0,0 +1 @@ +add test coverage for CleanupVolumeSnapshot diff --git a/changelogs/unreleased/10376-opbot-xd b/changelogs/unreleased/10376-opbot-xd new file mode 100644 index 000000000..a7f254e66 --- /dev/null +++ b/changelogs/unreleased/10376-opbot-xd @@ -0,0 +1 @@ +Fix context propagation bug in GetDefaultBackupStorageLocations and add missing test coverage for core components diff --git a/changelogs/unreleased/10430-Lyndon-Li b/changelogs/unreleased/10430-Lyndon-Li new file mode 100644 index 000000000..924b23a87 --- /dev/null +++ b/changelogs/unreleased/10430-Lyndon-Li @@ -0,0 +1 @@ +Enhance the doc for backup deletion \ No newline at end of file diff --git a/internal/credentials/secret_store_test.go b/internal/credentials/secret_store_test.go new file mode 100644 index 000000000..f9acf6c27 --- /dev/null +++ b/internal/credentials/secret_store_test.go @@ -0,0 +1,97 @@ +/* +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 credentials + +import ( + "testing" + + . "github.com/onsi/gomega" + corev1api "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestNamespacedSecretStore(t *testing.T) { + scheme := runtime.NewScheme() + g := NewWithT(t) + g.Expect(corev1api.AddToScheme(scheme)).To(Succeed()) + + secret := &corev1api.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "velero", + }, + Data: map[string][]byte{ + "creds-key": []byte("my-super-secret-value"), + }, + } + + client := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(secret).Build() + + store, err := NewNamespacedSecretStore(client, "velero") + g.Expect(err).ToNot(HaveOccurred()) + + tests := []struct { + name string + selector *corev1api.SecretKeySelector + expectedVal string + expectErr bool + }{ + { + name: "existing secret and key returns the correct value", + selector: &corev1api.SecretKeySelector{ + LocalObjectReference: corev1api.LocalObjectReference{Name: "test-secret"}, + Key: "creds-key", + }, + expectedVal: "my-super-secret-value", + expectErr: false, + }, + { + name: "missing secret returns an error", + selector: &corev1api.SecretKeySelector{ + LocalObjectReference: corev1api.LocalObjectReference{Name: "missing-secret"}, + Key: "creds-key", + }, + expectedVal: "", + expectErr: true, + }, + { + name: "missing key in existing secret returns an error", + selector: &corev1api.SecretKeySelector{ + LocalObjectReference: corev1api.LocalObjectReference{Name: "test-secret"}, + Key: "missing-key", + }, + expectedVal: "", + expectErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + val, err := store.Get(tc.selector) + if tc.expectErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(val).To(Equal(tc.expectedVal)) + } + }) + } +} diff --git a/internal/storage/storagelocation.go b/internal/storage/storagelocation.go index 59afe6b79..5024553dc 100644 --- a/internal/storage/storagelocation.go +++ b/internal/storage/storagelocation.go @@ -96,7 +96,7 @@ func ListBackupStorageLocations(ctx context.Context, kbClient client.Client, nam func GetDefaultBackupStorageLocations(ctx context.Context, kbClient client.Client, namespace string) (*velerov1api.BackupStorageLocationList, error) { locations := new(velerov1api.BackupStorageLocationList) defaultLocations := new(velerov1api.BackupStorageLocationList) - if err := kbClient.List(context.Background(), locations, &client.ListOptions{Namespace: namespace}); err != nil { + if err := kbClient.List(ctx, locations, &client.ListOptions{Namespace: namespace}); err != nil { return defaultLocations, errors.Wrapf(err, "failed to list backup storage locations in namespace %s", namespace) } diff --git a/internal/storage/storagelocation_test.go b/internal/storage/storagelocation_test.go index 44eabff48..474369487 100644 --- a/internal/storage/storagelocation_test.go +++ b/internal/storage/storagelocation_test.go @@ -173,3 +173,79 @@ func TestListBackupStorageLocations(t *testing.T) { }) } } + +func TestGetDefaultBackupStorageLocations(t *testing.T) { + tests := []struct { + name string + locations *velerov1api.BackupStorageLocationList + expectedDefaults []string + expectedErr bool + }{ + { + name: "no default locations", + locations: &velerov1api.BackupStorageLocationList{ + Items: []velerov1api.BackupStorageLocation{ + *builder.ForBackupStorageLocation("ns-1", "loc-1").Default(false).Result(), + *builder.ForBackupStorageLocation("ns-1", "loc-2").Default(false).Result(), + }, + }, + expectedDefaults: nil, + expectedErr: false, + }, + { + name: "one default location", + locations: &velerov1api.BackupStorageLocationList{ + Items: []velerov1api.BackupStorageLocation{ + *builder.ForBackupStorageLocation("ns-1", "loc-1").Default(false).Result(), + *builder.ForBackupStorageLocation("ns-1", "loc-2").Default(true).Result(), + }, + }, + expectedDefaults: []string{"loc-2"}, + expectedErr: false, + }, + { + name: "multiple default locations", + locations: &velerov1api.BackupStorageLocationList{ + Items: []velerov1api.BackupStorageLocation{ + *builder.ForBackupStorageLocation("ns-1", "loc-1").Default(true).Result(), + *builder.ForBackupStorageLocation("ns-1", "loc-2").Default(true).Result(), + *builder.ForBackupStorageLocation("ns-1", "loc-3").Default(false).Result(), + }, + }, + expectedDefaults: []string{"loc-1", "loc-2"}, + expectedErr: false, + }, + { + name: "empty locations list", + locations: &velerov1api.BackupStorageLocationList{}, + expectedDefaults: nil, + expectedErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + client := fake.NewClientBuilder().WithScheme(util.VeleroScheme).WithRuntimeObjects(tt.locations).Build() + + defaults, err := GetDefaultBackupStorageLocations(t.Context(), client, "ns-1") + if tt.expectedErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + + var defaultNames []string + for _, loc := range defaults.Items { + defaultNames = append(defaultNames, loc.Name) + } + + if tt.expectedDefaults == nil { + g.Expect(defaultNames).To(BeEmpty()) + } else { + g.Expect(defaultNames).To(ConsistOf(tt.expectedDefaults)) + } + } + }) + } +} diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index 8cdec3d99..2150061ab 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -25,6 +25,7 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" kubeerrs "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/tools/cache" kbclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -45,7 +46,21 @@ func NewCreateCommand(f client.Factory, use string) *cobra.Command { c := &cobra.Command{ Use: use + " NAME", Short: "Create a backup", - Args: cobra.MaximumNArgs(1), + Args: func(c *cobra.Command, args []string) error { + if err := cobra.MaximumNArgs(1)(c, args); err != nil { + return err + } + fromSchedule, _ := c.Flags().GetString("from-schedule") + if fromSchedule == "" && len(args) == 0 { + return fmt.Errorf("a backup name is required, unless you are creating based on a schedule") + } + if len(args) == 1 { + if errs := validation.IsDNS1123Subdomain(args[0]); len(errs) > 0 { + return fmt.Errorf("invalid backup name %q: %s", args[0], strings.Join(errs, "; ")) + } + } + return nil + }, Run: func(c *cobra.Command, args []string) { cmd.CheckError(o.Complete(args, f)) cmd.CheckError(o.Validate(c, args, f)) @@ -191,11 +206,27 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return err } - // Ensure that unless FromSchedule is set, args contains a backup name - if o.FromSchedule == "" && len(args) != 1 { + // Ensure that unless FromSchedule is set, a backup name is required + if o.FromSchedule == "" && o.Name == "" { return fmt.Errorf("a backup name is required, unless you are creating based on a schedule") } - + // Validate the backup name format whenever a name is provided + if o.Name != "" { + if errs := validation.IsDNS1123Subdomain(o.Name); len(errs) > 0 { + return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) + } + } + // When a backup name will be generated from the schedule (i.e. FromSchedule + // is set and no explicit name was given), ensure the schedule name leaves + // enough room for the generated timestamp suffix ("-" + 14-digit timestamp, + // 15 characters total) within the DNS1123 subdomain length limit. + if o.FromSchedule != "" && o.Name == "" { + const timestampSuffixLen = 15 // "-" + "20060102150405" + maxScheduleNameLen := validation.DNS1123SubdomainMaxLength - timestampSuffixLen + if len(o.FromSchedule) > maxScheduleNameLen { + return fmt.Errorf("schedule name %q is too long: must be %d characters or fewer to leave room for the generated timestamp suffix", o.FromSchedule, maxScheduleNameLen) + } + } errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces) if len(errs) > 0 { return kubeerrs.NewAggregate(errs) diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index 07d5bb493..90e9e8c8a 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -245,7 +245,7 @@ func TestCreateOptions_OrderedResources(t *testing.T) { } func TestCreateCommand(t *testing.T) { - name := "nameToBeCreated" + name := "name-to-be-created" args := []string{name} t.Run("create a backup create command with full options except fromSchedule and wait, then run by create option", func(t *testing.T) { @@ -457,3 +457,127 @@ func TestCreateCommand(t *testing.T) { assert.NoError(t, e) }) } +func TestCreateCommand_Args(t *testing.T) { + testCases := []struct { + name string + args []string + fromSchedule string + expectError bool + }{ + { + name: "should error when no name and no from-schedule", + args: []string{}, + expectError: true, + }, + { + name: "should pass when a valid name is provided", + args: []string{"my-backup"}, + expectError: false, + }, + { + name: "should error when the name is not a valid DNS1123 subdomain", + args: []string{"Invalid_Name!"}, + expectError: true, + }, + { + name: "should pass with no name when from-schedule is set", + args: []string{}, + fromSchedule: "daily-backup", + expectError: false, + }, + { + name: "should error when more than one arg is given", + args: []string{"name1", "name2"}, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + f := &factorymocks.Factory{} + cmd := NewCreateCommand(f, "") + if tc.fromSchedule != "" { + err := cmd.Flags().Set("from-schedule", tc.fromSchedule) + require.NoError(t, err) + } + + err := cmd.Args(cmd, tc.args) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestCreateOptions_Validate(t *testing.T) { + testCases := []struct { + name string + optName string + fromSchedule string + args []string + expectError bool + }{ + { + name: "should error when no name and no from-schedule", + optName: "", + args: []string{}, + expectError: true, + }, + { + name: "should pass with a valid name and no from-schedule", + optName: "my-backup", + args: []string{"my-backup"}, + expectError: false, + }, + { + name: "should error when name is invalid, regardless of from-schedule", + optName: "Invalid_Name!", + fromSchedule: "daily-backup", + args: []string{"Invalid_Name!"}, + expectError: true, + }, + { + name: "should pass when from-schedule is set and no name given", + optName: "", + fromSchedule: "daily-backup", + args: []string{}, + expectError: false, + }, + { + name: "should pass when schedule name leaves room for timestamp suffix", + optName: "", + fromSchedule: strings.Repeat("a", 238), // exactly at the 238-char limit + args: []string{}, + expectError: false, + }, + { + name: "should error when schedule name is too long to leave room for timestamp suffix", + optName: "", + fromSchedule: strings.Repeat("a", 239), // one over the 238-char limit + args: []string{}, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + f := &factorymocks.Factory{} + cmd := NewCreateCommand(f, "") + + o := NewCreateOptions() + o.Name = tc.optName + o.FromSchedule = tc.fromSchedule + + err := o.Validate(cmd, tc.args, f) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/pkg/util/csi/volume_snapshot_test.go b/pkg/util/csi/volume_snapshot_test.go index 895935b4b..a8ab5ed4e 100644 --- a/pkg/util/csi/volume_snapshot_test.go +++ b/pkg/util/csi/volume_snapshot_test.go @@ -2179,3 +2179,107 @@ func TestGetVSCForVS(t *testing.T) { }) } } + +func TestCleanupVolumeSnapshot(t *testing.T) { + retainVSCName := "retain-vsc" + + testCases := []struct { + name string + volSnap *snapshotv1api.VolumeSnapshot + objs []runtime.Object + expectDeleted bool + // name of the VolumeSnapshotContent expected to have been patched to + // DeletionPolicy=Delete; empty when no VSC should be touched. + expectedVSC string + }{ + { + name: "should be a no-op if the VolumeSnapshot no longer exists", + volSnap: &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-exist", + Namespace: "velero", + }, + }, + objs: []runtime.Object{}, + expectDeleted: false, + }, + { + name: "should delete a VolumeSnapshot with no bound VolumeSnapshotContent", + volSnap: &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vs-no-vsc", + Namespace: "velero", + }, + }, + objs: []runtime.Object{ + &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vs-no-vsc", + Namespace: "velero", + }, + }, + }, + expectDeleted: true, + }, + { + name: "should patch bound VSC DeletionPolicy to Delete and delete the VolumeSnapshot", + volSnap: &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vs-with-vsc", + Namespace: "velero", + }, + }, + objs: []runtime.Object{ + &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vs-with-vsc", + Namespace: "velero", + }, + Status: &snapshotv1api.VolumeSnapshotStatus{ + BoundVolumeSnapshotContentName: &retainVSCName, + }, + }, + &snapshotv1api.VolumeSnapshotContent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "retain-vsc", + }, + Spec: snapshotv1api.VolumeSnapshotContentSpec{ + DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain, + }, + }, + }, + expectDeleted: true, + expectedVSC: "retain-vsc", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, tc.objs...) + + CleanupVolumeSnapshot(t.Context(), tc.volSnap, fakeClient, velerotest.NewLogger()) + + actual := new(snapshotv1api.VolumeSnapshot) + err := fakeClient.Get( + t.Context(), + crclient.ObjectKey{Name: tc.volSnap.Name, Namespace: tc.volSnap.Namespace}, + actual, + ) + + if tc.expectDeleted { + assert.True(t, apierrors.IsNotFound(err), "expected VolumeSnapshot to be deleted") + } + + if tc.expectedVSC != "" { + actualVSC := new(snapshotv1api.VolumeSnapshotContent) + err := fakeClient.Get( + t.Context(), + crclient.ObjectKey{Name: tc.expectedVSC}, + actualVSC, + ) + require.NoError(t, err) + assert.Equal(t, snapshotv1api.VolumeSnapshotContentDelete, actualVSC.Spec.DeletionPolicy) + } + }) + } +} diff --git a/site/content/docs/main/backup-reference.md b/site/content/docs/main/backup-reference.md index f2045211f..46e0b0f5e 100644 --- a/site/content/docs/main/backup-reference.md +++ b/site/content/docs/main/backup-reference.md @@ -161,7 +161,12 @@ Pagination can be entirely disabled by setting `--client-page-size` to `0`. This ## Deleting Backups -Use the following commands to delete Velero backups and data: +Use the following commands to delete Velero backups: +`velero backup delete `: successful run of this command will: +- Immediately delete the resource backup data from the backup storage +- Immediately delete the volume snapshots associates to the backup if any (e.g., volumes are backed up with CSI snapshot backup or native snapshot method) +- Trigger the deletion of volume backup data if any data is persisted to the backup repository (e.g., volumes are backed up with CSI snapshot data movement or fs-backup method). You will not see the backup storage space is released immediately, the backup repository maintenance jobs will GC the data and finally release the storage space -* `kubectl delete backup -n ` will delete the backup custom resource only and will not delete any associated data from object/block storage -* `velero backup delete ` will delete the backup resource including all data in object/block storage +Velero backup deletion needs extra spaces in the backup storage, so make sure the backup storage is not full during the backup deletion, otherwise, the backup deletion or the following backup repository maintenance jobs may fail. + +`kubectl delete backup -n `: this command will delete the backup custom resource only and will not delete any associated data from the backup storage. So it is used for limited purposes only, e.g., all the backup data has been deleted in the backup storage, you just need to clear the orphaned CRs in the cluster.