don't require a default provider VSL if there's only 1

Signed-off-by: Steve Kriss <steve@heptio.com>

pkg/cmd/server/server.go

@@ -273,60 +273,17 @@ func (s *server) run() error {
 			Warnf("Default backup storage location %q not found; backups must explicitly specify a location", s.config.defaultBackupLocation)
 	}
 
-	defaultVolumeSnapshotLocations, err := getDefaultVolumeSnapshotLocations(s.arkClient, s.namespace, s.config.defaultVolumeSnapshotLocations)
-	if err != nil {
-		return err
-	}
-
 	if err := s.initRestic(); err != nil {
 		return err
 	}
 
-	if err := s.runControllers(defaultVolumeSnapshotLocations); err != nil {
+	if err := s.runControllers(s.config.defaultVolumeSnapshotLocations); err != nil {
 		return err
 	}
 
 	return nil
 }
 
-func getDefaultVolumeSnapshotLocations(arkClient clientset.Interface, namespace string, defaultVolumeSnapshotLocations map[string]string) (map[string]*api.VolumeSnapshotLocation, error) {
-	providerDefaults := make(map[string]*api.VolumeSnapshotLocation)
-	if len(defaultVolumeSnapshotLocations) == 0 {
-		return providerDefaults, nil
-	}
-
-	volumeSnapshotLocations, err := arkClient.ArkV1().VolumeSnapshotLocations(namespace).List(metav1.ListOptions{})
-	if err != nil {
-		return providerDefaults, errors.WithStack(err)
-	}
-
-	providerLocations := make(map[string][]*api.VolumeSnapshotLocation)
-	for i, vsl := range volumeSnapshotLocations.Items {
-		locations := providerLocations[vsl.Spec.Provider]
-		providerLocations[vsl.Spec.Provider] = append(locations, &volumeSnapshotLocations.Items[i])
-	}
-
-	for provider, locations := range providerLocations {
-		defaultLocation, ok := defaultVolumeSnapshotLocations[provider]
-		if !ok {
-			return providerDefaults, errors.Errorf("missing provider %s. When using default volume snapshot locations, one must exist for every known provider.", provider)
-		}
-
-		for _, location := range locations {
-			if location.ObjectMeta.Name == defaultLocation {
-				providerDefaults[provider] = location
-				break
-			}
-		}
-
-		if _, ok := providerDefaults[provider]; !ok {
-			return providerDefaults, errors.Errorf("%s is not a valid volume snapshot location for %s", defaultLocation, provider)
-		}
-	}
-
-	return providerDefaults, nil
-}
-
 // namespaceExists returns nil if namespace can be successfully
 // gotten from the kubernetes API, or an error otherwise.
 func (s *server) namespaceExists(namespace string) error {
@@ -510,7 +467,7 @@ func (s *server) initRestic() error {
 	return nil
 }
 
-func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]*api.VolumeSnapshotLocation) error {
+func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string) error {
 	s.logger.Info("Starting controllers")
 
 	ctx := s.ctx

pkg/cmd/server/server_test.go

@@ -23,7 +23,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/heptio/ark/pkg/apis/ark/v1"
-	fakeclientset "github.com/heptio/ark/pkg/generated/clientset/versioned/fake"
 	arktest "github.com/heptio/ark/pkg/util/test"
 )
 
@@ -70,59 +69,3 @@ func TestArkResourcesExist(t *testing.T) {
 	arkAPIResourceList.APIResources = arkAPIResourceList.APIResources[:3]
 	assert.Error(t, server.arkResourcesExist())
 }
