diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 4764dbf79b..ef62c53315 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -636,6 +636,8 @@ func (s *server) runControllers(config *api.Config) error { backupTracker, s.sharedInformerFactory.Ark().V1().BackupStorageLocations(), s.config.defaultBackupLocation, + s.sharedInformerFactory.Ark().V1().VolumeSnapshotLocations(), + nil, s.metrics, ) wg.Add(1) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 4cc5ee23d3..2cf045a19e 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -55,18 +55,20 @@ const backupVersion = 1 type backupController struct { *genericController - backupper backup.Backupper - pvProviderExists bool - lister listers.BackupLister - client arkv1client.BackupsGetter - clock clock.Clock - backupLogLevel logrus.Level - newPluginManager func(logrus.FieldLogger) plugin.Manager - backupTracker BackupTracker - backupLocationLister listers.BackupStorageLocationLister - defaultBackupLocation string - metrics *metrics.ServerMetrics - newBackupStore func(*api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) + backupper backup.Backupper + pvProviderExists bool + lister listers.BackupLister + client arkv1client.BackupsGetter + clock clock.Clock + backupLogLevel logrus.Level + newPluginManager func(logrus.FieldLogger) plugin.Manager + backupTracker BackupTracker + backupLocationLister listers.BackupStorageLocationLister + defaultBackupLocation string + snapshotLocationLister listers.VolumeSnapshotLocationLister + defaultSnapshotLocations map[string]string + metrics *metrics.ServerMetrics + newBackupStore func(*api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) } func NewBackupController( @@ -80,21 +82,25 @@ func NewBackupController( backupTracker BackupTracker, backupLocationInformer informers.BackupStorageLocationInformer, defaultBackupLocation string, + volumeSnapshotLocationInformer informers.VolumeSnapshotLocationInformer, + defaultSnapshotLocations map[string]string, metrics *metrics.ServerMetrics, ) Interface { c := &backupController{ - genericController: newGenericController("backup", logger), - backupper: backupper, - pvProviderExists: pvProviderExists, - lister: backupInformer.Lister(), - client: client, - clock: &clock.RealClock{}, - backupLogLevel: backupLogLevel, - newPluginManager: newPluginManager, - backupTracker: backupTracker, - backupLocationLister: backupLocationInformer.Lister(), - defaultBackupLocation: defaultBackupLocation, - metrics: metrics, + genericController: newGenericController("backup", logger), + backupper: backupper, + pvProviderExists: pvProviderExists, + lister: backupInformer.Lister(), + client: client, + clock: &clock.RealClock{}, + backupLogLevel: backupLogLevel, + newPluginManager: newPluginManager, + backupTracker: backupTracker, + backupLocationLister: backupLocationInformer.Lister(), + defaultBackupLocation: defaultBackupLocation, + snapshotLocationLister: volumeSnapshotLocationInformer.Lister(), + defaultSnapshotLocations: defaultSnapshotLocations, + metrics: metrics, newBackupStore: persistence.NewObjectBackupStore, } @@ -103,6 +109,7 @@ func NewBackupController( c.cacheSyncWaiters = append(c.cacheSyncWaiters, backupInformer.Informer().HasSynced, backupLocationInformer.Informer().HasSynced, + volumeSnapshotLocationInformer.Informer().HasSynced, ) backupInformer.Informer().AddEventHandler( @@ -178,9 +185,11 @@ func (c *backupController) processBackup(key string) error { backup.Status.Expiration = metav1.NewTime(c.clock.Now().Add(backup.Spec.TTL.Duration)) } - var backupLocation *api.BackupStorageLocation - // validation - if backupLocation, backup.Status.ValidationErrors = c.getLocationAndValidate(backup, c.defaultBackupLocation); len(backup.Status.ValidationErrors) > 0 { + backupLocation, errs := c.getLocationAndValidate(backup, c.defaultBackupLocation) + errs = append(errs, c.defaultAndValidateSnapshotLocations(backup, c.defaultSnapshotLocations)...) + backup.Status.ValidationErrors = append(backup.Status.ValidationErrors, errs...) + + if len(backup.Status.ValidationErrors) > 0 { backup.Status.Phase = api.BackupPhaseFailedValidation } else { backup.Status.Phase = api.BackupPhaseInProgress @@ -258,10 +267,6 @@ func (c *backupController) getLocationAndValidate(itm *api.Backup, defaultBackup validationErrors = append(validationErrors, fmt.Sprintf("Invalid included/excluded namespace lists: %v", err)) } - if !c.pvProviderExists && itm.Spec.SnapshotVolumes != nil && *itm.Spec.SnapshotVolumes { - validationErrors = append(validationErrors, "Server is not configured for PV snapshots") - } - if itm.Spec.StorageLocation == "" { itm.Spec.StorageLocation = defaultBackupLocation } @@ -281,6 +286,53 @@ func (c *backupController) getLocationAndValidate(itm *api.Backup, defaultBackup return backupLocation, validationErrors } +// defaultAndValidateSnapshotLocations ensures: +// - each location name in Spec VolumeSnapshotLocation exists as a location +// - exactly 1 location per existing or default provider +// - a given default provider's location name is added to the Spec VolumeSnapshotLocation if it does not exist as a VSL +func (c *backupController) defaultAndValidateSnapshotLocations(itm *api.Backup, defaultLocations map[string]string) []string { + var errors []string + perProviderLocationName := make(map[string]string) + var finalLocationNameList []string + for _, locationName := range itm.Spec.VolumeSnapshotLocations { + // validate each locationName exists as a VolumeSnapshotLocation + location, err := c.snapshotLocationLister.VolumeSnapshotLocations(itm.Namespace).Get(locationName) + if err != nil { + errors = append(errors, fmt.Sprintf("error getting volume snapshot location named %s: %v", locationName, err)) + continue + } + + // ensure we end up with exactly 1 locationName *per provider* + providerLocationName := perProviderLocationName[location.Spec.Provider] + if providerLocationName != "" { + // if > 1 location name per provider as in ["aws-us-east-1" | "aws-us-west-1"] (same provider, multiple names) + if providerLocationName != locationName { + errors = append(errors, fmt.Sprintf("more than one VolumeSnapshotLocation name specified for provider %s: %s; unexpected name was %s", location.Spec.Provider, locationName, providerLocationName)) + continue + } + } else { + // no dup exists: add locationName to the final list + finalLocationNameList = append(finalLocationNameList, locationName) + // keep track of all valid existing locations, per provider + perProviderLocationName[location.Spec.Provider] = locationName + } + } + + if len(errors) > 0 { + return errors + } + + for provider, defaultLocationName := range defaultLocations { + // if a location name for a given provider does not already exist, add the provider's default + if _, ok := perProviderLocationName[provider]; !ok { + finalLocationNameList = append(finalLocationNameList, defaultLocationName) + } + } + itm.Spec.VolumeSnapshotLocations = finalLocationNameList + + return nil +} + func (c *backupController) runBackup(backup *api.Backup, backupLocation *api.BackupStorageLocation) error { log := c.logger.WithField("backup", kubeutil.NamespaceAndName(backup)) log.Info("Starting backup") diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 8896cc63d9..36c99317af 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -24,6 +24,7 @@ import ( "testing" "time" + "github.com/pkg/errors" "github.com/sirupsen/logrus" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -66,6 +67,7 @@ func TestProcessBackup(t *testing.T) { backup *arktest.TestBackup expectBackup bool allowSnapshots bool + defaultLocations map[string]string }{ { name: "bad key", @@ -198,6 +200,8 @@ func TestProcessBackup(t *testing.T) { NewBackupTracker(), sharedInformers.Ark().V1().BackupStorageLocations(), "default", + sharedInformers.Ark().V1().VolumeSnapshotLocations(), + test.defaultLocations, metrics.NewServerMetrics(), ).(*backupController) @@ -422,3 +426,121 @@ func TestProcessBackup(t *testing.T) { }) } } + +func TestDefaultAndValidateSnapshotLocations(t *testing.T) { + defaultLocationsAWS := map[string]string{"aws": "aws-us-east-2"} + defaultLocationsFake := map[string]string{"fake-provider": "some-name"} + + multipleLocationNames := []string{"aws-us-west-1", "aws-us-east-1"} + + multipleLocation1 := arktest.LocationInfo{ + Name: multipleLocationNames[0], + Provider: "aws", + Config: map[string]string{"region": "us-west-1"}, + } + multipleLocation2 := arktest.LocationInfo{ + Name: multipleLocationNames[1], + Provider: "aws", + Config: map[string]string{"region": "us-west-1"}, + } + + multipleLocationList := []arktest.LocationInfo{multipleLocation1, multipleLocation2} + + dupLocationNames := []string{"aws-us-west-1", "aws-us-west-1"} + dupLocation1 := arktest.LocationInfo{ + Name: dupLocationNames[0], + Provider: "aws", + Config: map[string]string{"region": "us-west-1"}, + } + dupLocation2 := arktest.LocationInfo{ + Name: dupLocationNames[0], + Provider: "aws", + Config: map[string]string{"region": "us-west-1"}, + } + dupLocationList := []arktest.LocationInfo{dupLocation1, dupLocation2} + + tests := []struct { + name string + backup *arktest.TestBackup + locations []*arktest.TestVolumeSnapshotLocation + defaultLocations map[string]string + expectedVolumeSnapshotLocationNames []string // adding these in the expected order will allow to test with better msgs in case of a test failure + expectedErrors string + expectedSuccess bool + }{ + { + name: "location name does not correspond to any existing location", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations([]string{"random-name"}), + locations: arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList), + expectedErrors: "error getting volume snapshot location named random-name: volumesnapshotlocation.ark.heptio.com \"random-name\" not found", + expectedSuccess: false, + }, + { + name: "duplicate locationName per provider: should filter out dups", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(dupLocationNames), + locations: arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList), + expectedVolumeSnapshotLocationNames: []string{dupLocationNames[0]}, + expectedSuccess: true, + }, + { + name: "multiple location names per provider", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(multipleLocationNames), + locations: arktest.NewTestVolumeSnapshotLocation().WithName(multipleLocationNames[0]).WithProviderConfig(multipleLocationList), + expectedErrors: "more than one VolumeSnapshotLocation name specified for provider aws: aws-us-east-1; unexpected name was aws-us-west-1", + expectedSuccess: false, + }, + { + name: "no location name for the provider exists: the provider's default should be added", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew), + defaultLocations: defaultLocationsAWS, + expectedVolumeSnapshotLocationNames: []string{defaultLocationsAWS["aws"]}, + expectedSuccess: true, + }, + { + name: "no existing location name and no default location name given", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew), + expectedSuccess: true, + }, + { + name: "multiple location names for a provider, default location name for another provider", + backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations(dupLocationNames), + locations: arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList), + defaultLocations: defaultLocationsFake, + expectedVolumeSnapshotLocationNames: []string{dupLocationNames[0], defaultLocationsFake["fake-provider"]}, + expectedSuccess: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var ( + client = fake.NewSimpleClientset() + sharedInformers = informers.NewSharedInformerFactory(client, 0) + ) + + c := &backupController{ + snapshotLocationLister: sharedInformers.Ark().V1().VolumeSnapshotLocations().Lister(), + } + + // set up a Backup object to represent what we expect to be passed to backupper.Backup() + backup := test.backup.DeepCopy() + backup.Spec.VolumeSnapshotLocations = test.backup.Spec.VolumeSnapshotLocations + for _, location := range test.locations { + require.NoError(t, sharedInformers.Ark().V1().VolumeSnapshotLocations().Informer().GetStore().Add(location.VolumeSnapshotLocation)) + } + + errs := c.defaultAndValidateSnapshotLocations(backup, test.defaultLocations) + if test.expectedSuccess { + for _, err := range errs { + require.NoError(t, errors.New(err), "defaultAndValidateSnapshotLocations unexpected error: %v", err) + } + require.Equal(t, test.expectedVolumeSnapshotLocationNames, backup.Spec.VolumeSnapshotLocations) + } else { + if len(errs) == 0 { + require.Error(t, nil, "defaultAndValidateSnapshotLocations expected error") + } + require.Contains(t, errs, test.expectedErrors) + } + }) + } +} diff --git a/pkg/util/test/test_backup.go b/pkg/util/test/test_backup.go index 041dd9e848..e3a764daf9 100644 --- a/pkg/util/test/test_backup.go +++ b/pkg/util/test/test_backup.go @@ -140,3 +140,8 @@ func (b *TestBackup) WithStorageLocation(location string) *TestBackup { b.Spec.StorageLocation = location return b } + +func (b *TestBackup) WithVolumeSnapshotLocations(locations []string) *TestBackup { + b.Spec.VolumeSnapshotLocations = locations + return b +} diff --git a/pkg/util/test/test_volume_snapshot_location.go b/pkg/util/test/test_volume_snapshot_location.go new file mode 100644 index 0000000000..5bfb7e496e --- /dev/null +++ b/pkg/util/test/test_volume_snapshot_location.go @@ -0,0 +1,72 @@ +/* +Copyright 2018 the Heptio Ark contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package test + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/heptio/ark/pkg/apis/ark/v1" +) + +type TestVolumeSnapshotLocation struct { + *v1.VolumeSnapshotLocation +} + +func NewTestVolumeSnapshotLocation() *TestVolumeSnapshotLocation { + return &TestVolumeSnapshotLocation{ + VolumeSnapshotLocation: &v1.VolumeSnapshotLocation{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: v1.DefaultNamespace, + }, + Spec: v1.VolumeSnapshotLocationSpec{ + Provider: "aws", + Config: map[string]string{"region": "us-west-1"}, + }, + }, + } +} + +func (location *TestVolumeSnapshotLocation) WithName(name string) *TestVolumeSnapshotLocation { + location.Name = name + return location +} + +func (location *TestVolumeSnapshotLocation) WithProviderConfig(info []LocationInfo) []*TestVolumeSnapshotLocation { + var locations []*TestVolumeSnapshotLocation + + for _, v := range info { + location := &TestVolumeSnapshotLocation{ + VolumeSnapshotLocation: &v1.VolumeSnapshotLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: v.Name, + Namespace: v1.DefaultNamespace, + }, + Spec: v1.VolumeSnapshotLocationSpec{ + Provider: v.Provider, + Config: v.Config, + }, + }, + } + locations = append(locations, location) + } + return locations +} + +type LocationInfo struct { + Name, Provider string + Config map[string]string +}