Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tag volumes on Create/Attach #130

Merged
merged 4 commits into from
Mar 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/do-csi-plugin/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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)
}
Expand Down
58 changes: 58 additions & 0 deletions driver/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ const (

// createdByDO is used to tag volumes that are created by this CSI plugin
createdByDO = "Created by DigitalOcean CSI driver"

// doAPITimeout sets the timeout we will use when communicating with the
// Digital Ocean API. NOTE: some queries inherit the context timeout
doAPITimeout = 10 * time.Second
)

var (
Expand Down Expand Up @@ -149,6 +153,10 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
SizeGigaBytes: size / GB,
}

if d.doTag != "" {
volumeReq.Tags = append(volumeReq.Tags, d.doTag)
}

ll.Info("checking volume limit")
if err := d.checkLimit(ctx); err != nil {
return nil, err
Expand Down Expand Up @@ -253,6 +261,14 @@ func (d *Driver) ControllerPublishVolume(ctx context.Context, req *csi.Controlle
return nil, err
}

if d.doTag != "" {
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")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that came to my mind yesterday is that we shouldn't tag the volume if no values have been passed to --do-tag. This driver is also used by people who don't use our managed service, so in their case --do-tag will be most probably empty. We should change it therefore to:

if d.doTag != "" {
	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)
if err != nil {
Expand Down Expand Up @@ -963,3 +979,45 @@ func validateCapabilities(caps []*csi.VolumeCapability) bool {

return supported
}

func (d *Driver) tagVolume(parentCtx context.Context, vol *godo.Volume) error {
for _, tag := range vol.Tags {
if tag == d.doTag {
return nil
}
}

tagReq := &godo.TagResourcesRequest{
Resources: []godo.Resource{
godo.Resource{
ID: vol.ID,
Type: godo.VolumeResourceType,
},
},
}

ctx, cancel := context.WithTimeout(parentCtx, doAPITimeout)
defer cancel()
resp, err := d.tags.TagResources(ctx, d.doTag, tagReq)
if resp == nil || resp.StatusCode != http.StatusNotFound {
// either success or irrecoverable failure
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't understand what this code is meant to do. If we create a tag, why do we check for it's existence again?
Also seems like we're not checking resp here and just continue. Finally, we should return from the error check immediately and not continue. I know you're overriding the err here, but I feel like that makes the code open for future bugs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using the TagsService directly (as opposed to as an argument to CreateVolume) the tag must exist before you can assign resources (like a volume) to it. We get a 404 if the tag doesn't exist. In that case we create the tag, and then retry the TagResources which applies the tag to our volume.


// 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(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
}
jcodybaker marked this conversation as resolved.
Show resolved Hide resolved
jcodybaker marked this conversation as resolved.
Show resolved Hide resolved
153 changes: 153 additions & 0 deletions driver/controller_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
package driver

import (
"context"
"errors"
"net/http"
"testing"

"github.com/digitalocean/godo"
)

func TestTagger(t *testing.T) {
tag := "k8s:my-cluster-id"
tcs := []struct {
name string
vol *godo.Volume
createTagFunc func(ctx context.Context, req *godo.TagCreateRequest) (*godo.Tag, *godo.Response, error)
tagResourcesFunc func(context.Context, string, *godo.TagResourcesRequest) (*godo.Response, error)
tagExists bool
expectCreates int
expectTagResources int
expectError bool
expectTags int
}{
{
name: "success existing tag",
vol: &godo.Volume{ID: "hello-world"},
expectTagResources: 1,
tagExists: true,
expectTags: 1,
},
{
name: "success with new tag",
vol: &godo.Volume{ID: "hello-world"},
expectCreates: 1,
expectTagResources: 2,
expectTags: 1,
},
{
name: "success already tagged",
vol: &godo.Volume{
ID: "hello-world",
Tags: []string{tag},
},
expectCreates: 0,
expectTagResources: 0,
},
{
name: "failed first tag",
vol: &godo.Volume{ID: "hello-world"},
expectCreates: 0,
expectTagResources: 1,
expectError: true,
tagResourcesFunc: func(context.Context, string, *godo.TagResourcesRequest) (*godo.Response, error) {
return nil, errors.New("an error")
},
},
{
name: "failed create tag",
vol: &godo.Volume{ID: "hello-world"},
expectCreates: 1,
expectTagResources: 1,
expectError: true,
createTagFunc: func(ctx context.Context, req *godo.TagCreateRequest) (*godo.Tag, *godo.Response, error) {
return nil, nil, errors.New("an error")
},
},
}

for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {

tagService := &fakeTagsDriver{
createFunc: tc.createTagFunc,
tagResourcesFunc: tc.tagResourcesFunc,
exists: tc.tagExists,
}
driver := &Driver{
doTag: tag,
tags: tagService,
}

err := driver.tagVolume(context.Background(), tc.vol)

if err != nil && !tc.expectError {
t.Errorf("expected success but got error %v", err)
} else if tc.expectError && err == nil {
t.Error("expected error but got success")
}

if tagService.createCount != tc.expectCreates {
t.Errorf("createCount was %d, expected %d", tagService.createCount, tc.expectCreates)
}
if tagService.tagResourcesCount != tc.expectTagResources {
t.Errorf("tagResourcesCount was %d, expected %d", tagService.tagResourcesCount, tc.expectTagResources)
}
if tc.expectTags != len(tagService.resources) {
t.Errorf("expected %d tagged volume, %d found", tc.expectTags, len(tagService.resources))
}
})
}
}

type fakeTagsDriver struct {
createFunc func(ctx context.Context, req *godo.TagCreateRequest) (*godo.Tag, *godo.Response, error)
tagResourcesFunc func(context.Context, string, *godo.TagResourcesRequest) (*godo.Response, error)
exists bool
resources []godo.Resource
createCount int
tagResourcesCount int
}

func (*fakeTagsDriver) List(context.Context, *godo.ListOptions) ([]godo.Tag, *godo.Response, error) {
panic("not implemented")
}

func (*fakeTagsDriver) Get(context.Context, string) (*godo.Tag, *godo.Response, error) {
panic("not implemented")
}

func (f *fakeTagsDriver) Create(ctx context.Context, req *godo.TagCreateRequest) (*godo.Tag, *godo.Response, error) {
f.createCount++
if f.createFunc != nil {
return f.createFunc(ctx, req)
}
f.exists = true
return &godo.Tag{
Name: req.Name,
}, godoResponse(), nil
}

func (*fakeTagsDriver) Delete(context.Context, string) (*godo.Response, error) {
panic("not implemented")
}

func (f *fakeTagsDriver) TagResources(ctx context.Context, tag string, req *godo.TagResourcesRequest) (*godo.Response, error) {
f.tagResourcesCount++
if f.tagResourcesFunc != nil {
return f.tagResourcesFunc(ctx, tag, req)
}
if !f.exists {
return &godo.Response{
Response: &http.Response{StatusCode: 404},
Rate: godo.Rate{Limit: 10, Remaining: 10},
}, errors.New("An error occured")
}
f.resources = append(f.resources, req.Resources...)
return godoResponse(), nil
}

func (*fakeTagsDriver) UntagResources(context.Context, string, *godo.UntagResourcesRequest) (*godo.Response, error) {
panic("not implemented")
}
6 changes: 5 additions & 1 deletion driver/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type Driver struct {
endpoint string
nodeId string
region string
doTag string

srv *grpc.Server
log *logrus.Entry
Expand All @@ -67,6 +68,7 @@ type Driver struct {
droplets godo.DropletsService
snapshots godo.SnapshotsService
account godo.AccountService
tags godo.TagsService

// ready defines whether the driver is ready to function. This value will
// be used by the `Identity` service via the `Probe()` method.
Expand All @@ -77,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,
})
Expand Down Expand Up @@ -106,6 +108,7 @@ func NewDriver(ep, token, url string) (*Driver, error) {
})

return &Driver{
doTag: doTag,
endpoint: ep,
nodeId: nodeId,
region: region,
Expand All @@ -117,6 +120,7 @@ func NewDriver(ep, token, url string) (*Driver, error) {
droplets: doClient.Droplets,
snapshots: doClient.Snapshots,
account: doClient.Account,
tags: doClient.Tags,
}, nil
}

Expand Down
3 changes: 3 additions & 0 deletions driver/driver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ func TestDriverSuite(t *testing.T) {
}

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

Expand Down
4 changes: 1 addition & 3 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ require (
github.com/container-storage-interface/spec v1.0.0
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/digitalocean/go-metadata v0.0.0-20180111002115-15bd36e5f6f7
github.com/digitalocean/godo v1.4.2
github.com/digitalocean/godo v1.8.0
github.com/ghodss/yaml v1.0.0 // indirect
github.com/gogo/protobuf v1.1.1 // indirect
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b // indirect
Expand All @@ -29,7 +29,6 @@ require (
github.com/sirupsen/logrus v1.0.5
github.com/spf13/pflag v1.0.2 // indirect
github.com/stretchr/testify v1.2.2 // indirect
github.com/tent/http-link-go v0.0.0-20130702225549-ac974c61c2f9 // indirect
golang.org/x/crypto v0.0.0-20180411161317-d6449816ce06 // indirect
golang.org/x/net v0.0.0-20180406214816-61147c48b25b // indirect
golang.org/x/oauth2 v0.0.0-20180402223937-921ae394b943
Expand All @@ -48,6 +47,5 @@ require (
k8s.io/apimachinery v0.0.0-20181116115711-1b0702fe2927
k8s.io/client-go v9.0.0+incompatible
k8s.io/klog v0.1.0 // indirect
launchpad.net/gocheck v0.0.0-20140225173054-000000000087 // indirect
sigs.k8s.io/yaml v1.1.0 // indirect
)
8 changes: 2 additions & 6 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c
github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/digitalocean/go-metadata v0.0.0-20180111002115-15bd36e5f6f7 h1:ESwakcvVdp1uaY19Bt8e3OHLqpqh1hS6tDRgAIa8m78=
github.com/digitalocean/go-metadata v0.0.0-20180111002115-15bd36e5f6f7/go.mod h1:lNrzMwI4fx6xfzieyLEpYIJPLWjT/Sak4G/hIzGTEL4=
github.com/digitalocean/godo v1.4.2 h1:AOsEb4Sa2OpfIAZluAvA5LGp61uq53tCNs+BZte1I28=
github.com/digitalocean/godo v1.4.2/go.mod h1:h6faOIcZ8lWIwNQ+DN7b3CgX4Kwby5T+nbpNqkUIozU=
github.com/digitalocean/godo v1.8.0 h1:UnosidNT03zuOgObn7zA3DiObx0QbmLlfcpswpLqT3c=
github.com/digitalocean/godo v1.8.0/go.mod h1:h6faOIcZ8lWIwNQ+DN7b3CgX4Kwby5T+nbpNqkUIozU=
github.com/ghodss/yaml v1.0.0 h1:wQHKEahhL6wmXdzwWG11gIVCkOv05bNOh+Rxn0yngAk=
github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04=
github.com/gogo/protobuf v1.1.1 h1:72R+M5VuhED/KujmZVcIquuo8mBgX4oVda//DQb3PXo=
Expand Down Expand Up @@ -54,8 +54,6 @@ github.com/spf13/pflag v1.0.2 h1:Fy0orTDgHdbnzHcsOgfCN4LtHf0ec3wwtiwJqwvf3Gc=
github.com/spf13/pflag v1.0.2/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4=
github.com/stretchr/testify v1.2.2 h1:bSDNvY7ZPG5RlJ8otE/7V6gMiyenm9RtJ7IUVIAoJ1w=
github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
github.com/tent/http-link-go v0.0.0-20130702225549-ac974c61c2f9 h1:/Bsw4C+DEdqPjt8vAqaC9LAqpAQnaCQQqmolqq3S1T4=
github.com/tent/http-link-go v0.0.0-20130702225549-ac974c61c2f9/go.mod h1:RHkNRtSLfOK7qBTHaeSX1D6BNpI3qw7NTxsmNr4RvN8=
golang.org/x/crypto v0.0.0-20180411161317-d6449816ce06 h1:EOqG0JqGlLr+punVB69jvWCv/ErZKGlC7PMdyHfv+Bc=
golang.org/x/crypto v0.0.0-20180411161317-d6449816ce06/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
golang.org/x/net v0.0.0-20180406214816-61147c48b25b h1:7rskAFQwNXGW6AD8E/6y0LDHW5mT9rsLD7ViLVFfh5w=
Expand Down Expand Up @@ -94,7 +92,5 @@ k8s.io/client-go v9.0.0+incompatible h1:2kqW3X2xQ9SbFvWZjGEHBLlWc1LG9JIJNXWkuqwd
k8s.io/client-go v9.0.0+incompatible/go.mod h1:7vJpHMYJwNQCWgzmNV+VYUl1zCObLyodBc8nIyt8L5s=
k8s.io/klog v0.1.0 h1:I5HMfc/DtuVaGR1KPwUrTc476K8NCqNBldC7H4dYEzk=
k8s.io/klog v0.1.0/go.mod h1:Gq+BEi5rUBO/HRz0bTSXDUcqjScdoY3a9IHpCEIOOfk=
launchpad.net/gocheck v0.0.0-20140225173054-000000000087 h1:Izowp2XBH6Ya6rv+hqbceQyw/gSGoXfH/UPoTGduL54=
launchpad.net/gocheck v0.0.0-20140225173054-000000000087/go.mod h1:hj7XX3B/0A+80Vse0e+BUHsHMTEhd0O4cpUHr/e/BUM=
sigs.k8s.io/yaml v1.1.0 h1:4A07+ZFc2wgJwo8YNlQpr1rVlgUDlxXHhPJciaPY5gs=
sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=
Loading