make PVProvider optional in server config; disallow snap/restore PVs …

…when not provided

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

docs/config-definition.md

@@ -50,7 +50,7 @@ The configurable parameters are as follows:
 
 | Key | Type | Default | Meaning |
 | --- | --- | --- | --- |
-| `persistentVolumeProvider` | CloudProviderConfig<br><br>(Supported key values are `aws`, `gcp`, and `azure`, but only one can be present. See the corresponding [AWS][0], [GCP][1], and [Azure][2]-specific configs.) | Required Field | The specification for whichever cloud provider the cluster is using for persistent volumes (to be snapshotted).<br><br> *NOTE*: For Azure, your Kubernetes cluster needs to be version 1.7.2+ in order to support PV snapshotting of its managed disks. |
+| `persistentVolumeProvider` | CloudProviderConfig<br><br>(Supported key values are `aws`, `gcp`, and `azure`, but only one can be present. See the corresponding [AWS][0], [GCP][1], and [Azure][2]-specific configs.) | None (Optional) | The specification for whichever cloud provider the cluster is using for persistent volumes (to be snapshotted), if any.<br><br>If not specified, Backups and Restores requesting PV snapshots & restores, respectively, are considered invalid. <br><br> *NOTE*: For Azure, your Kubernetes cluster needs to be version 1.7.2+ in order to support PV snapshotting of its managed disks. |
 | `backupStorageProvider`/(inline) | CloudProviderConfig<br><br>(Supported key values are `aws`, `gcp`, and `azure`, but only one can be present. See the corresponding [AWS][0], [GCP][1], and [Azure][2]-specific configs.) | Required Field | The specification for whichever cloud provider will be used to actually store the backups. |
 | `backupStorageProvider/bucket` | String | Required Field | The storage bucket where backups are to be uploaded. |
 | `backupSyncPeriod` | metav1.Duration | 60m0s | How frequently Ark queries the object storage to make sure that the appropriate Backup resources have been created for existing backup files. |

examples/minio/10-ark-config.yaml

@@ -18,12 +18,6 @@ kind: Config
 metadata:
   namespace: heptio-ark
   name: default
-persistentVolumeProvider:
-  aws:
-    region: minio
-    availabilityZone: minio
-    s3ForcePathStyle: true
-    s3Url: http://minio:9000
 backupStorageProvider:
   bucket: ark
   aws:

pkg/apis/ark/v1/config.go

@@ -35,8 +35,8 @@ type Config struct {
 	metav1.ObjectMeta `json:"metadata"`
 
 	// PersistentVolumeProvider is the configuration information for the cloud where
-	// the cluster is running and has PersistentVolumes to snapshot or restore.
-	PersistentVolumeProvider CloudProviderConfig `json:"persistentVolumeProvider"`
+	// the cluster is running and has PersistentVolumes to snapshot or restore. Optional.
+	PersistentVolumeProvider *CloudProviderConfig `json:"persistentVolumeProvider"`
 
 	// BackupStorageProvider is the configuration information for the cloud where
 	// Ark backups are stored in object storage. This may be a different cloud than

pkg/backup/volume_snapshot_action.go

@@ -17,6 +17,7 @@ limitations under the License.
 package backup
 
 import (
+	"errors"
 	"fmt"
 	"regexp"
 
@@ -38,11 +39,15 @@ type volumeSnapshotAction struct {
 
 var _ Action = &volumeSnapshotAction{}
 
-func NewVolumeSnapshotAction(snapshotService cloudprovider.SnapshotService) Action {
+func NewVolumeSnapshotAction(snapshotService cloudprovider.SnapshotService) (Action, error) {
+	if snapshotService == nil {
+		return nil, errors.New("snapshotService cannot be nil")
+	}
+
 	return &volumeSnapshotAction{
 		snapshotService: snapshotService,
 		clock:           clock.RealClock{},
-	}
+	}, nil
 }
 
 // Execute triggers a snapshot for the volume/disk underlying a PersistentVolume if the provided

pkg/backup/volume_snapshot_action_test.go

@@ -155,7 +155,10 @@ func TestVolumeSnapshotAction(t *testing.T) {
 			}
 
 			snapshotService := &FakeSnapshotService{SnapshottableVolumes: test.volumeInfo}
-			action := NewVolumeSnapshotAction(snapshotService).(*volumeSnapshotAction)
+
+			vsa, _ := NewVolumeSnapshotAction(snapshotService)
+			action := vsa.(*volumeSnapshotAction)
+
 			fakeClock := clock.NewFakeClock(time.Now())
 			action.clock = fakeClock
 

pkg/cmd/server/server.go

@@ -251,8 +251,13 @@ func (s *server) initBackupService(config *api.Config) error {
 }
 
 func (s *server) initSnapshotService(config *api.Config) error {
+	if config.PersistentVolumeProvider == nil {
+		glog.Infof("PersistentVolumeProvider config not provided, skipping SnapshotService creation")
+		return nil
+	}
+
 	glog.Infof("Configuring cloud provider for snapshot service")
-	cloud, err := initCloud(config.PersistentVolumeProvider, "persistentVolumeProvider")
+	cloud, err := initCloud(*config.PersistentVolumeProvider, "persistentVolumeProvider")
 	if err != nil {
 		return err
 	}
@@ -408,6 +413,7 @@ func (s *server) runControllers(config *api.Config) error {
 			backupper,
 			s.backupService,
 			config.BackupStorageProvider.Bucket,
+			s.snapshotService != nil,
 		)
 		wg.Add(1)
 		go func() {
@@ -461,6 +467,7 @@ func (s *server) runControllers(config *api.Config) error {
 		s.backupService,
 		config.BackupStorageProvider.Bucket,
 		s.sharedInformerFactory.Ark().V1().Backups(),
+		s.snapshotService != nil,
 	)
 	wg.Add(1)
 	go func() {
@@ -490,7 +497,12 @@ func newBackupper(
 	actions := map[string]backup.Action{}
 
 	if snapshotService != nil {
-		actions["persistentvolumes"] = backup.NewVolumeSnapshotAction(snapshotService)
+		action, err := backup.NewVolumeSnapshotAction(snapshotService)
+		if err != nil {
+			return nil, err
+		}
+
+		actions["persistentvolumes"] = action
 	}
 
 	return backup.NewKubernetesBackupper(

pkg/controller/backup_controller.go

@@ -49,9 +49,10 @@ import (
 const backupVersion = 1
 
 type backupController struct {
-	backupper     backup.Backupper
-	backupService cloudprovider.BackupService
-	bucket        string
+	backupper      backup.Backupper
+	backupService  cloudprovider.BackupService
+	bucket         string
+	allowSnapshots bool
 
 	lister       listers.BackupLister
 	listerSynced cache.InformerSynced
@@ -68,11 +69,13 @@ func NewBackupController(
 	backupper backup.Backupper,
 	backupService cloudprovider.BackupService,
 	bucket string,
+	allowSnapshots bool,
 ) Interface {
 	c := &backupController{
-		backupper:     backupper,
-		backupService: backupService,
-		bucket:        bucket,
+		backupper:      backupper,
+		backupService:  backupService,
+		bucket:         bucket,
+		allowSnapshots: allowSnapshots,
 
 		lister:       backupInformer.Lister(),
 		listerSynced: backupInformer.Informer().HasSynced,
@@ -297,6 +300,10 @@ 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 {
+		validationErrors = append(validationErrors, "Server is not configured for PV snapshots")
+	}
+
 	return validationErrors
 }
 

pkg/controller/backup_controller_test.go

@@ -54,6 +54,7 @@ func TestProcessBackup(t *testing.T) {
 		expectedExcludes []string
 		backup           *TestBackup
 		expectBackup     bool
+		allowSnapshots   bool
 	}{
 		{
 			name:        "bad key",
@@ -129,6 +130,20 @@ func TestProcessBackup(t *testing.T) {
 			expectedIncludes: []string{"*"},
 			expectBackup:     true,
 		},
+		{
+			name:         "backup with SnapshotVolumes when allowSnapshots=false fails validation",
+			key:          "heptio-ark/backup1",
+			backup:       NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithSnapshotVolumes(true),
+			expectBackup: false,
+		},
+		{
+			name:             "backup with SnapshotVolumes when allowSnapshots=true gets executed",
+			key:              "heptio-ark/backup1",
+			backup:           NewTestBackup().WithName("backup1").WithPhase(v1.BackupPhaseNew).WithSnapshotVolumes(true),
+			allowSnapshots:   true,
+			expectedIncludes: []string{"*"},
+			expectBackup:     true,
+		},
 	}
 
 	// flag.Set("logtostderr", "true")
@@ -150,6 +165,7 @@ func TestProcessBackup(t *testing.T) {
 				backupper,
 				cloudBackups,
 				"bucket",
+				test.allowSnapshots,
 			).(*backupController)
 			c.clock = clock.NewFakeClock(time.Now())
 
@@ -180,6 +196,7 @@ func TestProcessBackup(t *testing.T) {
 				}
 
 				backup.Spec.IncludedNamespaces = expectedNSes
+				backup.Spec.SnapshotVolumes = test.backup.Spec.SnapshotVolumes
 				backup.Status.Phase = v1.BackupPhaseInProgress
 				backup.Status.Expiration.Time = expiration
 				backup.Status.Version = 1
@@ -226,6 +243,7 @@ func TestProcessBackup(t *testing.T) {
 						WithExcludedResources(test.expectedExcludes...).
 						WithIncludedNamespaces(expectedNSes...).
 						WithTTL(test.backup.Spec.TTL.Duration).
+						WithSnapshotVolumes(test.backup.Spec.SnapshotVolumes).
 						WithExpiration(expiration).
 						WithVersion(1).
 						Backup,
@@ -241,6 +259,7 @@ func TestProcessBackup(t *testing.T) {
 						WithExcludedResources(test.expectedExcludes...).
 						WithIncludedNamespaces(expectedNSes...).
 						WithTTL(test.backup.Spec.TTL.Duration).
+						WithSnapshotVolumes(test.backup.Spec.SnapshotVolumes).
 						WithExpiration(expiration).
 						WithVersion(1).
 						Backup,

pkg/controller/gc_controller.go

@@ -115,6 +115,12 @@ func (c *gcController) cleanBackups() {
 			}
 
 			for _, volumeBackup := range backup.Status.VolumeBackups {
+				if c.snapshotService == nil {
+					glog.Warningf("Cannot garbage-collect snapshot %s for %backup s/%s because server is not configured with PersistentVolumeProvider",
+						volumeBackup.SnapshotID, backup.Namespace, backup.Name)
+					continue
+				}
+
 				glog.Infof("Removing snapshot %s associated with backup %s/%s", volumeBackup.SnapshotID, backup.Namespace, backup.Name)
 				if err := c.snapshotService.DeleteSnapshot(volumeBackup.SnapshotID); err != nil {
 					glog.Errorf("error deleting snapshot %v: %v", volumeBackup.SnapshotID, err)

pkg/controller/gc_controller_test.go

@@ -31,16 +31,18 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	api "github.com/heptio/ark/pkg/apis/ark/v1"
+	"github.com/heptio/ark/pkg/cloudprovider"
 	"github.com/heptio/ark/pkg/generated/clientset/fake"
 	informers "github.com/heptio/ark/pkg/generated/informers/externalversions"
 	. "github.com/heptio/ark/pkg/util/test"
 )
 
 type gcTest struct {
-	name      string
-	bucket    string
-	backups   map[string][]*api.Backup
-	snapshots sets.String
+	name               string
+	bucket             string
+	backups            map[string][]*api.Backup
+	snapshots          sets.String
+	nilSnapshotService bool
 
 	expectedBackupsRemaining   map[string]sets.String
 	expectedSnapshotsRemaining sets.String
@@ -149,11 +151,33 @@ func TestGarbageCollect(t *testing.T) {
 			expectedBackupsRemaining:   make(map[string]sets.String),
 			expectedSnapshotsRemaining: sets.NewString("snapshot-3", "snapshot-4"),
 		},
+		gcTest{
+			name:   "no snapshot service does not error",
+			bucket: "bucket-1",
+			backups: map[string][]*api.Backup{
+				"bucket-1": []*api.Backup{
+					NewTestBackup().WithName("backup-1").
+						WithExpiration(fakeClock.Now().Add(-1*time.Second)).
+						WithSnapshot("pv-1", "snapshot-1").
+						WithSnapshot("pv-2", "snapshot-2").
+						Backup,
+				},
+			},
+			snapshots:                sets.NewString("snapshot-1", "snapshot-2"),
+			nilSnapshotService:       true,
+			expectedBackupsRemaining: make(map[string]sets.String),
+		},
 	}
 
 	for _, test := range tests {
-		backupService := &fakeBackupService{}
-		snapshotService := &FakeSnapshotService{}
+		var (
+			backupService   = &fakeBackupService{}
+			snapshotService *FakeSnapshotService
+		)
+
+		if !test.nilSnapshotService {
+			snapshotService = &FakeSnapshotService{SnapshotsTaken: test.snapshots}
+		}
 
 		t.Run(test.name, func(t *testing.T) {
 			backupService.backupsByBucket = make(map[string][]*api.Backup)
@@ -167,16 +191,19 @@ func TestGarbageCollect(t *testing.T) {
 				backupService.backupsByBucket[bucket] = data
 			}
 
-			snapshotService.SnapshotsTaken = test.snapshots
-
 			var (
 				client          = fake.NewSimpleClientset()
 				sharedInformers = informers.NewSharedInformerFactory(client, 0)
+				snapSvc         cloudprovider.SnapshotService
 			)
 
+			if snapshotService != nil {
+				snapSvc = snapshotService
+			}
+
 			controller := NewGCController(
 				backupService,
-				snapshotService,
+				snapSvc,
 				test.bucket,
 				1*time.Millisecond,
 				sharedInformers.Ark().V1().Backups(),
@@ -202,7 +229,9 @@ func TestGarbageCollect(t *testing.T) {
 				assert.Equal(t, test.expectedBackupsRemaining[bucket], backupNames)
 			}
 
-			assert.Equal(t, test.expectedSnapshotsRemaining, snapshotService.SnapshotsTaken)
+			if !test.nilSnapshotService {
+				assert.Equal(t, test.expectedSnapshotsRemaining, snapshotService.SnapshotsTaken)
+			}
 		})
 	}
 }

pkg/controller/restore_controller.go

@@ -42,11 +42,12 @@ import (
 )
 
 type restoreController struct {
-	restoreClient arkv1client.RestoresGetter
-	backupClient  arkv1client.BackupsGetter
-	restorer      restore.Restorer
-	backupService cloudprovider.BackupService
-	bucket        string
+	restoreClient         arkv1client.RestoresGetter
+	backupClient          arkv1client.BackupsGetter
+	restorer              restore.Restorer
+	backupService         cloudprovider.BackupService
+	bucket                string
+	allowSnapshotRestores bool
 
 	backupLister        listers.BackupLister
 	backupListerSynced  cache.InformerSynced
@@ -64,18 +65,20 @@ func NewRestoreController(
 	backupService cloudprovider.BackupService,
 	bucket string,
 	backupInformer informers.BackupInformer,
+	allowSnapshotRestores bool,
 ) Interface {
 	c := &restoreController{
-		restoreClient:       restoreClient,
-		backupClient:        backupClient,
-		restorer:            restorer,
-		backupService:       backupService,
-		bucket:              bucket,
-		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,
+		allowSnapshotRestores: allowSnapshotRestores,
+		backupLister:          backupInformer.Lister(),
+		backupListerSynced:    backupInformer.Informer().HasSynced,
+		restoreLister:         restoreInformer.Lister(),
+		restoreListerSynced:   restoreInformer.Informer().HasSynced,
+		queue:                 workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "restore"),
 	}
 
 	c.syncHandler = c.processRestore
@@ -275,6 +278,10 @@ 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 {
+		validationErrors = append(validationErrors, "Server is not configured for PV snapshot restores")
+	}
+
 	return validationErrors
 }