Default and validate VolumeSnapshotLocations

Signed-off-by: Carlisia <carlisia@grokkingtech.io>

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)

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")

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)
+			}
+		})
+	}
+}

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
+}

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
+}