-
-func TestDefaultVolumeSnapshotLocations(t *testing.T) {
-	namespace := "heptio-ark"
-	arkClient := fakeclientset.NewSimpleClientset()
-
-	location := &v1.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "location1"}, Spec: v1.VolumeSnapshotLocationSpec{Provider: "provider1"}}
-	arkClient.ArkV1().VolumeSnapshotLocations(namespace).Create(location)
-
-	defaultVolumeSnapshotLocations := make(map[string]string)
-
-	// No defaults
-	volumeSnapshotLocations, err := getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations)
-	assert.Equal(t, 0, len(volumeSnapshotLocations))
-	assert.NoError(t, err)
-
-	// Bad location
-	defaultVolumeSnapshotLocations["provider1"] = "badlocation"
-	volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations)
-	assert.Equal(t, 0, len(volumeSnapshotLocations))
-	assert.Error(t, err)
-
-	// Bad provider
-	defaultVolumeSnapshotLocations["provider2"] = "badlocation"
-	volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations)
-	assert.Equal(t, 0, len(volumeSnapshotLocations))
-	assert.Error(t, err)
-
-	// Good provider, good location
-	delete(defaultVolumeSnapshotLocations, "provider2")
-	defaultVolumeSnapshotLocations["provider1"] = "location1"
-	volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations)
-	assert.Equal(t, 1, len(volumeSnapshotLocations))
-	assert.NoError(t, err)
-
-	location2 := &v1.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "location2"}, Spec: v1.VolumeSnapshotLocationSpec{Provider: "provider2"}}
-	arkClient.ArkV1().VolumeSnapshotLocations(namespace).Create(location2)
-
-	// Mutliple Provider/Location 1 good, 1 bad
-	defaultVolumeSnapshotLocations["provider2"] = "badlocation"
-	volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations)
-	assert.Error(t, err)
-
-	location21 := &v1.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "location2-1"}, Spec: v1.VolumeSnapshotLocationSpec{Provider: "provider2"}}
-	arkClient.ArkV1().VolumeSnapshotLocations(namespace).Create(location21)
-
-	location11 := &v1.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "location1-1"}, Spec: v1.VolumeSnapshotLocationSpec{Provider: "provider1"}}
-	arkClient.ArkV1().VolumeSnapshotLocations(namespace).Create(location11)
-
-	// Mutliple Provider/Location all good
-	defaultVolumeSnapshotLocations["provider2"] = "location2"
-	volumeSnapshotLocations, err = getDefaultVolumeSnapshotLocations(arkClient, namespace, defaultVolumeSnapshotLocations)
-	assert.Equal(t, 2, len(volumeSnapshotLocations))
-	assert.NoError(t, err)
-	assert.Equal(t, volumeSnapshotLocations["provider1"].ObjectMeta.Name, "location1")
-	assert.Equal(t, volumeSnapshotLocations["provider2"].ObjectMeta.Name, "location2")
-}

pkg/controller/backup_controller.go

@@ -30,6 +30,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/clock"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -65,7 +66,7 @@ type backupController struct {
 	backupLocationLister     listers.BackupStorageLocationLister
 	defaultBackupLocation    string
 	snapshotLocationLister   listers.VolumeSnapshotLocationLister
-	defaultSnapshotLocations map[string]*api.VolumeSnapshotLocation
+	defaultSnapshotLocations map[string]string
 	metrics                  *metrics.ServerMetrics
 	newBackupStore           func(*api.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error)
 }
