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

Fix: handle max_request_size<=0 #10072

Merged
merged 5 commits into from Jan 19, 2021

Conversation

guacamole
Copy link
Contributor

Signed-off-by: guacamole gunjanwalecha@gmail.com

This PR tries to fix the issue #9936
Setting max_request_size to 0 or a negative number should disable request size limiting

Motivation

As per the documentation: max_request_size is the maximum allowed request size, in bytes. Defaults to 32 MB. Specifying a number less than or equal to 0 should turn off limiting altogether.

Implementation details

Old implementation:

  • when max_request_size is 0, it gets set to a default size of 32 MB
  • when max_request_size is negative, it throws an error saying: " max_request_size cannot be negative"

New implementation:

  • when max_request_size set to less than or equal to 0, the limiting is disabled

@hashicorp-cla
Copy link

hashicorp-cla commented Oct 1, 2020

CLA assistant check
All committers have signed the CLA.

@guacamole
Copy link
Contributor Author

@raskchanky Would you please have a look at this ?

@guacamole guacamole marked this pull request as ready for review October 1, 2020 18:12
Copy link
Contributor

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

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

One issue with the current approach is that 0 is Go's default value for integers, so if no max request size is specified, it will be 0, in which case the default should be used. Otherwise, it's kinda pointless to have a default value 😁.

In my opinion, the right fix here is to:

  • remove the error saying max request size can't be negative
  • if max request size is negative, then disable it
  • update the docs to reflect this change

Also, in addition to fixing the existing CI failure, it'd be great to have some additional tests showing that:

  • the default max request size is still in place if it's not overridden
  • it's properly disabled when set to a negative value
  • the config is parsed correctly with negative values in it

Thanks so much for working on this!

@guacamole
Copy link
Contributor Author

@raskchanky Thanks for the input, I'm working on it!

@HridoyRoy
Copy link
Contributor

Hi @guacamole ,
It's been a while since we've heard from you. Wondering if you were still working on this PR?

Thanks so much!

@guacamole
Copy link
Contributor Author

Hello @HridoyRoy
sorry for the long time, I'll get to it today and have an update for you soon:)

Signed-off-by: guacamole <gunjanwalecha@gmail.com>
Signed-off-by: guacamole <gunjanwalecha@gmail.com>
Signed-off-by: guacamole <gunjanwalecha@gmail.com>
@guacamole guacamole force-pushed the guacamole/max-request-size-fix branch from c5cf9d9 to 3d98b24 Compare January 16, 2021 08:40
@vercel
Copy link

vercel bot commented Jan 16, 2021

@guacamole is attempting to deploy a commit to the HashiCorp Team on Vercel.

A member of the Team first needs to authorize it.

@guacamole
Copy link
Contributor Author

Hi @raskchanky , I just rebased my PR from master and test-go is failing on circle-ci . Would you please check the failing test cases as the http module did not fail on my local.

Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

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

@guacamole lgtm!

@HridoyRoy
Copy link
Contributor

I've created a docs PR to go with this change here

@HridoyRoy HridoyRoy added this to the 1.7 milestone Jan 19, 2021
@HridoyRoy HridoyRoy merged commit 86b29be into hashicorp:master Jan 19, 2021
jartek pushed a commit to jartek/vault that referenced this pull request Sep 11, 2021
* Fix: handle max_request_size<=0

Signed-off-by: guacamole <gunjanwalecha@gmail.com>

* created test cases for listener

Signed-off-by: guacamole <gunjanwalecha@gmail.com>

* added test case for negative value of MaxRequestSize

Signed-off-by: guacamole <gunjanwalecha@gmail.com>

Co-authored-by: Hridoy Roy <roy@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants