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>
2026-08-20 06:06:06 +00:00 · 2019-05-01 10:25:11 -07:00
parent 28612afa27
commit 4e12b08953
18 changed files with 638 additions and 29 deletions
@@ -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) {
@@ -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 (
@@ -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 {
@@ -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) {
@@ -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)
@@ -28,11 +28,13 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/clock"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation"
 	core "k8s.io/client-go/testing"

 	velerov1api "github.com/heptio/velero/pkg/apis/velero/v1"
 	"github.com/heptio/velero/pkg/generated/clientset/versioned/fake"
 	informers "github.com/heptio/velero/pkg/generated/informers/externalversions"
+	"github.com/heptio/velero/pkg/label"
 	"github.com/heptio/velero/pkg/persistence"
 	persistencemocks "github.com/heptio/velero/pkg/persistence/mocks"
 	"github.com/heptio/velero/pkg/plugin/clientmgmt"
@@ -73,13 +75,47 @@ func defaultLocationsList(namespace string) []*velerov1api.BackupStorageLocation
 	}
 }

+func defaultLocationsListWithLongerLocationName(namespace string) []*velerov1api.BackupStorageLocation {
+	return []*velerov1api.BackupStorageLocation{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: namespace,
+				Name:      "the-really-long-location-name-that-is-much-more-than-63-characters-1",
+			},
+			Spec: velerov1api.BackupStorageLocationSpec{
+				Provider: "objStoreProvider",
+				StorageType: velerov1api.StorageType{
+					ObjectStorage: &velerov1api.ObjectStorageLocation{
+						Bucket: "bucket-1",
+					},
+				},
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: namespace,
+				Name:      "the-really-long-location-name-that-is-much-more-than-63-characters-2",
+			},
+			Spec: velerov1api.BackupStorageLocationSpec{
+				Provider: "objStoreProvider",
+				StorageType: velerov1api.StorageType{
+					ObjectStorage: &velerov1api.ObjectStorageLocation{
+						Bucket: "bucket-2",
+					},
+				},
+			},
+		},
+	}
+}
+
 func TestBackupSyncControllerRun(t *testing.T) {
 	tests := []struct {
-		name            string
-		namespace       string
-		locations       []*velerov1api.BackupStorageLocation
-		cloudBackups    map[string][]*velerov1api.Backup
-		existingBackups []*velerov1api.Backup
+		name                    string
+		namespace               string
+		locations               []*velerov1api.BackupStorageLocation
+		cloudBackups            map[string][]*velerov1api.Backup
+		existingBackups         []*velerov1api.Backup
+		longLocationNameEnabled bool
 	}{
 		{
 			name: "no cloud backups",
@@ -163,6 +199,21 @@ func TestBackupSyncControllerRun(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:                    "backup storage location names and labels get updated with location name greater than 63 chars",
+			namespace:               "ns-1",
+			locations:               defaultLocationsListWithLongerLocationName("ns-1"),
+			longLocationNameEnabled: true,
+			cloudBackups: map[string][]*velerov1api.Backup{
+				"bucket-1": {
+					velerotest.NewTestBackup().WithNamespace("ns-1").WithName("backup-1").WithStorageLocation("foo").WithLabel(velerov1api.StorageLocationLabel, "foo").Backup,
+					velerotest.NewTestBackup().WithNamespace("ns-1").WithName("backup-2").Backup,
+				},
+				"bucket-2": {
+					velerotest.NewTestBackup().WithNamespace("ns-1").WithName("backup-3").WithStorageLocation("bar").WithLabel(velerov1api.StorageLocationLabel, "bar").Backup,
+				},
+			},
+		},
 	}

 	for _, test := range tests {
@@ -259,7 +310,13 @@ func TestBackupSyncControllerRun(t *testing.T) {
 					} else {
 						// verify that the storage location field and label are set properly
 						assert.Equal(t, location.Name, obj.Spec.StorageLocation)
-						assert.Equal(t, location.Name, obj.Labels[velerov1api.StorageLocationLabel])
+
+						locationName := location.Name
+						if test.longLocationNameEnabled {
+							locationName = label.GetValidName(locationName)
+						}
+						assert.Equal(t, locationName, obj.Labels[velerov1api.StorageLocationLabel])
+						assert.Equal(t, true, len(obj.Labels[velerov1api.StorageLocationLabel]) <= validation.DNS1035LabelMaxLength)
 					}
 				}
 			}
@@ -402,6 +459,84 @@ func TestDeleteOrphanedBackups(t *testing.T) {
 	}
 }

