From f2d06bc5e92268df5a6a9e10f2c4f40a39fa0e4e Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Wed, 31 Jul 2019 09:27:12 -0600 Subject: [PATCH] strip leading/trailing slashes from BSL bucket & prefix vals (#1694) * strip leading/trailing slashes from BSL bucket & prefix vals Signed-off-by: Steve Kriss --- changelogs/unreleased/1694-skriss | 1 + .../backup_storage_location_builder.go | 15 ++- pkg/controller/backup_controller_test.go | 6 +- pkg/controller/restore_controller_test.go | 6 +- pkg/persistence/object_store.go | 27 +++-- pkg/persistence/object_store_test.go | 105 +++++++++++++++--- 6 files changed, 129 insertions(+), 31 deletions(-) create mode 100644 changelogs/unreleased/1694-skriss diff --git a/changelogs/unreleased/1694-skriss b/changelogs/unreleased/1694-skriss new file mode 100644 index 000000000..4fd54cbbd --- /dev/null +++ b/changelogs/unreleased/1694-skriss @@ -0,0 +1 @@ +error if backup storage location's Bucket field also contains a prefix, and gracefully handle leading/trailing slashes on Bucket and Prefix fields. diff --git a/pkg/builder/backup_storage_location_builder.go b/pkg/builder/backup_storage_location_builder.go index 3acccab03..30d4926b6 100644 --- a/pkg/builder/backup_storage_location_builder.go +++ b/pkg/builder/backup_storage_location_builder.go @@ -63,12 +63,21 @@ func (b *BackupStorageLocationBuilder) Provider(name string) *BackupStorageLocat return b } -// ObjectStorage sets the BackupStorageLocation's object storage. -func (b *BackupStorageLocationBuilder) ObjectStorage(bucketName string) *BackupStorageLocationBuilder { +// Bucket sets the BackupStorageLocation's object storage bucket. +func (b *BackupStorageLocationBuilder) Bucket(val string) *BackupStorageLocationBuilder { if b.object.Spec.StorageType.ObjectStorage == nil { b.object.Spec.StorageType.ObjectStorage = new(velerov1api.ObjectStorageLocation) } - b.object.Spec.ObjectStorage.Bucket = bucketName + b.object.Spec.ObjectStorage.Bucket = val + return b +} + +// Prefix sets the BackupStorageLocation's object storage prefix. +func (b *BackupStorageLocationBuilder) Prefix(val string) *BackupStorageLocationBuilder { + if b.object.Spec.StorageType.ObjectStorage == nil { + b.object.Spec.StorageType.ObjectStorage = new(velerov1api.ObjectStorageLocation) + } + b.object.Spec.ObjectStorage.Prefix = val return b } diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 026084d58..7a53d8d2c 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -311,7 +311,7 @@ func TestDefaultBackupTTL(t *testing.T) { } func TestProcessBackupCompletions(t *testing.T) { - defaultBackupLocation := builder.ForBackupStorageLocation("velero", "loc-1").ObjectStorage("store-1").Result() + defaultBackupLocation := builder.ForBackupStorageLocation("velero", "loc-1").Bucket("store-1").Result() now, err := time.Parse(time.RFC1123Z, time.RFC1123Z) require.NoError(t, err) @@ -357,7 +357,7 @@ func TestProcessBackupCompletions(t *testing.T) { { name: "backup with a specific backup location keeps it", backup: defaultBackup().StorageLocation("alt-loc").Result(), - backupLocation: builder.ForBackupStorageLocation("velero", "alt-loc").ObjectStorage("store-1").Result(), + backupLocation: builder.ForBackupStorageLocation("velero", "alt-loc").Bucket("store-1").Result(), expectedResult: &velerov1api.Backup{ TypeMeta: metav1.TypeMeta{ Kind: "Backup", @@ -386,7 +386,7 @@ func TestProcessBackupCompletions(t *testing.T) { name: "backup for a location with ReadWrite access mode gets processed", backup: defaultBackup().StorageLocation("read-write").Result(), backupLocation: builder.ForBackupStorageLocation("velero", "read-write"). - ObjectStorage("store-1"). + Bucket("store-1"). AccessMode(velerov1api.BackupStorageLocationAccessModeReadWrite). Result(), expectedResult: &velerov1api.Backup{ diff --git a/pkg/controller/restore_controller_test.go b/pkg/controller/restore_controller_test.go index 85fa9d1d4..d35a01c70 100644 --- a/pkg/controller/restore_controller_test.go +++ b/pkg/controller/restore_controller_test.go @@ -66,7 +66,7 @@ func TestFetchBackupInfo(t *testing.T) { { name: "lister has backup", backupName: "backup-1", - informerLocations: []*api.BackupStorageLocation{builder.ForBackupStorageLocation("velero", "default").Provider("myCloud").ObjectStorage("bucket").Result()}, + informerLocations: []*api.BackupStorageLocation{builder.ForBackupStorageLocation("velero", "default").Provider("myCloud").Bucket("bucket").Result()}, informerBackups: []*api.Backup{defaultBackup().StorageLocation("default").Result()}, expectedRes: defaultBackup().StorageLocation("default").Result(), }, @@ -74,7 +74,7 @@ func TestFetchBackupInfo(t *testing.T) { name: "lister does not have a backup, but backupSvc does", backupName: "backup-1", backupStoreBackup: defaultBackup().StorageLocation("default").Result(), - informerLocations: []*api.BackupStorageLocation{builder.ForBackupStorageLocation("velero", "default").Provider("myCloud").ObjectStorage("bucket").Result()}, + informerLocations: []*api.BackupStorageLocation{builder.ForBackupStorageLocation("velero", "default").Provider("myCloud").Bucket("bucket").Result()}, informerBackups: []*api.Backup{defaultBackup().StorageLocation("default").Result()}, expectedRes: defaultBackup().StorageLocation("default").Result(), }, @@ -227,7 +227,7 @@ func TestProcessQueueItemSkips(t *testing.T) { } func TestProcessQueueItem(t *testing.T) { - defaultStorageLocation := builder.ForBackupStorageLocation("velero", "default").Provider("myCloud").ObjectStorage("bucket").Result() + defaultStorageLocation := builder.ForBackupStorageLocation("velero", "default").Provider("myCloud").Bucket("bucket").Result() tests := []struct { name string diff --git a/pkg/persistence/object_store.go b/pkg/persistence/object_store.go index c9dea6860..c1cd5b912 100644 --- a/pkg/persistence/object_store.go +++ b/pkg/persistence/object_store.go @@ -91,9 +91,15 @@ func NewObjectBackupStore(location *velerov1api.BackupStorageLocation, objectSto return nil, errors.New("object storage provider name must not be empty") } - objectStore, err := objectStoreGetter.GetObjectStore(location.Spec.Provider) - if err != nil { - return nil, err + // trim off any leading/trailing slashes + bucket := strings.Trim(location.Spec.ObjectStorage.Bucket, "/") + prefix := strings.Trim(location.Spec.ObjectStorage.Prefix, "/") + + // if there are any slashes in the middle of 'bucket', the user + // probably put / in the bucket field, which we + // don't support. + if strings.Contains(bucket, "/") { + return nil, errors.Errorf("backup storage location's bucket name %q must not contain a '/' (if using a prefix, put it in the 'Prefix' field instead)", location.Spec.ObjectStorage.Bucket) } // add the bucket name to the config map so that object stores can use @@ -103,7 +109,12 @@ func NewObjectBackupStore(location *velerov1api.BackupStorageLocation, objectSto if location.Spec.Config == nil { location.Spec.Config = make(map[string]string) } - location.Spec.Config["bucket"] = location.Spec.ObjectStorage.Bucket + location.Spec.Config["bucket"] = bucket + } + + objectStore, err := objectStoreGetter.GetObjectStore(location.Spec.Provider) + if err != nil { + return nil, err } if err := objectStore.Init(location.Spec.Config); err != nil { @@ -111,14 +122,14 @@ func NewObjectBackupStore(location *velerov1api.BackupStorageLocation, objectSto } log := logger.WithFields(logrus.Fields(map[string]interface{}{ - "bucket": location.Spec.ObjectStorage.Bucket, - "prefix": location.Spec.ObjectStorage.Prefix, + "bucket": bucket, + "prefix": prefix, })) return &objectBackupStore{ objectStore: objectStore, - bucket: location.Spec.ObjectStorage.Bucket, - layout: NewObjectStoreLayout(location.Spec.ObjectStorage.Prefix), + bucket: bucket, + layout: NewObjectStoreLayout(prefix), logger: log, }, nil } diff --git a/pkg/persistence/object_store_test.go b/pkg/persistence/object_store_test.go index 5976b39f4..c597c6a53 100644 --- a/pkg/persistence/object_store_test.go +++ b/pkg/persistence/object_store_test.go @@ -35,8 +35,10 @@ import ( "k8s.io/apimachinery/pkg/runtime" velerov1api "github.com/heptio/velero/pkg/apis/velero/v1" + "github.com/heptio/velero/pkg/builder" "github.com/heptio/velero/pkg/cloudprovider" cloudprovidermocks "github.com/heptio/velero/pkg/cloudprovider/mocks" + "github.com/heptio/velero/pkg/plugin/velero" "github.com/heptio/velero/pkg/util/encode" velerotest "github.com/heptio/velero/pkg/util/test" "github.com/heptio/velero/pkg/volume" @@ -170,8 +172,8 @@ func TestListBackups(t *testing.T) { { name: "normal case", storageData: map[string][]byte{ - "backups/backup-1/velero-backup.json": encodeToBytes(&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-1"}}), - "backups/backup-2/velero-backup.json": encodeToBytes(&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-2"}}), + "backups/backup-1/velero-backup.json": encodeToBytes(builder.ForBackup("", "backup-1").Result()), + "backups/backup-2/velero-backup.json": encodeToBytes(builder.ForBackup("", "backup-2").Result()), }, expectedRes: []string{"backup-1", "backup-2"}, }, @@ -179,8 +181,8 @@ func TestListBackups(t *testing.T) { name: "normal case with backup store prefix", prefix: "velero-backups/", storageData: map[string][]byte{ - "velero-backups/backups/backup-1/velero-backup.json": encodeToBytes(&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-1"}}), - "velero-backups/backups/backup-2/velero-backup.json": encodeToBytes(&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "backup-2"}}), + "velero-backups/backups/backup-1/velero-backup.json": encodeToBytes(builder.ForBackup("", "backup-1").Result()), + "velero-backups/backups/backup-2/velero-backup.json": encodeToBytes(builder.ForBackup("", "backup-2").Result()), }, expectedRes: []string{"backup-1", "backup-2"}, }, @@ -335,16 +337,7 @@ func TestGetBackupMetadata(t *testing.T) { name: "metadata file returns correctly", backupName: "foo", key: "backups/foo/velero-backup.json", - obj: &velerov1api.Backup{ - TypeMeta: metav1.TypeMeta{ - Kind: "Backup", - APIVersion: velerov1api.SchemeGroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Namespace: velerov1api.DefaultNamespace, - Name: "foo", - }, - }, + obj: builder.ForBackup(velerov1api.DefaultNamespace, "foo").Result(), }, { name: "no metadata file returns an error", @@ -558,6 +551,90 @@ func TestGetDownloadURL(t *testing.T) { } } +type objectStoreGetter map[string]velero.ObjectStore + +func (osg objectStoreGetter) GetObjectStore(provider string) (velero.ObjectStore, error) { + res, ok := osg[provider] + if !ok { + return nil, errors.New("object store not found") + } + + return res, nil +} + +// TestNewObjectBackupStore runs the NewObjectBackupStore constructor and ensures +// that an ObjectBackupStore is constructed correctly or an appropriate error is +// returned. +func TestNewObjectBackupStore(t *testing.T) { + tests := []struct { + name string + location *velerov1api.BackupStorageLocation + objectStoreGetter objectStoreGetter + wantBucket string + wantPrefix string + wantErr string + }{ + { + name: "location with no ObjectStorage field results in an error", + location: new(velerov1api.BackupStorageLocation), + wantErr: "backup storage location does not use object storage", + }, + { + name: "location with no Provider field results in an error", + location: builder.ForBackupStorageLocation("", "").Bucket("").Result(), + wantErr: "object storage provider name must not be empty", + }, + { + name: "location with a Bucket field with a '/' in the middle results in an error", + location: builder.ForBackupStorageLocation("", "").Provider("provider-1").Bucket("invalid/bucket").Result(), + wantErr: "backup storage location's bucket name \"invalid/bucket\" must not contain a '/' (if using a prefix, put it in the 'Prefix' field instead)", + }, + { + name: "when Bucket has a leading and trailing slash, they are both stripped", + location: builder.ForBackupStorageLocation("", "").Provider("provider-1").Bucket("/bucket/").Result(), + objectStoreGetter: objectStoreGetter{ + "provider-1": cloudprovider.NewInMemoryObjectStore("bucket"), + }, + wantBucket: "bucket", + }, + { + name: "when Prefix has a leading and trailing slash, the leading slash is stripped and the trailing slash is left", + location: builder.ForBackupStorageLocation("", "").Provider("provider-1").Bucket("bucket").Prefix("/prefix/").Result(), + objectStoreGetter: objectStoreGetter{ + "provider-1": cloudprovider.NewInMemoryObjectStore("bucket"), + }, + wantBucket: "bucket", + wantPrefix: "prefix/", + }, + { + name: "when Prefix has no leading or trailing slash, a trailing slash is added", + location: builder.ForBackupStorageLocation("", "").Provider("provider-1").Bucket("bucket").Prefix("prefix").Result(), + objectStoreGetter: objectStoreGetter{ + "provider-1": cloudprovider.NewInMemoryObjectStore("bucket"), + }, + wantBucket: "bucket", + wantPrefix: "prefix/", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + res, err := NewObjectBackupStore(tc.location, tc.objectStoreGetter, velerotest.NewLogger()) + if tc.wantErr != "" { + require.Equal(t, tc.wantErr, err.Error()) + } else { + require.Nil(t, err) + + store, ok := res.(*objectBackupStore) + require.True(t, ok) + + assert.Equal(t, tc.wantBucket, store.bucket) + assert.Equal(t, tc.wantPrefix, store.layout.rootPrefix) + } + }) + } +} + func encodeToBytes(obj runtime.Object) []byte { res, err := encode.Encode(obj, "json") if err != nil {