Bug fix: Calculate label using backup name for CSI objects (#2510)

* Use a helper function when querying w/ backup label

Setting or querying for a backup label name should always pass the value
through the GetValidName function. This change passes query uses of the
backup label value through the GetValidName function by introducing 2
new helpers, one for making a Selector, one for making a ListOptions.

It also removes functions returning the same data, but under
unecessarily specific names.

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Document using the label.GetValidName function

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Update copyright year

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Clarify labels.GetValidName and annotations

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Move functions to pkg/label

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

* Fix function comments

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>

pkg/cmd/cli/backup/describe.go

@@ -28,7 +28,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/client"
 	"github.com/vmware-tanzu/velero/pkg/cmd"
 	"github.com/vmware-tanzu/velero/pkg/cmd/util/output"
-	"github.com/vmware-tanzu/velero/pkg/restic"
+	"github.com/vmware-tanzu/velero/pkg/label"
 )
 
 func NewDescribeCommand(f client.Factory, use string) *cobra.Command {
@@ -72,7 +72,7 @@ func NewDescribeCommand(f client.Factory, use string) *cobra.Command {
 					fmt.Fprintf(os.Stderr, "error getting DeleteBackupRequests for backup %s: %v\n", backup.Name, err)
 				}
 
-				opts := restic.NewPodVolumeBackupListOptions(backup.Name)
+				opts := label.NewListOptionsForBackup(backup.Name)
 				podVolumeBackupList, err := veleroClient.VeleroV1().PodVolumeBackups(f.Namespace()).List(opts)
 				if err != nil {
 					fmt.Fprintf(os.Stderr, "error getting PodVolumeBackups for backup %s: %v\n", backup.Name, err)

pkg/controller/backup_controller.go

@@ -559,7 +559,7 @@ func (c *backupController) runBackup(backup *pkgbackup.Request) error {
 	var volumeSnapshots []*snapshotv1beta1api.VolumeSnapshot
 	var volumeSnapshotContents []*snapshotv1beta1api.VolumeSnapshotContent
 	if features.IsEnabled(velerov1api.CSIFeatureFlag) {
-		selector := labels.SelectorFromSet(map[string]string{velerov1api.BackupNameLabel: backup.Name})
+		selector := label.NewSelectorForBackup(backup.Name)
 
 		// Listers are wrapped in a nil check out of caution, since they may not be populated based on the
 		// EnableCSI feature flag. This is more to guard against programmer error, as they shouldn't be nil

pkg/controller/backup_deletion_controller.go

@@ -426,9 +426,7 @@ func volumeSnapshotterForSnapshotLocation(
 
 func (c *backupDeletionController) deleteExistingDeletionRequests(req *velerov1api.DeleteBackupRequest, log logrus.FieldLogger) []error {
 	log.Info("Removing existing deletion requests for backup")
-	selector := labels.SelectorFromSet(labels.Set(map[string]string{
-		velerov1api.BackupNameLabel: label.GetValidName(req.Spec.BackupName),
-	}))
+	selector := label.NewSelectorForBackup(req.Spec.BackupName)
 	dbrs, err := c.deleteBackupRequestLister.DeleteBackupRequests(req.Namespace).List(selector)
 	if err != nil {
 		return []error{errors.Wrap(err, "error listing existing DeleteBackupRequests for backup")}
@@ -485,7 +483,7 @@ func deleteCSIVolumeSnapshots(backupName string, csiSnapshotLister snapshotv1bet
 	csiClient snapshotter.SnapshotV1beta1Interface, log *logrus.Entry) []error {
 	errs := []error{}
 
-	selector := labels.SelectorFromSet(map[string]string{velerov1api.BackupNameLabel: backupName})
+	selector := label.NewSelectorForBackup(backupName)
 	csiVolSnaps, err := csiSnapshotLister.List(selector)
 	if err != nil {
 		return []error{err}
@@ -515,7 +513,7 @@ func deleteCSIVolumeSnapshots(backupName string, csiSnapshotLister snapshotv1bet
 func deleteCSIVolumeSnapshotContents(backupName string, csiVSCLister snapshotv1beta1listers.VolumeSnapshotContentLister,
 	csiClient snapshotter.SnapshotV1beta1Interface, log *logrus.Entry) []error {
 	errs := []error{}
-	selector := labels.SelectorFromSet(map[string]string{velerov1api.BackupNameLabel: backupName})
+	selector := label.NewSelectorForBackup(backupName)
 	csiVolSnapConts, err := csiVSCLister.List(selector)
 	if err != nil {
 		return []error{err}

pkg/controller/restore_controller.go

@@ -40,10 +40,10 @@ import (
 	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/label"
 	"github.com/vmware-tanzu/velero/pkg/metrics"
 	"github.com/vmware-tanzu/velero/pkg/persistence"
 	"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
-	"github.com/vmware-tanzu/velero/pkg/restic"
 	pkgrestore "github.com/vmware-tanzu/velero/pkg/restore"
 	"github.com/vmware-tanzu/velero/pkg/util/collections"
 	kubeutil "github.com/vmware-tanzu/velero/pkg/util/kube"
@@ -438,7 +438,7 @@ func (c *restoreController) runValidatedRestore(restore *api.Restore, info backu
 	}
 	defer closeAndRemoveFile(backupFile, c.logger)
 
-	opts := restic.NewPodVolumeBackupListOptions(restore.Spec.BackupName)
+	opts := label.NewListOptionsForBackup(restore.Spec.BackupName)
 
 	podVolumeBackupList, err := c.podVolumeBackupClient.PodVolumeBackups(c.namespace).List(opts)
 	if err != nil {

pkg/label/label.go

@@ -20,7 +20,11 @@ import (
 	"crypto/sha256"
 	"fmt"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/validation"
+
+	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 )
 
 // GetValidName converts an input string to valid kubernetes label string in accordance to rfc1035 DNS Label spec
@@ -43,3 +47,17 @@ func GetValidName(label string) string {
 
 	return label[:charsFromLabel] + strSha[:6]
 }
+
+// NewSelectorForBackup returns a Selector based on the backup name.
+// This is useful for interacting with Listers that need a Selector.
+func NewSelectorForBackup(name string) labels.Selector {
+	return labels.SelectorFromSet(map[string]string{velerov1api.BackupNameLabel: GetValidName(name)})
+}
+
+// NewListOptionsForBackup returns a ListOptions based on the backup name.
+// This is useful for interacting with client-go clients that needs a ListOptions.
+func NewListOptionsForBackup(name string) metav1.ListOptions {
+	return metav1.ListOptions{
+		LabelSelector: fmt.Sprintf("%s=%s", velerov1api.BackupNameLabel, GetValidName(name)),
+	}
+}

pkg/restic/common.go

@@ -242,14 +242,6 @@ func GetCACert(backupLocationLister velerov1listers.BackupStorageLocationLister,
 	return nil, nil
 }
 
-// NewPodVolumeBackupListOptions creates a ListOptions with a label selector configured to
-// find PodVolumeBackups for the backup identified by name.
-func NewPodVolumeBackupListOptions(name string) metav1.ListOptions {
-	return metav1.ListOptions{
-		LabelSelector: fmt.Sprintf("%s=%s", velerov1api.BackupNameLabel, label.GetValidName(name)),
-	}
-}
-
 // NewPodVolumeRestoreListOptions creates a ListOptions with a label selector configured to
 // find PodVolumeRestores for the restore identified by name.
 func NewPodVolumeRestoreListOptions(name string) metav1.ListOptions {

pkg/restore/restic_restore_action.go

@@ -32,6 +32,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/builder"
 	"github.com/vmware-tanzu/velero/pkg/buildinfo"
 	velerov1client "github.com/vmware-tanzu/velero/pkg/generated/clientset/versioned/typed/velero/v1"
+	"github.com/vmware-tanzu/velero/pkg/label"
 	"github.com/vmware-tanzu/velero/pkg/plugin/framework"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
 	"github.com/vmware-tanzu/velero/pkg/restic"
@@ -75,7 +76,7 @@ func (a *ResticRestoreAction) Execute(input *velero.RestoreItemActionExecuteInpu
 
 	log := a.logger.WithField("pod", kube.NamespaceAndName(&pod))
 
-	opts := restic.NewPodVolumeBackupListOptions(input.Restore.Spec.BackupName)
+	opts := label.NewListOptionsForBackup(input.Restore.Spec.BackupName)
 	podVolumeBackupList, err := a.podVolumeBackupClient.List(opts)
 	if err != nil {
 		return nil, errors.WithStack(err)

site/docs/master/code-standards.md

@@ -64,6 +64,16 @@ mockery -name=Restorer
 
 Might need to run `make update` to update the imports.
 
+## Kubernetes Labels
+
+When generating label values, be sure to pass them through the `label.GetValidName()` helper function.
+
+This will help ensure that the values are the proper length and format to be stored and queried.
+
+In general, UIDs are safe to persist as label values.
+
+This function is not relevant to annotation values, which do not have restrictions.
+
 ## DCO Sign off
 
 All authors to the project retain copyright to their work. However, to ensure