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