Validate restore name label length

Velero should handle cases when the label length exceeds 63 characters. - if the length of the backup/restore name is <= 63 characters, use it as the value of the label - if it's > 63 characters, take the SHA256 hash of the name. the value of the label will be the first 57 characters of the backup/restore name plus the first six characters of the SHA256 hash. Fixes heptio#1021 Signed-off-by: Anshul Chandra <anshulc@vmware.com>
canshul08 · Apr 30, 2019 · 58a1739 · 58a1739
1 parent 28612af
commit 58a1739
Show file tree

Hide file tree

Showing 18 changed files with 636 additions and 29 deletions.
diff --git a/changelogs/unreleased/1392-anshulc b/changelogs/unreleased/1392-anshulc
@@ -0,0 +1 @@
+Validate restore name label length
diff --git a/pkg/backup/delete_helpers.go b/pkg/backup/delete_helpers.go
@@ -21,7 +21,8 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-	v1 "github.com/heptio/velero/pkg/apis/velero/v1"
+	"github.com/heptio/velero/pkg/apis/velero/v1"
+	"github.com/heptio/velero/pkg/label"
 )
 
 // NewDeleteBackupRequest creates a DeleteBackupRequest for the backup identified by name and uid.
@@ -30,7 +31,7 @@ func NewDeleteBackupRequest(name string, uid string) *v1.DeleteBackupRequest {
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: name + "-",
 			Labels: map[string]string{
-				v1.BackupNameLabel: name,
+				v1.BackupNameLabel: label.GetValidName(name),
 				v1.BackupUIDLabel:  uid,
 			},
 		},