+func TestStorageLabelsInDeleteOrphanedBackups(t *testing.T) {
+	longLabelName := "the-really-long-location-name-that-is-much-more-than-63-characters"
+	tests := []struct {
+		name            string
+		cloudBackups    sets.String
+		k8sBackups      []*velerotest.TestBackup
+		namespace       string
+		expectedDeletes sets.String
+	}{
+		{
+			name:         "some overlapping backups",
+			namespace:    "ns-1",
+			cloudBackups: sets.NewString("backup-1", "backup-2", "backup-3"),
+			k8sBackups: []*velerotest.TestBackup{
+				velerotest.NewTestBackup().WithNamespace("ns-1").WithName("backup-1").
+					WithLabel(velerov1api.StorageLocationLabel, "the-really-long-location-name-that-is-much-more-than-63-c69e779").WithPhase(velerov1api.BackupPhaseCompleted),
+				velerotest.NewTestBackup().WithNamespace("ns-1").WithName("backup-2").
+					WithLabel(velerov1api.StorageLocationLabel, "the-really-long-location-name-that-is-much-more-than-63-c69e779").WithPhase(velerov1api.BackupPhaseCompleted),
+				velerotest.NewTestBackup().WithNamespace("ns-1").WithName("backup-C").
+					WithLabel(velerov1api.StorageLocationLabel, "the-really-long-location-name-that-is-much-more-than-63-c69e779").WithPhase(velerov1api.BackupPhaseCompleted),
+			},
+			expectedDeletes: sets.NewString("backup-C"),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			var (
+				client          = fake.NewSimpleClientset()
+				sharedInformers = informers.NewSharedInformerFactory(client, 0)
+			)
+
+			c := NewBackupSyncController(
+				client.VeleroV1(),
+				client.VeleroV1(),
+				sharedInformers.Velero().V1().Backups(),
+				sharedInformers.Velero().V1().BackupStorageLocations(),
+				time.Duration(0),
+				test.namespace,
+				"",
+				nil, // new plugin manager func
+				velerotest.NewLogger(),
+			).(*backupSyncController)
+
+			expectedDeleteActions := make([]core.Action, 0)
+
+			for _, backup := range test.k8sBackups {
+				// add test backup to informer
+				require.NoError(t, sharedInformers.Velero().V1().Backups().Informer().GetStore().Add(backup.Backup), "Error adding backup to informer")
+
+				// add test backup to client
+				_, err := client.VeleroV1().Backups(test.namespace).Create(backup.Backup)
+				require.NoError(t, err, "Error adding backup to clientset")
+
+				// if we expect this backup to be deleted, set up the expected DeleteAction
+				if test.expectedDeletes.Has(backup.Name) {
+					actionDelete := core.NewDeleteAction(
+						velerov1api.SchemeGroupVersion.WithResource("backups"),
+						test.namespace,
+						backup.Name,
+					)
+					expectedDeleteActions = append(expectedDeleteActions, actionDelete)
+				}
+			}
+
+			c.deleteOrphanedBackups(longLabelName, test.cloudBackups, velerotest.NewLogger())
+
+			numBackups, err := numBackups(t, client, c.namespace)
+			assert.NoError(t, err)
+
+			expected := len(test.k8sBackups) - len(test.expectedDeletes)
+			assert.Equal(t, expected, numBackups)
+
+			velerotest.CompareActions(t, expectedDeleteActions, getDeleteActions(client.Actions()))
+		})
+	}
+}
+
 func TestShouldSync(t *testing.T) {
 	c := clock.NewFakeClock(time.Now())

@@ -31,6 +31,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"
 )

 const (
@@ -133,7 +134,7 @@ func (c *gcController) processQueueItem(key string) error {
 	log.Info("Backup has expired")

 	selector := labels.SelectorFromSet(labels.Set(map[string]string{
-		velerov1api.BackupNameLabel: backup.Name,
+		velerov1api.BackupNameLabel: label.GetValidName(backup.Name),
 		velerov1api.BackupUIDLabel:  string(backup.UID),
 	}))