Read backends data even if buffered to temp file #2408

andrewloux · 2018-04-23T19:37:45Z

What this PR does / why we need it:

Read from temp file if the request body from the controller's POST to update backends is larger than client_body_buffer_size.
There was an to address this problem over here: https://github.com/kubernetes/ingress-nginx/pull/2309/files - but the updates will still silently fail by updating the lua shared table with nil if the controller POST body is larger than 10m.
This PR makes sure that the dictionary is not updated if there is either too large of a request body - or an empty request body.

Which issue this PR fixes

BUG: --enable-dynamic-configuration=true doesn't work properly #2225 (comment)

Special notes for your reviewer:
cc r/ @ElvinEfendi @zrdaley

aledbf · 2018-04-23T20:27:40Z

/approve

ElvinEfendi · 2018-04-23T23:51:46Z

rootfs/etc/nginx/lua/configuration.lua

+  local body = ngx.req.get_body_data()
+
+  if not body then
+    -- request body might've been wrote to tmp file if body > client_body_buffer_size


s/wrote/written

ElvinEfendi · 2018-04-23T23:54:17Z

test/e2e/lua/dynamic_configuration.go

@@ -39,6 +39,8 @@ import (
 var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
 	f := framework.NewDefaultFramework("dynamic-configuration")

+	var defaultNginxConfigMapData map[string]string = nil


where is this being used?

Nice catch, issa relic of a previous iteration. Will nip

ElvinEfendi · 2018-04-23T23:55:27Z

test/e2e/lua/dynamic_configuration.go

@@ -104,6 +112,30 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
 			Expect(restOfLogs).ToNot(ContainSubstring("first sync of Nginx configuration"))
 		})

+		It("configuration module should read from temp file when request body > client_body_buffer_size", func() {
+			// Update client-body-buffer-size to 1 byte
+			err := f.UpdateNginxConfigMapData("client-body-buffer-size", "8")


lower than 8 is not possible?

Didn't really make a difference, I could lower it though.

My hypothesis for why it doesn't matter comes from reading about how nginx applies the client body buffer size limit. Seems like there is sometimes Nginx pre-reads responses, and if Nginx is able to pre-read the entire response in the pre-read buffer - the client body buffer size is not looked at at all.

It also seems to compare only what is left of the response after pre-reading against the client body buffer size instead of the size of the entire response: https://github.com/nginx/nginx/blob/55b37eff8f700ea1dd279368d5a4a7b00f3c1344/src/http/ngx_http_request_body.c#L165

But yes, can lower it to 1/2 for clarity. Seems like we would still need the same amount of replicas though to reliably reproduce temp file buffering.

ElvinEfendi · 2018-04-23T23:57:03Z

test/e2e/lua/dynamic_configuration.go

@@ -104,6 +112,30 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
 			Expect(restOfLogs).ToNot(ContainSubstring("first sync of Nginx configuration"))
 		})

+		It("configuration module should read from temp file when request body > client_body_buffer_size", func() {


this test should not care about which module is doing what, rather it should make sure that given a payload of backends that can not be buffered then the request to that backends should still succeed

ElvinEfendi · 2018-04-23T23:58:43Z

please also run make code-generator and push the changes

ElvinEfendi · 2018-04-24T18:20:20Z

/lgtm

Actually read from the file Logs probably shouldn't assume knowledge of implementation detail Typos Added integration test, and dynamic update config refactor Don't force the 8k default Minimal test case to make the configuration/backends request body write to temp file Leverage new safe config updating methods, and use 2 replicas instead of 4 Small refactor Better integration test, addresses other feedback Update bindata

andrewloux · 2018-04-24T19:11:13Z

@ElvinEfendi added updated bindata, might need you to re-approve. Refactored the integration test to make more sense.

codecov-io · 2018-04-24T19:29:05Z

Codecov Report

Merging #2408 into master will decrease coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2408      +/-   ##
==========================================
- Coverage   41.05%   40.91%   -0.14%     
==========================================
  Files          74       74              
  Lines        5252     5252              
==========================================
- Hits         2156     2149       -7     
- Misses       2803     2809       +6     
- Partials      293      294       +1

Impacted Files	Coverage Δ
internal/file/bindata.go	`60.17% <ø> (ø)`	⬆️
cmd/nginx/main.go	`21.37% <0%> (-4.83%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb1d661...d3d383d. Read the comment docs.

ElvinEfendi · 2018-04-24T21:08:14Z

/lgtm

k8s-ci-robot · 2018-04-24T21:08:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, andrewlouis93, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 23, 2018

aledbf assigned ElvinEfendi Apr 23, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2018

aledbf requested a review from ElvinEfendi April 23, 2018 20:34

ElvinEfendi reviewed Apr 23, 2018

View reviewed changes

andrewloux force-pushed the updated-buffered-backends branch from 39b8986 to 6ac63e0 Compare April 24, 2018 18:06

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2018

andrewloux force-pushed the updated-buffered-backends branch from 6ac63e0 to d3d383d Compare April 24, 2018 19:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2018

k8s-ci-robot merged commit 9533aa4 into kubernetes:master Apr 24, 2018

andrewloux mentioned this pull request Apr 25, 2018

Buffered backends fix Shopify/ingress#44

Closed

ElvinEfendi deleted the updated-buffered-backends branch April 25, 2018 12:41

ElvinEfendi mentioned this pull request Apr 26, 2018

dynamic-configuration: attempt to index local 'backend' (a nil value) in 0.12.0-0.13.0 #2404

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read backends data even if buffered to temp file #2408

Read backends data even if buffered to temp file #2408

andrewloux commented Apr 23, 2018

aledbf commented Apr 23, 2018

ElvinEfendi Apr 23, 2018

ElvinEfendi Apr 23, 2018

andrewloux Apr 24, 2018

ElvinEfendi Apr 23, 2018

andrewloux Apr 24, 2018

ElvinEfendi Apr 23, 2018

ElvinEfendi commented Apr 23, 2018

ElvinEfendi commented Apr 24, 2018

andrewloux commented Apr 24, 2018

codecov-io commented Apr 24, 2018

ElvinEfendi commented Apr 24, 2018

k8s-ci-robot commented Apr 24, 2018

Read backends data even if buffered to temp file #2408

Read backends data even if buffered to temp file #2408

Conversation

andrewloux commented Apr 23, 2018

aledbf commented Apr 23, 2018

ElvinEfendi Apr 23, 2018

Choose a reason for hiding this comment

ElvinEfendi Apr 23, 2018

Choose a reason for hiding this comment

andrewloux Apr 24, 2018

Choose a reason for hiding this comment

ElvinEfendi Apr 23, 2018

Choose a reason for hiding this comment

andrewloux Apr 24, 2018

Choose a reason for hiding this comment

ElvinEfendi Apr 23, 2018

Choose a reason for hiding this comment

ElvinEfendi commented Apr 23, 2018

ElvinEfendi commented Apr 24, 2018

andrewloux commented Apr 24, 2018

codecov-io commented Apr 24, 2018

Codecov Report

ElvinEfendi commented Apr 24, 2018

k8s-ci-robot commented Apr 24, 2018