@@ -44,6 +45,6 @@ func NewDeleteBackupRequest(name string, uid string) *v1.DeleteBackupRequest {
 // find DeleteBackupRequests for the backup identified by name and uid.
 func NewDeleteBackupRequestListOptions(name, uid string) metav1.ListOptions {
 	return metav1.ListOptions{
-		LabelSelector: fmt.Sprintf("%s=%s,%s=%s", v1.BackupNameLabel, name, v1.BackupUIDLabel, uid),
+		LabelSelector: fmt.Sprintf("%s=%s,%s=%s", v1.BackupNameLabel, label.GetValidName(name), v1.BackupUIDLabel, uid),
 	}
 }
diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go
@@ -42,6 +42,7 @@ import (
 	velerov1client "github.com/heptio/velero/pkg/generated/clientset/versioned/typed/velero/v1"
 	informers "github.com/heptio/velero/pkg/generated/informers/externalversions/velero/v1"
 	listers "github.com/heptio/velero/pkg/generated/listers/velero/v1"
+	"github.com/heptio/velero/pkg/label"
 	"github.com/heptio/velero/pkg/metrics"
 	"github.com/heptio/velero/pkg/persistence"
 	"github.com/heptio/velero/pkg/plugin/clientmgmt"
@@ -296,7 +297,7 @@ func (c *backupController) prepareBackupRequest(backup *velerov1api.Backup) *pkg
 	if request.Labels == nil {
 		request.Labels = make(map[string]string)
 	}
-	request.Labels[velerov1api.StorageLocationLabel] = request.Spec.StorageLocation
+	request.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(request.Spec.StorageLocation)
 
 	// validate the included/excluded resources
 	for _, err := range collections.ValidateIncludesExcludes(request.Spec.IncludedResources, request.Spec.ExcludedResources) {

diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go
@@ -34,6 +34,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/clock"
 
 	v1 "github.com/heptio/velero/pkg/apis/velero/v1"
+	velerov1api "github.com/heptio/velero/pkg/apis/velero/v1"
 	pkgbackup "github.com/heptio/velero/pkg/backup"
 	"github.com/heptio/velero/pkg/generated/clientset/versioned/fake"
 	informers "github.com/heptio/velero/pkg/generated/informers/externalversions"
@@ -191,6 +192,53 @@ func TestProcessBackupValidationFailures(t *testing.T) {
 	}
 }
 
+func TestBackupLocationLabel(t *testing.T) {
+	tests := []struct {
+		name                   string
+		backup                 *v1.Backup
+		backupLocation         *v1.BackupStorageLocation
+		expectedBackupLocation string
+	}{
+		{
+			name:                   "valid backup location name should be used as a label",
+			backup:                 velerotest.NewTestBackup().WithName("backup-1").Backup,
+			backupLocation:         velerotest.NewTestBackupStorageLocation().WithName("loc-1").BackupStorageLocation,
+			expectedBackupLocation: "loc-1",
+		},
+		{
+			name:   "invalid storage location name should be handled while creating label",
+			backup: velerotest.NewTestBackup().WithName("backup-1").Backup,
+			backupLocation: velerotest.NewTestBackupStorageLocation().
+				WithName("defaultdefaultdefaultdefaultdefaultdefaultdefaultdefaultdefaultdefault").BackupStorageLocation,
+			expectedBackupLocation: "defaultdefaultdefaultdefaultdefaultdefaultdefaultdefaultd58343f",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			var (
+				clientset       = fake.NewSimpleClientset(test.backup)
+				sharedInformers = informers.NewSharedInformerFactory(clientset, 0)
+				logger          = logging.DefaultLogger(logrus.DebugLevel)
+			)
+
+			c := &backupController{
+				genericController:      newGenericController("backup-test", logger),
+				client:                 clientset.VeleroV1(),
+				lister:                 sharedInformers.Velero().V1().Backups().Lister(),
+				backupLocationLister:   sharedInformers.Velero().V1().BackupStorageLocations().Lister(),
+				snapshotLocationLister: sharedInformers.Velero().V1().VolumeSnapshotLocations().Lister(),
+				defaultBackupLocation:  test.backupLocation.Name,
+				clock:                  &clock.RealClock{},
+			}
+
+			res := c.prepareBackupRequest(test.backup)
+			assert.NotNil(t, res)
+			assert.Equal(t, test.expectedBackupLocation, res.Labels[velerov1api.StorageLocationLabel])
+		})
+	}
+}
+
 func TestDefaultBackupTTL(t *testing.T) {
 
 	var (

diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go
@@ -21,7 +21,7 @@ import (
 	"encoding/json"
 	"time"
 
-	jsonpatch "github.com/evanphx/json-patch"
+	"github.com/evanphx/json-patch"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -32,11 +32,12 @@ import (
 	kubeerrs "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/tools/cache"
 
-	v1 "github.com/heptio/velero/pkg/apis/velero/v1"
+	"github.com/heptio/velero/pkg/apis/velero/v1"
 	pkgbackup "github.com/heptio/velero/pkg/backup"
 	velerov1client "github.com/heptio/velero/pkg/generated/clientset/versioned/typed/velero/v1"
 	informers "github.com/heptio/velero/pkg/generated/informers/externalversions/velero/v1"
 	listers "github.com/heptio/velero/pkg/generated/listers/velero/v1"
+	"github.com/heptio/velero/pkg/label"
 	"github.com/heptio/velero/pkg/metrics"
 	"github.com/heptio/velero/pkg/persistence"
 	"github.com/heptio/velero/pkg/plugin/clientmgmt"
@@ -196,7 +197,7 @@ func (c *backupDeletionController) processRequest(req *v1.DeleteBackupRequest) e
 		r.Status.Phase = v1.DeleteBackupRequestPhaseInProgress
 
 		if req.Labels[v1.BackupNameLabel] == "" {
-			req.Labels[v1.BackupNameLabel] = req.Spec.BackupName
+			req.Labels[v1.BackupNameLabel] = label.GetValidName(req.Spec.BackupName)
 		}
 	})
 	if err != nil {
@@ -390,7 +391,7 @@ func (c *backupDeletionController) backupStoreForBackup(backup *v1.Backup, plugi
 func (c *backupDeletionController) deleteExistingDeletionRequests(req *v1.DeleteBackupRequest, log logrus.FieldLogger) []error {
 	log.Info("Removing existing deletion requests for backup")
 	selector := labels.SelectorFromSet(labels.Set(map[string]string{
-		v1.BackupNameLabel: req.Spec.BackupName,
+		v1.BackupNameLabel: label.GetValidName(req.Spec.BackupName),
 	}))
 	dbrs, err := c.deleteBackupRequestLister.DeleteBackupRequests(req.Namespace).List(selector)
 	if err != nil {

diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go
@@ -465,6 +465,151 @@ func TestBackupDeletionControllerProcessRequest(t *testing.T) {
 		// Make sure snapshot was deleted
 		assert.Equal(t, 0, td.volumeSnapshotter.SnapshotsTaken.Len())
 	})
+
+	t.Run("full delete, no errors, with backup name greater than 63 chars", func(t *testing.T) {
+		backup := velerotest.NewTestBackup().WithName("the-really-long-backup-name-that-is-much-more-than-63-characters").Backup
+		backup.UID = "uid"
+		backup.Spec.StorageLocation = "primary"
+
+		restore1 := velerotest.NewTestRestore("velero", "restore-1", v1.RestorePhaseCompleted).
+			WithBackup("the-really-long-backup-name-that-is-much-more-than-63-characters").Restore
+		restore2 := velerotest.NewTestRestore("velero", "restore-2", v1.RestorePhaseCompleted).
+			WithBackup("the-really-long-backup-name-that-is-much-more-than-63-characters").Restore
+		restore3 := velerotest.NewTestRestore("velero", "restore-3", v1.RestorePhaseCompleted).
+			WithBackup("some-other-backup").Restore
+
+		td := setupBackupDeletionControllerTest(backup, restore1, restore2, restore3)
+		td.req = pkgbackup.NewDeleteBackupRequest(backup.Name, string(backup.UID))
+		td.req.Namespace = "velero"
+		td.req.Name = "foo-abcde"
+		td.sharedInformers.Velero().V1().Restores().Informer().GetStore().Add(restore1)
+		td.sharedInformers.Velero().V1().Restores().Informer().GetStore().Add(restore2)
+		td.sharedInformers.Velero().V1().Restores().Informer().GetStore().Add(restore3)
+
+		location := &v1.BackupStorageLocation{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: backup.Namespace,
+				Name:      backup.Spec.StorageLocation,
+			},
+			Spec: v1.BackupStorageLocationSpec{
+				Provider: "objStoreProvider",
+				StorageType: v1.StorageType{
+					ObjectStorage: &v1.ObjectStorageLocation{
+						Bucket: "bucket",
+					},
+				},
+			},
+		}
+		require.NoError(t, td.sharedInformers.Velero().V1().BackupStorageLocations().Informer().GetStore().Add(location))
+
+		snapshotLocation := &v1.VolumeSnapshotLocation{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: backup.Namespace,
+				Name:      "vsl-1",
+			},
+			Spec: v1.VolumeSnapshotLocationSpec{
+				Provider: "provider-1",
+			},
+		}
+		require.NoError(t, td.sharedInformers.Velero().V1().VolumeSnapshotLocations().Informer().GetStore().Add(snapshotLocation))
+
+		// Clear out req labels to make sure the controller adds them
+		td.req.Labels = make(map[string]string)
+
+		td.client.PrependReactor("get", "backups", func(action core.Action) (bool, runtime.Object, error) {
+			return true, backup, nil
+		})
+		td.volumeSnapshotter.SnapshotsTaken.Insert("snap-1")
+
+		td.client.PrependReactor("patch", "deletebackuprequests", func(action core.Action) (bool, runtime.Object, error) {
+			return true, td.req, nil
+		})
+
+		td.client.PrependReactor("patch", "backups", func(action core.Action) (bool, runtime.Object, error) {
+			return true, backup, nil
+		})
+
+		snapshots := []*volume.Snapshot{
+			{
+				Spec: volume.SnapshotSpec{
+					Location: "vsl-1",
+				},
+				Status: volume.SnapshotStatus{
+					ProviderSnapshotID: "snap-1",
+				},
+			},
+		}
+
+		pluginManager := &pluginmocks.Manager{}
+		pluginManager.On("GetVolumeSnapshotter", "provider-1").Return(td.volumeSnapshotter, nil)
+		pluginManager.On("CleanupClients")
+		td.controller.newPluginManager = func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager }
+
+		td.backupStore.On("GetBackupVolumeSnapshots", td.req.Spec.BackupName).Return(snapshots, nil)
+		td.backupStore.On("DeleteBackup", td.req.Spec.BackupName).Return(nil)
+		td.backupStore.On("DeleteRestore", "restore-1").Return(nil)
+		td.backupStore.On("DeleteRestore", "restore-2").Return(nil)
+
+		err := td.controller.processRequest(td.req)
+		require.NoError(t, err)
+
+		expectedActions := []core.Action{
+			core.NewPatchAction(
+				v1.SchemeGroupVersion.WithResource("deletebackuprequests"),
+				td.req.Namespace,
+				td.req.Name,
+				[]byte(`{"metadata":{"labels":{"velero.io/backup-name":"the-really-long-backup-name-that-is-much-more-than-63-cha6ca4bc"}},"status":{"phase":"InProgress"}}`),
+			),
+			core.NewGetAction(
+				v1.SchemeGroupVersion.WithResource("backups"),
+				td.req.Namespace,
+				td.req.Spec.BackupName,
+			),
+			core.NewPatchAction(
+				v1.SchemeGroupVersion.WithResource("deletebackuprequests"),
+				td.req.Namespace,
+				td.req.Name,
+				[]byte(`{"metadata":{"labels":{"velero.io/backup-uid":"uid"}}}`),
+			),
+			core.NewPatchAction(
+				v1.SchemeGroupVersion.WithResource("backups"),
+				td.req.Namespace,
+				td.req.Spec.BackupName,
+				[]byte(`{"status":{"phase":"Deleting"}}`),
+			),
+			core.NewDeleteAction(
+				v1.SchemeGroupVersion.WithResource("restores"),
+				td.req.Namespace,
+				"restore-1",
+			),
+			core.NewDeleteAction(
+				v1.SchemeGroupVersion.WithResource("restores"),
+				td.req.Namespace,
+				"restore-2",
+			),
+			core.NewDeleteAction(
+				v1.SchemeGroupVersion.WithResource("backups"),
+				td.req.Namespace,
+				td.req.Spec.BackupName,
+			),
+			core.NewPatchAction(
+				v1.SchemeGroupVersion.WithResource("deletebackuprequests"),
+				td.req.Namespace,
+				td.req.Name,
+				[]byte(`{"status":{"phase":"Processed"}}`),
+			),
+			core.NewDeleteCollectionAction(
+				v1.SchemeGroupVersion.WithResource("deletebackuprequests"),
+				td.req.Namespace,
+				pkgbackup.NewDeleteBackupRequestListOptions(td.req.Spec.BackupName, "uid"),
+			),
+		}
+
+		velerotest.CompareActions(t, expectedActions, td.client.Actions())
+
+		// Make sure snapshot was deleted
+		assert.Equal(t, 0, td.volumeSnapshotter.SnapshotsTaken.Len())
+	})
 }
 
 func TestBackupDeletionControllerDeleteExpiredRequests(t *testing.T) {

diff --git a/pkg/controller/backup_sync_controller.go b/pkg/controller/backup_sync_controller.go
@@ -33,6 +33,7 @@ import (
 	velerov1client "github.com/heptio/velero/pkg/generated/clientset/versioned/typed/velero/v1"
 	informers "github.com/heptio/velero/pkg/generated/informers/externalversions/velero/v1"
 	listers "github.com/heptio/velero/pkg/generated/listers/velero/v1"
+	"github.com/heptio/velero/pkg/label"
 	"github.com/heptio/velero/pkg/persistence"
 	"github.com/heptio/velero/pkg/plugin/clientmgmt"
 )
@@ -218,7 +219,7 @@ func (c *backupSyncController) run() {
 			if backup.Labels == nil {
 				backup.Labels = make(map[string]string)
 			}
-			backup.Labels[velerov1api.StorageLocationLabel] = backup.Spec.StorageLocation
+			backup.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(backup.Spec.StorageLocation)
 
 			_, err = c.backupClient.Backups(backup.Namespace).Create(backup)
 			switch {
@@ -283,7 +284,7 @@ func patchStorageLocation(backup *velerov1api.Backup, client velerov1client.Back
 // and a phase of Completed, but no corresponding backup in object storage.
 func (c *backupSyncController) deleteOrphanedBackups(locationName string, cloudBackupNames sets.String, log logrus.FieldLogger) {
 	locationSelector := labels.Set(map[string]string{
-		velerov1api.StorageLocationLabel: locationName,
+		velerov1api.StorageLocationLabel: label.GetValidName(locationName),
 	}).AsSelector()
 
 	backups, err := c.backupLister.Backups(c.namespace).List(locationSelector)