Skip to content
This repository has been archived by the owner on Apr 27, 2021. It is now read-only.

Buffered backends fix #44

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions rootfs/etc/nginx/lua/configuration.lua
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Choose a reason for hiding this comment

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

just body? since this is not backends specific


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"))

Choose a reason for hiding this comment

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

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

backends = file:read("*all")
file:close()

Choose a reason for hiding this comment

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

what happens any of this function calls fail?

Copy link
Author

Choose a reason for hiding this comment

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

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

end

return backends
end

function _M.call()
if ngx.var.request_method ~= "POST" and ngx.var.request_method ~= "GET" then
ngx.status = ngx.HTTP_BAD_REQUEST
Expand All @@ -26,11 +41,16 @@ function _M.call()
return
end

ngx.req.read_body()
local backends = fetch_request_body()
if not backends then
ngx.log(ngx.ERR, "dynamic-configuration: POST carries empty response body")
ngx.status = ngx.HTTP_BAD_REQUEST
return
end

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.


local success, err = configuration_data:set("backends", ngx.req.get_body_data())
local success, err = configuration_data:set("backends", backends)
if not success then
ngx.log(ngx.ERR, "error while saving configuration: " .. tostring(err))
ngx.log(ngx.ERR, "dynamic-configuration: error updating configuration: " .. tostring(err))
ngx.status = ngx.HTTP_BAD_REQUEST
return
end
Expand Down
45 changes: 45 additions & 0 deletions test/e2e/lua/dynamic_configuration.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ import (
var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
f := framework.NewDefaultFramework("dynamic-configuration")

var defaultNginxConfigMapData map[string]string = nil

BeforeEach(func() {
err := enableDynamicConfiguration(f.KubeClientSet)
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -68,11 +70,20 @@ var _ = framework.IngressNginxDescribe("Dynamic Configuration", func() {
Expect(err).ToNot(HaveOccurred())
Expect(log).ToNot(ContainSubstring("could not dynamically reconfigure"))
Expect(log).To(ContainSubstring("first sync of Nginx configuration"))

if defaultNginxConfigMapData == nil {
defaultNginxConfigMapData, err = f.GetNginxConfigMapData()
Expect(err).NotTo(HaveOccurred())
Expect(defaultNginxConfigMapData).NotTo(BeNil())
}
})

AfterEach(func() {
err := disableDynamicConfiguration(f.KubeClientSet)
Expect(err).NotTo(HaveOccurred())

err = f.SetNginxConfigMapData(defaultNginxConfigMapData)
Expect(err).NotTo(HaveOccurred())
})

Context("when only backends change", func() {
Expand Down Expand Up @@ -111,6 +122,40 @@ 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() {
resp, _, errs := gorequest.New().
Get(fmt.Sprintf("%s?id=endpoints_only_changes", f.NginxHTTPURL)).
Set("Host", "foo.com").
End()
Expect(len(errs)).Should(BeNumerically("==", 0))
Expect(resp.StatusCode).Should(Equal(http.StatusOK))

Choose a reason for hiding this comment

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

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

Copy link

@ElvinEfendi ElvinEfendi Apr 10, 2018

Choose a reason for hiding this comment

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

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


// Update client-body-buffer-size to 1 byte
err := f.UpdateNginxConfigMapData("client-body-buffer-size", "8")
Expect(err).NotTo(HaveOccurred())

replicas := 2
err = framework.UpdateDeployment(f.KubeClientSet, f.Namespace.Name, "http-svc", replicas,
func(deployment *appsv1beta1.Deployment) error {
deployment.Spec.Replicas = framework.NewInt32(int32(replicas))
_, err := f.KubeClientSet.AppsV1beta1().Deployments(f.Namespace.Name).Update(deployment)
return err
})
Expect(err).NotTo(HaveOccurred())

time.Sleep(5 * time.Second)
log, err := f.NginxLogs()

Expect(err).ToNot(HaveOccurred())
Expect(log).ToNot(BeEmpty())
index := strings.Index(log, "id=endpoints_only_changes")
restOfLogs := log[index:]

By("POSTing new backends to Lua endpoint")
Expect(restOfLogs).To(ContainSubstring("a client request body is buffered to a temporary file"))
Expect(restOfLogs).ToNot(ContainSubstring("POST carries empty response body"))
})

It("should handle annotation changes", func() {
ingress, err := f.KubeClientSet.ExtensionsV1beta1().Ingresses(f.Namespace.Name).Get("foo.com", metav1.GetOptions{})
Expect(err).ToNot(HaveOccurred())
Expand Down