-
Notifications
You must be signed in to change notification settings - Fork 171
[THREESCALE-10164] Add support to set large_client_header_buffers directive #1446
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
[THREESCALE-10164] Add support to set large_client_header_buffers directive #1446
Conversation
59ae70b to
9e6c973
Compare
eguzki
left a comment
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.
Looking good.
|
I kindly requested review from @3scale/documentation team. For some reason the test keeps failing in this PR 😞 , hopefully after I address the feedback from the docs team and make a new commit the test will work |
CHANGELOG.md
Outdated
|
|
||
| - Dev environment: Camel proxy [PR #1441](https://github.com/3scale/APIcast/pull/1441) | ||
|
|
||
| - Added `APICAST_CLIENT_REQUEST_HEADER_BUFFERS` variable that allows to configure nginx `client_request_header_buffers` directive [PR #1446](https://github.com/3scale/APIcast/pull/1446), [THREESCALE-10164](https://issues.redhat.com/browse/THREESCALE-10164) |
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.
| - Added `APICAST_CLIENT_REQUEST_HEADER_BUFFERS` variable that allows to configure nginx `client_request_header_buffers` directive [PR #1446](https://github.com/3scale/APIcast/pull/1446), [THREESCALE-10164](https://issues.redhat.com/browse/THREESCALE-10164) | |
| - Added `APICAST_CLIENT_REQUEST_HEADER_BUFFERS` variable to allow configuration of the NGINX `client_request_header_buffers` directive: [PR #1446](https://github.com/3scale/APIcast/pull/1446), [THREESCALE-10164](https://issues.redhat.com/browse/THREESCALE-10164) |
dfennessy
left a comment
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.
I've added a suggestion.
9e6c973 to
5f752bf
Compare
eguzki
left a comment
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.
Verification steps working like a charm
Some nitpicking and ready to merge.
t/large-client-header-buffers.t
Outdated
|
|
||
| __DATA__ | ||
| === TEST 1: large header (the header exceed exceed the size of one buffer) |
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.
exceed exceed 👀
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.
good catch
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.
fixed
t/large-client-header-buffers.t
Outdated
| } | ||
| --- upstream | ||
| client_header_buffer_size 10; |
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.
this is needed? why?
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.
This is not needed. I was trying to force nginx to use a large client header buffer, but I realized it's easier to just send a large header.
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.
fixed
eguzki
left a comment
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.
LGTM but there are new conflicts (in CHANGELOG)
3cda591 to
30cec6c
Compare
|
Conflict resolved |
|
@dfennessy your approval is needed as you requested changes |
dfennessy
left a comment
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.
/lgtm
What
Fixes: https://issues.redhat.com/browse/THREESCALE-10164
Verification Steps
apicast-config.jsonfile with the following contentThe response should be HTTP/1.1 400 Bad Request
APICAST_LARGE_CLIENT_HEADER_BUFFERS=4 12kThe response should be HTTP/1.1 200 OK