Buffered backends fix #44

andrewloux · 2018-04-10T13:57:19Z

Superseded by kubernetes#2408

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

https://github.com/Shopify/edgescale/issues/398

Special notes for your reviewer:

Could someone look at the test I've written? What I'm doing right now is updating the ingress-nginx controller's client-body-buffer-size to be set to 1k so I can reproduce the problem.
After the test, the attribute is reset.

r/ @ElvinEfendi @zrdaley @fmejia97

ElvinEfendi · 2018-04-10T16:12:43Z

rootfs/etc/nginx/lua/configuration.lua

@@ -7,6 +7,21 @@ function _M.get_backends_data()
  return configuration_data:get("backends")
 end

+function _M.new_backends()


I think the convention is to name these kind of functions _handler, no? i.e backends_handler in this case.

--

Also since you are using this only locally, you don't have to export it. You can do

local function backends_handler() ...

Oh wait this is just for extracting request body. IMO we should not make it backends specific - there's nothing in the function body that's backends specific - this should just be a helper function that returns request body and nil or in case of error nil and error message

ElvinEfendi · 2018-04-10T16:17:17Z

rootfs/etc/nginx/lua/configuration.lua

-  ngx.req.read_body()
+  local backends = _M.new_backends()
+  if not backends then
+    ngx.log(ngx.ERR, "dynamic-configuration: POST carries empty response body")


dynamic-configuration

I'm not sure what value this adds. Can you describe a real use case?

Other users might see the error, and see that it originated from this line in configuration.lua, but still might not know what "component" of ingress-nginx is not working. Given the annotation is named dynamic-configuration, I figured that this is a good way of making error logs from this lua module more meaningful.

ElvinEfendi · 2018-04-10T16:18:01Z

test/e2e/framework/framework.go

@@ -270,7 +270,7 @@ func UpdateDeployment(kubeClientSet kubernetes.Interface, namespace string, name
 		return err
 	}

