make --snapshot-volumes, --restore-volumes optional with sensible default behavior based on PVProvider existence

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

pkg/controller/backup_controller.go

@@ -49,10 +49,10 @@ import (
 const backupVersion = 1
 
 type backupController struct {
-	backupper      backup.Backupper
-	backupService  cloudprovider.BackupService
-	bucket         string
-	allowSnapshots bool
+	backupper        backup.Backupper
+	backupService    cloudprovider.BackupService
+	bucket           string
+	pvProviderExists bool
 
 	lister       listers.BackupLister
 	listerSynced cache.InformerSynced
@@ -69,13 +69,13 @@ func NewBackupController(
 	backupper backup.Backupper,
 	backupService cloudprovider.BackupService,
 	bucket string,
-	allowSnapshots bool,
+	pvProviderExists bool,
 ) Interface {
 	c := &backupController{
-		backupper:      backupper,
-		backupService:  backupService,
-		bucket:         bucket,
-		allowSnapshots: allowSnapshots,
+		backupper:        backupper,
+		backupService:    backupService,
+		bucket:           bucket,
+		pvProviderExists: pvProviderExists,
 
 		lister:       backupInformer.Lister(),
 		listerSynced: backupInformer.Informer().HasSynced,
@@ -300,7 +300,7 @@ func (controller *backupController) getValidationErrors(itm *api.Backup) []strin
 		validationErrors = append(validationErrors, fmt.Sprintf("Invalid included/excluded namespace lists: %v", err))
 	}
 
-	if !controller.allowSnapshots && itm.Spec.SnapshotVolumes {
+	if !controller.pvProviderExists && itm.Spec.SnapshotVolumes != nil && *itm.Spec.SnapshotVolumes {
 		validationErrors = append(validationErrors, "Server is not configured for PV snapshots")
 	}
 

pkg/controller/backup_controller_test.go

@@ -243,7 +243,7 @@ func TestProcessBackup(t *testing.T) {
 						WithExcludedResources(test.expectedExcludes...).
 						WithIncludedNamespaces(expectedNSes...).
 						WithTTL(test.backup.Spec.TTL.Duration).
-						WithSnapshotVolumes(test.backup.Spec.SnapshotVolumes).
+						WithSnapshotVolumesPointer(test.backup.Spec.SnapshotVolumes).
 						WithExpiration(expiration).
 						WithVersion(1).
 						Backup,
@@ -259,7 +259,7 @@ func TestProcessBackup(t *testing.T) {
 						WithExcludedResources(test.expectedExcludes...).
 						WithIncludedNamespaces(expectedNSes...).
 						WithTTL(test.backup.Spec.TTL.Duration).
-						WithSnapshotVolumes(test.backup.Spec.SnapshotVolumes).
+						WithSnapshotVolumesPointer(test.backup.Spec.SnapshotVolumes).
 						WithExpiration(expiration).
 						WithVersion(1).
 						Backup,

pkg/controller/restore_controller.go

@@ -42,12 +42,12 @@ import (
 )
 
 type restoreController struct {
-	restoreClient         arkv1client.RestoresGetter
-	backupClient          arkv1client.BackupsGetter
-	restorer              restore.Restorer
-	backupService         cloudprovider.BackupService
-	bucket                string
-	allowSnapshotRestores bool
+	restoreClient    arkv1client.RestoresGetter
+	backupClient     arkv1client.BackupsGetter
+	restorer         restore.Restorer
+	backupService    cloudprovider.BackupService
+	bucket           string
+	pvProviderExists bool
 
 	backupLister        listers.BackupLister
 	backupListerSynced  cache.InformerSynced
@@ -65,20 +65,20 @@ func NewRestoreController(
 	backupService cloudprovider.BackupService,
 	bucket string,
 	backupInformer informers.BackupInformer,
-	allowSnapshotRestores bool,
+	pvProviderExists bool,
 ) Interface {
 	c := &restoreController{
-		restoreClient:         restoreClient,
-		backupClient:          backupClient,
-		restorer:              restorer,
-		backupService:         backupService,
-		bucket:                bucket,
-		allowSnapshotRestores: allowSnapshotRestores,
-		backupLister:          backupInformer.Lister(),
-		backupListerSynced:    backupInformer.Informer().HasSynced,
-		restoreLister:         restoreInformer.Lister(),
-		restoreListerSynced:   restoreInformer.Informer().HasSynced,
-		queue:                 workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "restore"),
+		restoreClient:       restoreClient,
+		backupClient:        backupClient,
+		restorer:            restorer,
+		backupService:       backupService,
+		bucket:              bucket,
+		pvProviderExists:    pvProviderExists,
+		backupLister:        backupInformer.Lister(),
+		backupListerSynced:  backupInformer.Informer().HasSynced,
+		restoreLister:       restoreInformer.Lister(),
+		restoreListerSynced: restoreInformer.Informer().HasSynced,
+		queue:               workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "restore"),
 	}
 
 	c.syncHandler = c.processRestore
@@ -278,7 +278,7 @@ func (controller *restoreController) getValidationErrors(itm *api.Restore) []str
 		validationErrors = append(validationErrors, "BackupName must be non-empty and correspond to the name of a backup in object storage.")
 	}
 
-	if !controller.allowSnapshotRestores && itm.Spec.RestorePVs {
+	if !controller.pvProviderExists && itm.Spec.RestorePVs != nil && *itm.Spec.RestorePVs {
 		validationErrors = append(validationErrors, "Server is not configured for PV snapshot restores")
 	}
 

pkg/controller/schedule_controller_test.go

@@ -382,7 +382,6 @@ func TestGetBackup(t *testing.T) {
 						IncludedResources:  []string{"foo", "bar"},
 						ExcludedResources:  []string{"baz"},
 						LabelSelector:      &metav1.LabelSelector{MatchLabels: map[string]string{"label": "value"}},
-						SnapshotVolumes:    true,
 						TTL:                metav1.Duration{Duration: time.Duration(300)},
 					},
 				},
@@ -399,7 +398,6 @@ func TestGetBackup(t *testing.T) {
 					IncludedResources:  []string{"foo", "bar"},
 					ExcludedResources:  []string{"baz"},
 					LabelSelector:      &metav1.LabelSelector{MatchLabels: map[string]string{"label": "value"}},
-					SnapshotVolumes:    true,
 					TTL:                metav1.Duration{Duration: time.Duration(300)},
 				},
 			},