From de8962ea18fbcbcd78ffb3cfb451b8cf00ca1f20 Mon Sep 17 00:00:00 2001 From: Nolan Brubaker Date: Thu, 7 May 2020 14:56:13 -0400 Subject: [PATCH] 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 * Document using the label.GetValidName function Signed-off-by: Nolan Brubaker * Update copyright year Signed-off-by: Nolan Brubaker * Clarify labels.GetValidName and annotations Signed-off-by: Nolan Brubaker * Move functions to pkg/label Signed-off-by: Nolan Brubaker * Fix function comments Signed-off-by: Nolan Brubaker --- pkg/cmd/cli/backup/describe.go | 4 ++-- pkg/controller/backup_controller.go | 2 +- pkg/controller/backup_deletion_controller.go | 8 +++----- pkg/controller/restore_controller.go | 4 ++-- pkg/label/label.go | 18 ++++++++++++++++++ pkg/restic/common.go | 8 -------- pkg/restore/restic_restore_action.go | 3 ++- site/docs/master/code-standards.md | 10 ++++++++++ 8 files changed, 38 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/cli/backup/describe.go b/pkg/cmd/cli/backup/describe.go index 536776ba5..ac5a0a7c0 100644 --- a/pkg/cmd/cli/backup/describe.go +++ b/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) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 8772f7963..d7af2cb29 100644 --- a/pkg/controller/backup_controller.go +++ b/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 diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index be0fa72b9..1af193e15 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/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} diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index b66e9a166..cfee38fc0 100644 --- a/pkg/controller/restore_controller.go +++ b/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 { diff --git a/pkg/label/label.go b/pkg/label/label.go index b66e7d822..4b7445d1a 100644 --- a/pkg/label/label.go +++ b/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)), + } +} diff --git a/pkg/restic/common.go b/pkg/restic/common.go index 75e510337..41f5c9f89 100644 --- a/pkg/restic/common.go +++ b/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 { diff --git a/pkg/restore/restic_restore_action.go b/pkg/restore/restic_restore_action.go index e0e993a90..ec02fdb77 100644 --- a/pkg/restore/restic_restore_action.go +++ b/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) diff --git a/site/docs/master/code-standards.md b/site/docs/master/code-standards.md index 594ba2d34..5c6c8c67c 100644 --- a/site/docs/master/code-standards.md +++ b/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