-	err = WaitForPodsReady(kubeClientSet, 60*time.Second, replicas, namespace, metav1.ListOptions{
+	err = WaitForPodsReady(kubeClientSet, 90*time.Second, replicas, namespace, metav1.ListOptions{


probably started to be an issue with 60s because you're spinning up 30 replicas

haha yeah, I'm pretty sure that's it.

ElvinEfendi · 2018-04-10T16:20:24Z

test/e2e/lua/dynamic_configuration.go

+
+			cm.Data["client-body-buffer-size"] = "1k"
+			_, updateErr := f.KubeClientSet.CoreV1().ConfigMaps("ingress-nginx").Update(cm)
+			Expect(updateErr).ToNot(HaveOccurred())


There's no helper function in the test framework for updating configmap? If not I suggest we extract it out into the framework code so that it can be reused.

I'll take a closer look. I notice that Zenara made a similar updateConfigmap method in her session affinity PR as well, so definitely should be made generic if there isn't already a way.

ElvinEfendi · 2018-04-10T16:20:50Z

test/e2e/lua/dynamic_configuration.go

+			Expect(getErr).ToNot(HaveOccurred())
+			oldClientBodyBufferSize, oldClientBodyBufferSizeExists := cm.Data["client-body-buffer-size"];
+
+			cm.Data["client-body-buffer-size"] = "1k"


is this the lowest possible value?

From here: http://nginx.org/en/docs/syntax.html - I think so. Could also always make it 0 to make sure it such that it's always writing out to the temp file. What do you think?

if possible I'd go lower than this so that we don't have to spin up 30 replicas in test

I think 0 means never write to temp file

--

reading http://nginx.org/en/docs/syntax.html I think you can set in bytes

Makes sense. Re-read that link, so without suffix denotes bytes.

Will do, thanks Elvin

ElvinEfendi · 2018-04-10T21:02:05Z

rootfs/etc/nginx/lua/configuration.lua

@@ -7,6 +7,21 @@ function _M.get_backends_data()
  return configuration_data:get("backends")
 end

+local function fetch_request_body()
+  ngx.req.read_body()
+  local backends = ngx.req.get_body_data()


just body? since this is not backends specific

ElvinEfendi · 2018-04-10T21:03:51Z

test/e2e/lua/dynamic_configuration.go

+				Set("Host", "foo.com").
+				End()
+			Expect(len(errs)).Should(BeNumerically("==", 0))
+			Expect(resp.StatusCode).Should(Equal(http.StatusOK))


what's this for? we already assert that backend i s available at https://github.com/Shopify/ingress/pull/44/files#diff-8700c46a8379c603b5e41ccbaff6b71eR60

I think you wanna send this request after updating the client max body size, to make sure we parsed request body correctly

ElvinEfendi · 2018-04-10T21:04:33Z

test/e2e/lua/dynamic_configuration.go

+			_, updateErr := f.KubeClientSet.CoreV1().ConfigMaps("ingress-nginx").Update(cm)
+			Expect(updateErr).ToNot(HaveOccurred())
+
+			replicas := 4


1 replica is not enough to generate a payload with sie of over 1 byte?

Not enough. I'm not entirely certain why.

Did some looking into upstream, might be possibly happening because Nginx might be pre-reading part of the request body. If it's able to pre-read the entire request body in its pre-read buffer - it might not even check the client_body_buffer_size.

See: https://github.com/nginx/nginx/blob/e66073c4d3a3d25b001dee617ee22d2a969ceab2/src/event/ngx_event_pipe.c#L149

But 2 replicas does look like it is enough :)

ElvinEfendi · 2018-04-10T21:09:29Z

rootfs/etc/nginx/lua/configuration.lua

+  if not backends then
+    -- request body might've been wrote to tmp file if body > client_body_buffer_size
+    local file_name = ngx.req.get_body_file()
+    local file = assert(io.open(file_name, "rb"))


what does assert do? what happens if io.open fails?

ElvinEfendi · 2018-04-10T21:09:51Z

rootfs/etc/nginx/lua/configuration.lua

+    local file_name = ngx.req.get_body_file()
+    local file = assert(io.open(file_name, "rb"))
+    backends = file:read("*all")
+    file:close()


what happens any of this function calls fail?

lines 18, 19 should be safe.

file:read & file:close will return a nil if the read or close operations are problematic.

And then since backends is nil, this will be handled appropriately here: https://github.com/Shopify/ingress/pull/44/files#diff-27d52aa3bacfc9feff2d5eb208af0a17R52

ElvinEfendi · 2018-04-10T21:11:54Z

rootfs/etc/nginx/lua/configuration.lua

+    ngx.log(ngx.ERR, "dynamic-configuration: POST carries empty response body")
+    ngx.status = ngx.HTTP_BAD_REQUEST
+    return
+  end


This is a change of behaviour - i can not think of anything bad with it, but also don't see why it it necessary. Did you add it merely so that you can assert for absence of this logs in the e2e test?

The intention with this is to not update the config if the request_body is nil, as we were doing previously. This could be problematic, and is essentially a silent failure mode - since there are no symptoms i.e. 200 was being returned.

Understandably, if the balancer module cannot parse the backends data, then it doesn't sync the update - so the failure is somewhat limited, but I still think that the logging and 5xx response increases visibility here.

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

andrewloux · 2018-04-25T05:21:56Z

Closing in favour of kubernetes#2408

ElvinEfendi reviewed Apr 10, 2018

View reviewed changes

andrewloux force-pushed the buffered-backends-fix branch from 00880b8 to f593663 Compare April 12, 2018 20:26

ElvinEfendi mentioned this pull request Apr 17, 2018

BUG: --enable-dynamic-configuration=true doesn't work properly kubernetes/ingress-nginx#2225

Closed

andrewloux force-pushed the buffered-backends-fix branch from f593663 to d4a395c Compare April 23, 2018 14:39

andrewloux closed this Apr 25, 2018

andrewloux deleted the buffered-backends-fix branch April 25, 2018 05:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffered backends fix #44

Buffered backends fix #44

andrewloux commented Apr 10, 2018 •

edited

Loading

ElvinEfendi Apr 10, 2018

ElvinEfendi Apr 10, 2018

ElvinEfendi Apr 10, 2018

andrewloux Apr 10, 2018

ElvinEfendi Apr 10, 2018

andrewloux Apr 10, 2018

ElvinEfendi Apr 10, 2018

andrewloux Apr 10, 2018

ElvinEfendi Apr 10, 2018

andrewloux Apr 10, 2018

ElvinEfendi Apr 10, 2018

andrewloux Apr 10, 2018

ElvinEfendi Apr 10, 2018

ElvinEfendi Apr 10, 2018

andrewloux Apr 10, 2018

ElvinEfendi Apr 10, 2018

ElvinEfendi Apr 10, 2018

ElvinEfendi Apr 10, 2018 •

edited

Loading

ElvinEfendi Apr 10, 2018 •

edited

Loading

andrewloux Apr 12, 2018

andrewloux Apr 12, 2018

ElvinEfendi Apr 10, 2018

ElvinEfendi Apr 10, 2018

andrewloux Apr 23, 2018

ElvinEfendi Apr 10, 2018

andrewloux Apr 23, 2018

andrewloux commented Apr 25, 2018

Buffered backends fix #44

Buffered backends fix #44

Conversation

andrewloux commented Apr 10, 2018 • edited Loading

Superseded by kubernetes#2408

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewloux commented Apr 25, 2018

andrewloux commented Apr 10, 2018 •

edited

Loading

ElvinEfendi Apr 10, 2018 •

edited

Loading

ElvinEfendi Apr 10, 2018 •

edited

Loading