Merge branch 'main' into main

.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"

.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'

changelogs/unreleased/10167-samay43

@@ -0,0 +1 @@
+Validate backup name format before contacting the API server

changelogs/unreleased/10198-samay43

@@ -0,0 +1 @@
+add test coverage for CleanupVolumeSnapshot

changelogs/unreleased/10376-opbot-xd

@@ -0,0 +1 @@
+Fix context propagation bug in GetDefaultBackupStorageLocations and add missing test coverage for core components

changelogs/unreleased/10430-Lyndon-Li

@@ -0,0 +1 @@
+Enhance the doc for backup deletion

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))
+			}
+		})
+	}
+}

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)
 	}
 

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))
+				}
+			}
+		})
+	}
+}

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)

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)
+			}
+		})
+	}
+}

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)
+			}
+		})
+	}
+}

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 <backupName>`: 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 <backupName> -n <veleroNamespace>` will delete the backup custom resource only and will not delete any associated data from object/block storage
-* `velero backup delete <backupName>` 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 <backupName> -n <veleroNamespace>`: 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.