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

Jump to bottom

Configurable proxy_request_buffering per location.. #1335

Merged
merged 1 commit into from
Sep 12, 2017
Merged

Configurable proxy_request_buffering per location.. #1335

merged 1 commit into from
Sep 12, 2017

Conversation

auhlig
Copy link
Contributor

@auhlig auhlig commented Sep 11, 2017

.. fix typo

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 11, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 11, 2017
@auhlig auhlig changed the title Configruable proxy request buffering per location.. Configurable proxy request buffering per location.. Sep 11, 2017
@auhlig auhlig changed the title Configurable proxy request buffering per location.. Configurable proxy_request_buffering per location.. Sep 11, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 44.013% when pulling dd8fb96 on auhlig:requestbuffering into 18ea2f7 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Sep 12, 2017

@auhlig please squash the commits

@aledbf aledbf self-assigned this Sep 12, 2017
aledbf
aledbf reviewed Sep 12, 2017
View reviewed changes
controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl Outdated
@@ -740,6 +740,7 @@ stream {
proxy_buffering off;
proxy_buffer_size "{{ $location.Proxy.BufferSize }}";
proxy_buffers 4 "{{ $location.Proxy.BufferSize }}";
proxy_request_buffering on "{{ $location.Proxy.RequestBuffering }}";
Copy link
Member

Choose a reason for hiding this comment

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

"on" should not be here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Removed.

@auhlig
Copy link
Contributor Author

auhlig commented Sep 12, 2017

Squashed @aledbf.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 43.989% when pulling f81d6c3a64947e0f423266363c447ac56e003e66 on auhlig:requestbuffering into 0a96924 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Sep 12, 2017

@auhlig where are you setting the default to on?

@auhlig
Copy link
Contributor Author

auhlig commented Sep 12, 2017

@aledbf Here aa191c8#diff-f187832f2d974cf9415d15d7115778acR425 ;)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 44.009% when pulling aa191c8 on auhlig:requestbuffering into a5084d1 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Sep 12, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 12, 2017
@aledbf
Copy link
Member

aledbf commented Sep 12, 2017

@auhlig thanks!

@aledbf aledbf merged commit 587a344 into kubernetes:master Sep 12, 2017
@auhlig auhlig deleted the requestbuffering branch September 13, 2017 05:50
@albertvaka
Copy link
Contributor

@auhlig Since this patch my ingress controller won't start:

2017/09/14 14:38:47 [emerg] 65#65: invalid value "" in "proxy_request_buffering" directive, it must be "on" or "off" in /tmp/nginx-cfg822961353:409
nginx: [emerg] invalid value "" in "proxy_request_buffering" directive, it must be "on" or "off" in /tmp/nginx-cfg822961353:409
nginx: configuration file /tmp/nginx-cfg822961353 test failed

@aledbf
Copy link
Member

aledbf commented Sep 14, 2017

@albertvaka #1363 fixes the error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aledbf aledbf aledbf left review comments

Assignees

@aledbf aledbf

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@auhlig @k8s-reviewable @coveralls @aledbf @albertvaka @k8s-ci-robot