Skip to content

CON-9080 Remove checkLimit validation in favor of forbidden response when creating a volume #481

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Mar 17, 2023
Merged

CON-9080 Remove checkLimit validation in favor of forbidden response when creating a volume #481

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bb280e9
CON-9080 Removed the checklimit validation to favor the forbidden res…
llDrLove Mar 14, 2023
90f1972
CON-9080 handle limit if we have a ErrorResponse
llDrLove Mar 14, 2023
cb4f9ec
CON-9080 refactor conditions
llDrLove Mar 14, 2023
7612e01
CON-9080 refactor error handling
llDrLove Mar 14, 2023
0dbbc1e
CON-9080 add unit tests
llDrLove Mar 15, 2023
910a432
CON-9080 Add release note and use error.As instead of casting error type
llDrLove Mar 15, 2023
1aa54c8
CON-9080 fix unit tests with new limit if statement implementation
llDrLove Mar 15, 2023
68afc7f
CON-9080 fix typo error and don't check inner response struct for nil
llDrLove Mar 17, 2023
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
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
## unreleased

* Validate CreateVolume limits with forbidden error response
[[GH-481]](https://github.com/digitalocean/csi-digitalocean/pull/481)
* CSI driver can accidentally format existing volume
[[GH-469]](https://github.com/digitalocean/csi-digitalocean/pull/478)
[[GH-478]](https://github.com/digitalocean/csi-digitalocean/pull/478)

## v4.5.0 - 2023.01.11
* Update CSI driver for Kubernetes 1.26
Expand Down
19 changes: 7 additions & 12 deletions driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,20 +181,15 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
volumeReq.SnapshotID = snapshotID
}

log.Info("checking volume limit")
details, err := d.checkLimit(ctx)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to check volume limit: %s", err)
}
if details != nil {
return nil, status.Errorf(codes.ResourceExhausted,
"volume limit (%d) has been reached. Current number of volumes: %d. Please contact support.",
details.limit, details.numVolumes)
}

log.WithField("volume_req", volumeReq).Info("creating volume")
vol, _, err := d.storage.CreateVolume(ctx, volumeReq)
vol, cvResp, err := d.storage.CreateVolume(ctx, volumeReq)
if err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timoreimann ,

Here we have a payload ressembling this :

{
  "message": "failed to create volume: volume/snapshot capacity limit exceeded",
  "id":"forbidden",
  "request_id": "7c230c90-c7a0-4e79-a40d-b8e3ca003bd4"
}

I'm not looking at the id field of the CreateVolume response for the exact match of forbidden since it is not exported by godo. Instead, I'm relying on the http code of the response which is kind of the same check IMO. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, wasn't aware it's not exported. I think your approach makes sense then. 👍

var errorResponse *godo.ErrorResponse
if errors.As(err, &errorResponse) && strings.Contains(err.(*godo.ErrorResponse).Message, "capacity limit exceeded") &&
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use errors.As , then we should also inspect Message on errorResponse instead of doing another type cast.

Note though how this remark is obsolete now given my next comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't do based on comment below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that we do not need to check for and cast into an ErrorResponse now that we have discovered that the error ID isn't exported and that we can only check for a portion of the error message: the Error() implementation always includes it, which means it suffices to check err.Error() for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I refactored the code to check the substring from the basic err.Error().

cvResp.StatusCode == http.StatusForbidden {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can add some extra safety by also checking that cvResp is not nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

return nil, status.Errorf(codes.ResourceExhausted, "volume limit has been reached. Please contact support")
}

return nil, status.Error(codes.Internal, err.Error())
}

Expand Down
59 changes: 54 additions & 5 deletions driver/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"
"net/http"
"net/url"
"strconv"
"strings"
"testing"
Expand Down Expand Up @@ -284,10 +285,16 @@ func TestControllerExpandVolume(t *testing.T) {
}

func TestCreateVolume(t *testing.T) {
snapshotId := "snapshotId"

tests := []struct {
name string
listVolumesErr error
getSnapshotErr error
name string
listVolumesErr error
getSnapshotErr error
snapchots map[string]*godo.Snapshot
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: snapchots -> snapshots

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Fixed.

wantErr error
createVolumeErr error
createVolumeResponseErr *godo.Response
}{
{
name: "listing volumes failing",
Expand All @@ -297,24 +304,64 @@ func TestCreateVolume(t *testing.T) {
name: "fetching snapshot failing",
getSnapshotErr: errors.New("failed to get snapshot"),
},
{
name: "volume limit has been reached",
snapchots: map[string]*godo.Snapshot{
snapshotId: {
ID: snapshotId,
},
},
createVolumeErr: &godo.ErrorResponse{
Message: "failed to create volume: volume/snapshot capacity limit exceeded",
},
createVolumeResponseErr: &godo.Response{
Response: &http.Response{
StatusCode: http.StatusForbidden,
},
},
wantErr: errors.New("volume limit has been reached. Please contact support"),
},
{
name: "error occurred when creating a volume",
snapchots: map[string]*godo.Snapshot{
snapshotId: {
ID: snapshotId,
},
},
createVolumeErr: &godo.ErrorResponse{
Response: &http.Response{
Request: &http.Request{
Method: http.MethodPost,
URL: &url.URL{},
},
StatusCode: http.StatusInternalServerError,
},
Message: "internal server error",
},
createVolumeResponseErr: &godo.Response{},
wantErr: errors.New("internal server error"),
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
d := &Driver{
storage: &fakeStorageDriver{
listVolumesErr: test.listVolumesErr,
listVolumesErr: test.listVolumesErr,
createVolumeErr: test.createVolumeErr,
createVolumeErrResponse: test.createVolumeResponseErr,
},
snapshots: &fakeSnapshotsDriver{
getSnapshotErr: test.getSnapshotErr,
snapshots: test.snapchots,
},
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{},
},
Expand All @@ -338,6 +385,8 @@ func TestCreateVolume(t *testing.T) {
wantErr = test.listVolumesErr
case test.getSnapshotErr != nil:
wantErr = test.getSnapshotErr
case test.createVolumeErr != nil:
wantErr = test.wantErr
}

if wantErr == nil && err != nil {
Expand Down
12 changes: 9 additions & 3 deletions driver/driver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,11 @@ func (f *fakeAccountDriver) Get(context.Context) (*godo.Account, *godo.Response,
}

type fakeStorageDriver struct {
volumes map[string]*godo.Volume
snapshots map[string]*godo.Snapshot
listVolumesErr error
volumes map[string]*godo.Volume
snapshots map[string]*godo.Snapshot
listVolumesErr error
createVolumeErr error
createVolumeErrResponse *godo.Response
}

func (f *fakeStorageDriver) ListVolumes(ctx context.Context, param *godo.ListVolumeParams) ([]godo.Volume, *godo.Response, error) {
Expand Down Expand Up @@ -213,6 +215,10 @@ func (f *fakeStorageDriver) GetVolume(ctx context.Context, id string) (*godo.Vol
}

func (f *fakeStorageDriver) CreateVolume(ctx context.Context, req *godo.VolumeCreateRequest) (*godo.Volume, *godo.Response, error) {
if f.createVolumeErr != nil && f.createVolumeErrResponse != nil {
return nil, f.createVolumeErrResponse, f.createVolumeErr
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return as soon as one of f.createVolumeErr or f.createVolumeErrResponse is non-nil (as opposed to requiring both)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! It gives more flexibility in the tests. Added.

}

id := randString(10)
vol := &godo.Volume{
ID: id,
Expand Down