From dbcb92c024fa71c71e74c0b40c70a12b3848350c Mon Sep 17 00:00:00 2001
From: Timo Reimann <treimann@digitalocean.com>
Date: Thu, 12 Dec 2019 11:47:28 +0100
Subject: [PATCH] Return error when fetching the snapshot fails (#233)

We previously continued processing when fetching the snapshot failed for
errors other than 404.
---
 CHANGELOG.md              |  2 ++
 driver/controller.go      |  1 +
 driver/controller_test.go | 68 +++++++++++++++++++++++++++++++++++++++
 driver/driver_test.go     | 16 +++++++--
 4 files changed, 84 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 710711cd..4b02caa0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 * Use WARN log level for non-critical failures to get an action
   [[GH-241]](https://github.com/digitalocean/csi-digitalocean/pull/241)
+* Return error when fetching the snapshot fails
+  [[GH-233]](https://github.com/digitalocean/csi-digitalocean/pull/233)
 * Reject requests for block access type
   [[GH-225]](https://github.com/digitalocean/csi-digitalocean/pull/225)
 * Assume detached state on 404 during ControllerUnpublishVolume
diff --git a/driver/controller.go b/driver/controller.go
index c4566cd7..65b1f50d 100644
--- a/driver/controller.go
+++ b/driver/controller.go
@@ -169,6 +169,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 			if resp != nil && resp.StatusCode == http.StatusNotFound {
 				return nil, status.Errorf(codes.NotFound, "snapshot %q not found", snapshotID)
 			}
+			return nil, err
 		}
 
 		ll.WithField("snapshot_id", snapshotID).Info("using snapshot as volume source")
diff --git a/driver/controller_test.go b/driver/controller_test.go
index 906bcd64..9c0e3851 100644
--- a/driver/controller_test.go
+++ b/driver/controller_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"net/http"
+	"strings"
 	"testing"
 	"time"
 
@@ -255,6 +256,73 @@ func TestControllerExpandVolume(t *testing.T) {
 	}
 }
 
+func TestCreateVolume(t *testing.T) {
+	tests := []struct {
+		name           string
+		listVolumesErr error
+		getSnapshotErr error
+	}{
+		{
+			name:           "listing volumes failing",
+			listVolumesErr: errors.New("failed to list volumes"),
+		},
+		{
+			name:           "fetching snapshot failing",
+			getSnapshotErr: errors.New("failed to get snapshot"),
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			d := &Driver{
+				storage: &fakeStorageDriver{
+					listVolumesErr: test.listVolumesErr,
+				},
+				snapshots: &fakeSnapshotsDriver{
+					getSnapshotErr: test.getSnapshotErr,
+				},
+				log: logrus.New().WithField("test_enabed", true),
+			}
+
+			_, err := d.CreateVolume(context.Background(), &csi.CreateVolumeRequest{
+				Name: "name",
+				VolumeCapabilities: []*csi.VolumeCapability{
+					&csi.VolumeCapability{
+						AccessType: &csi.VolumeCapability_Mount{
+							Mount: &csi.VolumeCapability_MountVolume{},
+						},
+						AccessMode: &csi.VolumeCapability_AccessMode{
+							Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
+						},
+					},
+				},
+				VolumeContentSource: &csi.VolumeContentSource{
+					Type: &csi.VolumeContentSource_Snapshot{
+						Snapshot: &csi.VolumeContentSource_SnapshotSource{
+							SnapshotId: "snapshotId",
+						},
+					},
+				},
+			})
+
+			var wantErr error
+			switch {
+			case test.listVolumesErr != nil:
+				wantErr = test.listVolumesErr
+			case test.getSnapshotErr != nil:
+				wantErr = test.getSnapshotErr
+			}
+
+			if wantErr == nil && err != nil {
+				t.Errorf("got error %q, want none", err)
+			}
+			if wantErr != nil && !strings.Contains(err.Error(), wantErr.Error()) {
+				t.Errorf("want error %q to include %q", err, wantErr)
+			}
+		})
+	}
+}
+
 type fakeStorageAction struct {
 	*fakeStorageActionsDriver
 	storageGetValsFunc func(invocation int) (*godo.Action, *godo.Response, error)
diff --git a/driver/driver_test.go b/driver/driver_test.go
index bae41cd2..4255afd2 100644
--- a/driver/driver_test.go
+++ b/driver/driver_test.go
@@ -101,11 +101,16 @@ func (f *fakeAccountDriver) Get(context.Context) (*godo.Account, *godo.Response,
 }
 
 type fakeStorageDriver struct {
-	volumes   map[string]*godo.Volume
-	snapshots map[string]*godo.Snapshot
+	volumes        map[string]*godo.Volume
+	snapshots      map[string]*godo.Snapshot
+	listVolumesErr error
 }
 
 func (f *fakeStorageDriver) ListVolumes(ctx context.Context, param *godo.ListVolumeParams) ([]godo.Volume, *godo.Response, error) {
+	if f.listVolumesErr != nil {
+		return nil, nil, f.listVolumesErr
+	}
+
 	var volumes []godo.Volume
 
 	for _, vol := range f.volumes {
@@ -313,7 +318,8 @@ func (f *fakeDropletsDriver) Neighbors(context.Context, int) ([]godo.Droplet, *g
 }
 
 type fakeSnapshotsDriver struct {
-	snapshots map[string]*godo.Snapshot
+	snapshots      map[string]*godo.Snapshot
+	getSnapshotErr error
 }
 
 func (f *fakeSnapshotsDriver) List(context.Context, *godo.ListOptions) ([]godo.Snapshot, *godo.Response, error) {
@@ -334,6 +340,10 @@ func (f *fakeSnapshotsDriver) ListDroplet(context.Context, *godo.ListOptions) ([
 }
 
 func (f *fakeSnapshotsDriver) Get(ctx context.Context, id string) (*godo.Snapshot, *godo.Response, error) {
+	if f.getSnapshotErr != nil {
+		return nil, nil, f.getSnapshotErr
+	}
+
 	resp := godoResponse()
 	snap, ok := f.snapshots[id]
 	if !ok {