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.

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

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

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 all 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
17 changes: 5 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,13 @@ 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. 👍

if cvResp != nil && cvResp.StatusCode == http.StatusForbidden && strings.Contains(err.Error(), "capacity limit exceeded") {
return nil, status.Errorf(codes.ResourceExhausted, "volume limit has been reached. Please contact support")
}

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

Expand Down
72 changes: 66 additions & 6 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
snapshots map[string]*godo.Snapshot
wantErr error
createVolumeErr error
createVolumeResponseErr *godo.Response
}{
{
name: "listing volumes failing",
Expand All @@ -297,24 +304,75 @@ func TestCreateVolume(t *testing.T) {
name: "fetching snapshot failing",
getSnapshotErr: errors.New("failed to get snapshot"),
},
{
name: "volume limit has been reached",
snapshots: map[string]*godo.Snapshot{
snapshotId: {
ID: snapshotId,
},
},
createVolumeErr: &godo.ErrorResponse{
Response: &http.Response{
Request: &http.Request{
Method: http.MethodPost,
URL: &url.URL{},
},
StatusCode: http.StatusForbidden,
},
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",
snapshots: 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{
Response: &http.Response{
StatusCode: http.StatusInternalServerError,
},
},
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.snapshots,
},
log: logrus.New().WithField("test_enabed", true),
log: logrus.New().WithField("test_enabled", 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 +396,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