address PR comments

- rework tagVolumes for clarity and to fix accidental context reuse in after cancel
- use flag rather than environmental var
- rename doVolTag to doTag in anticipation of upcoming snapshot tagging
- check errors on tagVolumes

cmd/do-csi-plugin/main.go

@@ -30,6 +30,7 @@ func main() {
 		endpoint = flag.String("endpoint", "unix:///var/lib/kubelet/plugins/"+driver.DriverName+"/csi.sock", "CSI endpoint")
 		token    = flag.String("token", "", "DigitalOcean access token")
 		url      = flag.String("url", "https://api.digitalocean.com/", "DigitalOcean API URL")
+		doTag    = flag.String("do-tag", "", "Tag DigitalOcean volumes on Create/Attach")
 		version  = flag.Bool("version", false, "Print the version and exit.")
 	)
 	flag.Parse()
@@ -39,7 +40,7 @@ func main() {
 		os.Exit(0)
 	}
 
-	drv, err := driver.NewDriver(*endpoint, *token, *url)
+	drv, err := driver.NewDriver(*endpoint, *token, *url, *doTag)
 	if err != nil {
 		log.Fatalln(err)
 	}

driver/controller.go

@@ -153,8 +153,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		SizeGigaBytes: size / GB,
 	}
 
-	if d.doVolTag != "" {
-		volumeReq.Tags = append(volumeReq.Tags, d.doVolTag)
+	if d.doTag != "" {
+		volumeReq.Tags = append(volumeReq.Tags, d.doTag)
 	}
 
 	ll.Info("checking volume limit")
@@ -261,7 +261,11 @@ func (d *Driver) ControllerPublishVolume(ctx context.Context, req *csi.Controlle
 		return nil, err
 	}
 
-	d.tagVolume(ctx, vol)
+	err = d.tagVolume(ctx, vol)
+	if err != nil {
+		ll.Errorf("error tagging volume: %s", err)
+		return nil, status.Errorf(codes.Internal, "failed to tag volume")
+	}
 
 	// check if droplet exist before trying to attach the volume to the droplet
 	_, resp, err = d.droplets.Get(ctx, dropletID)
@@ -974,9 +978,9 @@ func validateCapabilities(caps []*csi.VolumeCapability) bool {
 	return supported
 }
 
-func (d *Driver) tagVolume(ctx context.Context, vol *godo.Volume) error {
+func (d *Driver) tagVolume(parentCtx context.Context, vol *godo.Volume) error {
 	for _, tag := range vol.Tags {
-		if tag == d.doVolTag {
+		if tag == d.doTag {
 			return nil
 		}
 	}
@@ -990,29 +994,29 @@ func (d *Driver) tagVolume(ctx context.Context, vol *godo.Volume) error {
 		},
 	}
 
-	ctx, cancel := context.WithTimeout(ctx, doAPITimeout)
-	resp, err := d.tags.TagResources(ctx, d.doVolTag, tagReq)
-	cancel()
+	ctx, cancel := context.WithTimeout(parentCtx, doAPITimeout)
+	defer cancel()
+	resp, err := d.tags.TagResources(ctx, d.doTag, tagReq)
 
-	if resp != nil && resp.StatusCode == http.StatusNotFound {
-		// If the tag doesn't exist, create it and retry.
-		err = d.createTag(ctx)
-		if err != nil {
-			return err
-		}
-		ctx, cancel = context.WithTimeout(ctx, doAPITimeout)
-		resp, err = d.tags.TagResources(ctx, d.doVolTag, tagReq)
-		cancel()
+	if err == nil || (resp != nil && resp.StatusCode != http.StatusNotFound) {
+		// either success or irrecoverable failure
+		return err
 	}
 
-	return err
-}
-
-func (d *Driver) createTag(ctx context.Context) error {
-	ctx, cancel := context.WithTimeout(ctx, doAPITimeout)
+	// godo.TagsService returns 404 if the tag has not yet been
+	// created, if that happens we need to create the tag
+	// and then retry tagging the volume resource.
+	ctx, cancel = context.WithTimeout(parentCtx, doAPITimeout)
 	defer cancel()
-	_, _, err := d.tags.Create(ctx, &godo.TagCreateRequest{
-		Name: d.doVolTag,
+	_, _, err = d.tags.Create(parentCtx, &godo.TagCreateRequest{
+		Name: d.doTag,
 	})
+	if err != nil {
+		return err
+	}
+
+	ctx, cancel = context.WithTimeout(parentCtx, doAPITimeout)
+	defer cancel()
+	_, err = d.tags.TagResources(ctx, d.doTag, tagReq)
 	return err
 }

driver/controller_test.go

@@ -76,7 +76,7 @@ func TestTagger(t *testing.T) {
 				exists:           tc.tagExists,
 			}
 			driver := &Driver{
-				doVolTag: tag,
+				doTag: tag,
 				tags:     tagService,
 			}
 

driver/driver.go

@@ -42,10 +42,6 @@ const (
 )
 
 var (
-	// doVolTagEnv describes an environmental var. If present, the controller
-	// will apply the specified Digital Ocean tag to volumes during create/attach.
-	doVolTagEnv = "DIGITALOCEAN_TAG_VOLUMES"
-
 	gitTreeState = "not a git tree"
 	commit       string
 	version      string
@@ -61,7 +57,7 @@ type Driver struct {
 	endpoint string
 	nodeId   string
 	region   string
-	doVolTag string
+	doTag    string
 
 	srv     *grpc.Server
 	log     *logrus.Entry
@@ -83,7 +79,7 @@ type Driver struct {
 // NewDriver returns a CSI plugin that contains the necessary gRPC
 // interfaces to interact with Kubernetes over unix domain sockets for
 // managaing DigitalOcean Block Storage
-func NewDriver(ep, token, url string) (*Driver, error) {
+func NewDriver(ep, token, url, doTag string) (*Driver, error) {
 	tokenSource := oauth2.StaticTokenSource(&oauth2.Token{
 		AccessToken: token,
 	})
@@ -112,7 +108,7 @@ func NewDriver(ep, token, url string) (*Driver, error) {
 	})
 
 	return &Driver{
-		doVolTag: os.Getenv(doVolTagEnv),
+		doTag:    doTag,
 		endpoint: ep,
 		nodeId:   nodeId,
 		region:   region,

driver/driver_test.go

@@ -59,7 +59,7 @@ func TestDriverSuite(t *testing.T) {
 	}
 
 	nodeID := 987654
-	doVolTag := "k8s:cluster-id"
+	doTag := "k8s:cluster-id"
 	volumes := make(map[string]*godo.Volume, 0)
 	snapshots := make(map[string]*godo.Snapshot, 0)
 	droplets := map[int]*godo.Droplet{
@@ -71,7 +71,7 @@ func TestDriverSuite(t *testing.T) {
 	driver := &Driver{
 		endpoint: endpoint,
 		nodeId:   strconv.Itoa(nodeID),
-		doVolTag: doVolTag,
+		doTag: doTag,
 		region:   "nyc3",
 		mounter:  &fakeMounter{},
 		log:      logrus.New().WithField("test_enabed", true),