@@ -81,7 +82,7 @@ func NewBackupController(
 	backupLocationInformer informers.BackupStorageLocationInformer,
 	defaultBackupLocation string,
 	volumeSnapshotLocationInformer informers.VolumeSnapshotLocationInformer,
-	defaultSnapshotLocations map[string]*api.VolumeSnapshotLocation,
+	defaultSnapshotLocations map[string]string,
 	metrics *metrics.ServerMetrics,
 ) Interface {
 	c := &backupController{
@@ -298,7 +299,8 @@ func (c *backupController) prepareBackupRequest(backup *api.Backup) *pkgbackup.R
 // - each location name in .spec.volumeSnapshotLocations exists as a location
 // - exactly 1 location per provider
 // - a given provider's default location name is added to .spec.volumeSnapshotLocations if one
-//   is not explicitly specified for the provider
+//   is not explicitly specified for the provider (if there's only one location for the provider,
+//   it will automatically be used)
 func (c *backupController) validateAndGetSnapshotLocations(backup *api.Backup) (map[string]*api.VolumeSnapshotLocation, []string) {
 	errors := []string{}
 	providerLocations := make(map[string]*api.VolumeSnapshotLocation)
@@ -328,11 +330,51 @@ func (c *backupController) validateAndGetSnapshotLocations(backup *api.Backup) (
 		return nil, errors
 	}
 
-	for provider, defaultLocation := range c.defaultSnapshotLocations {
-		// if a location name for a given provider does not already exist, add the provider's default
-		if _, ok := providerLocations[provider]; !ok {
-			providerLocations[provider] = defaultLocation
+	allLocations, err := c.snapshotLocationLister.VolumeSnapshotLocations(backup.Namespace).List(labels.Everything())
+	if err != nil {
+		errors = append(errors, fmt.Sprintf("error listing volume snapshot locations: %v", err))
+		return nil, errors
+	}
+
+	// build a map of provider->list of all locations for the provider
+	allProviderLocations := make(map[string][]*api.VolumeSnapshotLocation)
+	for i := range allLocations {
+		loc := allLocations[i]
+		allProviderLocations[loc.Spec.Provider] = append(allProviderLocations[loc.Spec.Provider], loc)
+	}
+
+	// go through each provider and make sure we have/can get a VSL
+	// for it
+	for provider, locations := range allProviderLocations {
+		if _, ok := providerLocations[provider]; ok {
+			// backup's spec had a location named for this provider
+			continue
 		}
+
+		if len(locations) > 1 {
+			// more than one possible location for the provider: check
+			// the defaults
+			defaultLocation := c.defaultSnapshotLocations[provider]
+			if defaultLocation == "" {
+				errors = append(errors, fmt.Sprintf("provider %s has more than one possible volume snapshot location, and none were specified explicitly or as a default", provider))
+				continue
+			}
+			location, err := c.snapshotLocationLister.VolumeSnapshotLocations(backup.Namespace).Get(defaultLocation)
+			if err != nil {
+				errors = append(errors, fmt.Sprintf("error getting volume snapshot location named %s: %v", defaultLocation, err))
+				continue
+			}
+
+			providerLocations[provider] = location
+			continue
+		}
+
+		// exactly one location for the provider: use it
+		providerLocations[provider] = locations[0]
+	}
+
+	if len(errors) > 0 {
+		return nil, errors
 	}
 
 	return providerLocations, nil

pkg/controller/backup_controller_test.go

@@ -163,11 +163,12 @@ func TestProcessBackupValidationFailures(t *testing.T) {
 			)
 
 			c := &backupController{
-				genericController:     newGenericController("backup-test", logger),
-				client:                clientset.ArkV1(),
-				lister:                sharedInformers.Ark().V1().Backups().Lister(),
-				backupLocationLister:  sharedInformers.Ark().V1().BackupStorageLocations().Lister(),
-				defaultBackupLocation: defaultBackupLocation.Name,
+				genericController:      newGenericController("backup-test", logger),
+				client:                 clientset.ArkV1(),
+				lister:                 sharedInformers.Ark().V1().Backups().Lister(),
+				backupLocationLister:   sharedInformers.Ark().V1().BackupStorageLocations().Lister(),
+				snapshotLocationLister: sharedInformers.Ark().V1().VolumeSnapshotLocations().Lister(),
+				defaultBackupLocation:  defaultBackupLocation.Name,
 			}
 
 			require.NotNil(t, test.backup)
@@ -294,15 +295,16 @@ func TestProcessBackupCompletions(t *testing.T) {
 			)
 
 			c := &backupController{
-				genericController:     newGenericController("backup-test", logger),
-				client:                clientset.ArkV1(),
-				lister:                sharedInformers.Ark().V1().Backups().Lister(),
-				backupLocationLister:  sharedInformers.Ark().V1().BackupStorageLocations().Lister(),
-				defaultBackupLocation: defaultBackupLocation.Name,
-				backupTracker:         NewBackupTracker(),
-				metrics:               metrics.NewServerMetrics(),
-				clock:                 clock.NewFakeClock(now),
-				newPluginManager:      func(logrus.FieldLogger) plugin.Manager { return pluginManager },
+				genericController:      newGenericController("backup-test", logger),
+				client:                 clientset.ArkV1(),
+				lister:                 sharedInformers.Ark().V1().Backups().Lister(),
+				backupLocationLister:   sharedInformers.Ark().V1().BackupStorageLocations().Lister(),
+				snapshotLocationLister: sharedInformers.Ark().V1().VolumeSnapshotLocations().Lister(),
+				defaultBackupLocation:  defaultBackupLocation.Name,
+				backupTracker:          NewBackupTracker(),
+				metrics:                metrics.NewServerMetrics(),
+				clock:                  clock.NewFakeClock(now),
+				newPluginManager:       func(logrus.FieldLogger) plugin.Manager { return pluginManager },
 				newBackupStore: func(*v1.BackupStorageLocation, persistence.ObjectStoreGetter, logrus.FieldLogger) (persistence.BackupStore, error) {
 					return backupStore, nil
 				},
@@ -351,76 +353,74 @@ func TestProcessBackupCompletions(t *testing.T) {
 }
 
 func TestValidateAndGetSnapshotLocations(t *testing.T) {
-	defaultLocationsAWS := map[string]*v1.VolumeSnapshotLocation{
-		"aws": arktest.NewTestVolumeSnapshotLocation().WithName("aws-us-east-2").VolumeSnapshotLocation,
-	}
-	defaultLocationsFake := map[string]*v1.VolumeSnapshotLocation{
-		"fake-provider": arktest.NewTestVolumeSnapshotLocation().WithName("some-name").VolumeSnapshotLocation,
-	}
-
-	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]*v1.VolumeSnapshotLocation
+		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("random-name"),
-			locations:       arktest.NewTestVolumeSnapshotLocation().WithName(dupLocationNames[0]).WithProviderConfig(dupLocationList),
+			name:   "location name does not correspond to any existing location",
+			backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations("random-name"),
+			locations: []*arktest.TestVolumeSnapshotLocation{
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"),
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"),
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("fake-provider").WithName("some-name"),
+			},
 			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]},
+			name:   "duplicate locationName per provider: should filter out dups",
+			backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithVolumeSnapshotLocations("aws-us-west-1", "aws-us-west-1"),
+			locations: []*arktest.TestVolumeSnapshotLocation{
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"),
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"),
+			},
+			expectedVolumeSnapshotLocationNames: []string{"aws-us-west-1"},
 			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",
+			name:   "multiple non-dupe location names per provider should error",
+			backup: arktest.NewTestBackup().WithName("backup1").WithVolumeSnapshotLocations("aws-us-east-1", "aws-us-west-1"),
+			locations: []*arktest.TestVolumeSnapshotLocation{
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"),
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"),
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("fake-provider").WithName("some-name"),
+			},
+			expectedErrors:  "more than one VolumeSnapshotLocation name specified for provider aws: aws-us-west-1; unexpected name was aws-us-east-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"].Name},
+			name:   "no location name for the provider exists, only one VSL for the provider: use it",
+			backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew),
+			locations: []*arktest.TestVolumeSnapshotLocation{
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"),
+			},
+			expectedVolumeSnapshotLocationNames: []string{"aws-us-east-1"},
+			expectedSuccess:                     true,
+		},
+		{
+			name:   "no location name for the provider exists, no default, more than one VSL for the provider: error",
+			backup: arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew),
+			locations: []*arktest.TestVolumeSnapshotLocation{
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-east-1"),
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"),
+			},
+			expectedErrors: "provider aws has more than one possible volume snapshot location, and none were specified explicitly or as a default",
+		},
+		{
+			name:             "no location name for the provider exists, more than one VSL for the provider: the provider's default should be added",
+			backup:           arktest.NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew),
+			defaultLocations: map[string]string{"aws": "aws-us-east-1"},
+			locations: []*arktest.TestVolumeSnapshotLocation{
+				arktest.NewTestVolumeSnapshotLocation().WithName("aws-us-east-1").WithProvider("aws"),
+				arktest.NewTestVolumeSnapshotLocation().WithName("aws-us-west-1").WithProvider("aws"),
+			},
+			expectedVolumeSnapshotLocationNames: []string{"aws-us-east-1"},
 			expectedSuccess:                     true,
 		},
 		{
@@ -429,11 +429,14 @@ func TestValidateAndGetSnapshotLocations(t *testing.T) {
 			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"].Name},
+			name:             "multiple location names for a provider, default location name for another provider",
+			backup:           arktest.NewTestBackup().WithName("backup1").WithVolumeSnapshotLocations("aws-us-west-1", "aws-us-west-1"),
+			defaultLocations: map[string]string{"fake-provider": "some-name"},
+			locations: []*arktest.TestVolumeSnapshotLocation{
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("aws").WithName("aws-us-west-1"),
+				arktest.NewTestVolumeSnapshotLocation().WithProvider("fake-provider").WithName("some-name"),
+			},
+			expectedVolumeSnapshotLocationNames: []string{"aws-us-west-1", "some-name"},
 			expectedSuccess:                     true,
 		},
 	}