detailyang / lua-resty-cors

It's the implement of CORS on OpenResty
MIT License
56 stars 18 forks source link

local variables are not cleanup #4

Closed danielmotaleite closed 6 years ago

danielmotaleite commented 6 years ago

Hi

I tried to setup this module and noticed that while with few requests it looks that it works, with many we start to get duplicated headers and the server load keeps increasing

Adding a debug line like the one below

     for k, v in pairs(allow_hosts) do
         local from, to, err = ngx.re.find(origin, v, "jo")
+        ngx.log(ngx.ERR, "cors.re: " .. #allow_hosts )
         if from then
             matched = true

i get this result:

2018/10/22 21:15:32 [error] 24637#24637: *520 [lua] cors.lua:81: run(): cors.re: 6 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"
2018/10/22 21:15:32 [error] 24637#24637: *520 [lua] cors.lua:81: run(): cors.re: 6 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"
2018/10/22 21:15:32 [error] 24637#24637: *520 [lua] cors.lua:81: run(): cors.re: 6 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"
2018/10/22 21:15:32 [error] 24637#24637: *520 [lua] cors.lua:81: run(): cors.re: 6 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"
2018/10/22 21:15:32 [error] 24637#24637: *132 [lua] cors.lua:81: run(): cors.re: 3 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"
2018/10/22 21:15:32 [error] 24637#24637: *132 [lua] cors.lua:81: run(): cors.re: 3 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"
2018/10/22 21:15:32 [error] 24637#24637: *132 [lua] cors.lua:81: run(): cors.re: 3 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"
2018/10/22 21:15:32 [error] 24638#24638: *86 [lua] cors.lua:81: run(): cors.re: 9 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"
2018/10/22 21:15:32 [error] 24638#24638: *86 [lua] cors.lua:81: run(): cors.re: 9 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"
2018/10/22 21:15:32 [error] 24638#24638: *86 [lua] cors.lua:81: run(): cors.re: 9 while reading response header from upstream, client: 127.0.0.1, server: site-staging.example.com, request: "HEAD / HTTP/1.1", upstream: "http://127.0.0.1:2081/", host: "site-staging.example.com"

the allow_host jumps from 3 (the real number of allow_host configs i have) to 6, 9, 12, etc and then randomly resets back to 3

Looks like that the variable sometimes is global, other times local

I tried to add this between the for and the if check:

+    -- cleanup variables so they do not grow on next request
+    allow_hosts = {}
     if matched == false then

and it do help, makes it less common, but still get some higher count than 3 in the allow_host

So any idea what is going on?

i'm using nginx 1.14.0 and libluajit-5.1-2 on a ubuntu xenial

detailyang commented 6 years ago

Sorry, my bad.

We should use the module in init_by_lua_block to avoid duplicate init as the following:

http {
      init_by_lua_block {
        local cors = require('lib.resty.cors');
        cors.allow_host([==[.*\.google\.com]==])
        cors.allow_host([==[.*\.facebook\.com]==])
        cors.expose_header('x-custom-field1')
        cors.expose_header('x-custom-field2')
        cors.allow_method('GET')
        cors.allow_method('POST')
        cors.allow_method('PUT')
        cors.allow_method('DELETE')
        cors.allow_header('x-custom-field1')
        cors.allow_header('x-custom-field2')
        cors.max_age(7200)
        cors.allow_credentials(false)
      }

      header_filter_by_lua_block {
        local cors = require('lib.resty.cors');
        cors.run()
    }
}

Can you try this?

danielmotaleite commented 6 years ago

Thanks, that works... sort of! :smile:

while now i do not see the variable growing and load keeps constant, i do have several virtual hosts where the allow_host is different. Putting this directly in the init_by_lua_block makes the allow_host list global to all virtualhosts. Usually the methods and age are the same for all, the allow_host is the one that vary.

Is there any way to workaround that? maybe setting those fields via a nginx variable and importing then in the lua... without duplicate then or a cors.allow_host(cleanup) before setting it up, to make sure it is not reused?

detailyang commented 6 years ago

Hey, @danielmotaleite I rewrite codebase which uses table hash to allow reinit allow_host and expose_header and such on.

You can run the code in header_filter_by_lua_block in different server conf if you want like as the following:

server {
   server_name xxx;
   header_filter_by_lua_block {
        local cors = require('lib.resty.cors');
        local cors = require('lib.resty.cors');
        cors.allow_host([==[.*\.google\.com]==])
        cors.allow_host([==[.*\.facebook\.com]==])
        cors.expose_header('x-custom-field1')
        cors.expose_header('x-custom-field2')
        cors.allow_method('GET')
        cors.allow_method('POST')
        cors.allow_method('PUT')
        cors.allow_method('DELETE')
        cors.allow_header('x-custom-field1')
        cors.allow_header('x-custom-field2')
        cors.max_age(7200)
        cors.allow_credentials(false)
        cors.run()
    }
}

server {
   server_name yyy;
   header_filter_by_lua_block {
        local cors = require('lib.resty.cors');
        local cors = require('lib.resty.cors');
        cors.allow_host([==[.*\.twitter\.com]==])
        cors.allow_host([==[.*\.wiki\.com]==])
        cors.expose_header('x-custom-field1')
        cors.expose_header('x-custom-field2')
        cors.allow_method('GET')
        cors.allow_method('POST')
        cors.allow_method('PUT')
        cors.allow_method('DELETE')
        cors.allow_header('x-custom-field1')
        cors.allow_header('x-custom-field2')
        cors.max_age(7200)
        cors.allow_credentials(false)
        cors.run()
    }
}
danielmotaleite commented 6 years ago

Thanks for the change!

The code looks good, but i still see the allow_host size increasing ... ... WAIT, found the problem! you have a wrong "hosts" in this line:

allow_hosts[host] = hosts

If i replace that hosts with host it works fine

i also assume that the duplicate local cors = ... lines is a mistake too. I also removed the init_by_lua_block as it doesn't look as it is needed anymore.

So fixing this and adding the break in the https://github.com/detailyang/lua-resty-cors/issues/5 i would say this is perfect! :)

Again, many thanks!

detailyang commented 6 years ago

fixed