-
Notifications
You must be signed in to change notification settings - Fork 108
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ( | ||
|
@@ -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 | ||
|
@@ -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") | ||
} | ||
} | ||
|
||
// check if droplet exist before trying to attach the volume to the droplet | ||
_, resp, err = d.droplets.Get(ctx, dropletID) | ||
if err != nil { | ||
|
@@ -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 | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When using the TagsService directly (as opposed to as an argument to |
||
|
||
// 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
|
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") | ||
} |
There was a problem hiding this comment.
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: