CON-9080 Remove checkLimit validation in favor of forbidden response when creating a volume #481
Conversation
…ponse when creating a volume
| 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 { |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Ah, wasn't aware it's not exported. I think your approach makes sense then. 👍
driver/controller.go
Outdated
| if err != nil { | ||
| var errorResponse *godo.ErrorResponse | ||
| if errors.As(err, &errorResponse) && strings.Contains(err.(*godo.ErrorResponse).Message, "capacity limit exceeded") && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Won't do based on comment below.
driver/controller.go
Outdated
| if err != nil { | ||
| var errorResponse *godo.ErrorResponse | ||
| if errors.As(err, &errorResponse) && strings.Contains(err.(*godo.ErrorResponse).Message, "capacity limit exceeded") && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
True, I refactored the code to check the substring from the basic err.Error().
driver/controller.go
Outdated
| if err != nil { | ||
| var errorResponse *godo.ErrorResponse | ||
| if errors.As(err, &errorResponse) && strings.Contains(err.(*godo.ErrorResponse).Message, "capacity limit exceeded") && | ||
| cvResp.StatusCode == http.StatusForbidden { |
There was a problem hiding this comment.
We can add some extra safety by also checking that cvResp is not nil.
| @@ -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 | |||
There was a problem hiding this comment.
Should we return as soon as one of f.createVolumeErr or f.createVolumeErrResponse is non-nil (as opposed to requiring both)?
There was a problem hiding this comment.
Sure! It gives more flexibility in the tests. Added.
driver/controller.go
Outdated
| if err != nil { | ||
| if cvResp != nil && cvResp.Response != nil && cvResp.StatusCode == http.StatusForbidden && strings.Contains(err.Error(), "capacity limit exceeded") { |
There was a problem hiding this comment.
Small nit / simplification suggestion: I think it's common to check the outer *godo.Response for nil-ness only, not the inner *http.Response since godo always populates them together.
There was a problem hiding this comment.
Sure, I've removed the cvResp.Response != nil check and only kept cvResp != nil.
driver/controller_test.go
Outdated
| name string | ||
| listVolumesErr error | ||
| getSnapshotErr error | ||
| snapchots map[string]*godo.Snapshot |
There was a problem hiding this comment.
typo: snapchots -> snapshots
There was a problem hiding this comment.
Nice catch! Fixed.
llDrLove
deleted the
CON-9080-do-not-check-volumes-limit-in-csi-driver-for-limits-v3
branch
Given that the volume limit is to be generalized and raised to 5,000 (which should be more than enough for any [DOKS] customer), showing the actual limit does not provide any value anymore. Thus, we favor handling the http forbidden response from godo for a Forbidden http code (403) with some specific response string than to use the
checkLimitmethod.