[resty.http_ng] use http/s proxy for backend and upstream connections #800
Conversation
mikz
force-pushed
the
use-http-proxy
branch
2 times, most recently
from
ec4f24f to
049fd9d
Compare
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
gateway/src/resty/http_ng/proxy.lua
Outdated
| local resty_env = require 'resty.env' | ||
| local http = require "resty.http" | ||
| local resty_url = require "resty.url" | ||
| local balancer = require 'apicast.balancer' |
gateway/src/resty/http_ng/proxy.lua
Outdated
| end | ||
|
|
||
| function _M.request(url) | ||
| local httpc = http.new() |
There was a problem hiding this comment.
accessing undefined variable 'http'
| _M.content = function() | ||
| if not ngx.headers_sent then | ||
| ngx.exec("@upstream") | ||
| _M.content = function(_, context) |
mikz
force-pushed
the
use-http-proxy
branch
2 times, most recently
from
02a8e9e to
1a9d8d6
Compare
gateway/src/proxy.lua
Outdated
| data, err, partial = readline() | ||
|
|
||
| local match = tab_new(1,3) | ||
| match, err = re_match(data, [[^(?<method>[A-Z]+)\s(?<uri>\S+)\s(?<http>\S+)$]], 'oj', nil, match) |
There was a problem hiding this comment.
value assigned to variable 'err' is unused
gateway/src/proxy.lua
Outdated
|
|
||
| local readline = client:receiveuntil(cr_lf) | ||
| local data, partial | ||
| data, err, partial = readline() |
There was a problem hiding this comment.
value assigned to variable 'err' is overwritten on line 183 before use
gateway/src/proxy.lua
Outdated
| if not client then return nil, err end | ||
|
|
||
| local readline = client:receiveuntil(cr_lf) | ||
| local data, partial |
There was a problem hiding this comment.
variable 'partial' is never accessed
gateway/conf/proxy.conf
Outdated
| resolver local=on; | ||
|
|
||
|
|
||
| # define a TCP server listening on the port 1234: |
There was a problem hiding this comment.
Should be 8099 instead of 1234.
gateway/src/proxy.lua
Outdated
| end | ||
| end | ||
|
|
||
| function _M.preread() |
There was a problem hiding this comment.
Is this needed to override the default behavior?
There was a problem hiding this comment.
Nope. I'll remove this.
gateway/src/proxy.lua
Outdated
|
|
||
| local upstream = ngx.socket.tcp() | ||
|
|
||
| if match.method == 'CONNECT' then |
There was a problem hiding this comment.
We should check whether there was an error first. Otherwise, this could fail.
There was a problem hiding this comment.
Same in the ngx_re_match below.
| request.proxy = resty_url.parse(proxy_url) | ||
| request.uri = uri | ||
|
|
||
| local ok, err |
| request.proxy = resty_url.parse(proxy_url) | ||
| request.uri = uri | ||
|
|
||
| local ok, err |
gateway/src/apicast/upstream.lua
Outdated
|
|
||
| local function v1() | ||
| ngx.ctx.upstream_server = upstream | ||
| ngx.ctx.upstream = resty_resolver:instance():get_servers(upstream.server, { port = upstream.port }) |
There was a problem hiding this comment.
accessing undefined variable 'upstream'
gateway/src/apicast/upstream.lua
Outdated
| url.host, { port = url.port }) | ||
|
|
||
| ngx.var.proxy_pass = proxy_pass(url) | ||
| ngx.req.set_header('Host', url.host) |
There was a problem hiding this comment.
accessing undefined variable 'url'
| @@ -80,18 +70,14 @@ function _M:rewrite(context) | |||
| end | |||
|
|
|||
| function _M.content(_, context) | |||
| local new_upstream = context.new_upstream | |||
| local upstream = context[self] | |||
There was a problem hiding this comment.
accessing undefined variable 'self'
gateway/src/apicast/upstream.lua
Outdated
| ngx.ctx.upstream = resty_resolver:instance():get_servers( | ||
| url.host, { port = url.port }) | ||
|
|
||
| ngx.var.proxy_pass = proxy_pass(url) |
There was a problem hiding this comment.
accessing undefined variable 'url'
gateway/src/apicast/upstream.lua
Outdated
|
|
||
| local function v2() | ||
| ngx.ctx.upstream = resty_resolver:instance():get_servers( | ||
| url.host, { port = url.port }) |
There was a problem hiding this comment.
accessing undefined variable 'url'
gateway/src/apicast/balancer.lua
Outdated
| @@ -15,11 +15,22 @@ local function exit_service_unavailable() | |||
| ngx.exit(ngx.status) | |||
| end | |||
|
|
|||
| function _M.call(_, _, balancer) | |||
| balancer = balancer or _M.default_balancer | |||
| function _M:call(context, bal) | |||
There was a problem hiding this comment.
cyclomatic complexity of method '_M.call' is too high (8 > 7)
gateway/src/apicast/balancer.lua
Outdated
| @@ -15,11 +15,22 @@ local function exit_service_unavailable() | |||
| ngx.exit(ngx.status) | |||
| end | |||
|
|
|||
| function _M.call(_, _, balancer) | |||
| balancer = balancer or _M.default_balancer | |||
| function _M:call(context, bal) | |||
spec/upstream_spec.lua
Outdated
| @@ -0,0 +1,6 @@ | |||
| local _M = require('apicast.upstream') | |||
gateway/src/apicast/balancer.lua
Outdated
| local function exit_service_unavailable() | ||
| ngx.status = ngx.HTTP_SERVICE_UNAVAILABLE | ||
| ngx.exit(ngx.status) | ||
| end | ||
|
|
||
| function _M.call(_, _, balancer) | ||
| balancer = balancer or _M.default_balancer | ||
| function _M:call(context, bal) |
There was a problem hiding this comment.
cyclomatic complexity of method '_M.call' is too high (9 > 7)
gateway/src/resty/balancer.lua
Outdated
|
|
||
| ok, err = balancer.set_current_peer(address, port) | ||
| local peer, err = self:select_peer(peers) |
There was a problem hiding this comment.
value assigned to variable 'err' is unused
spec/balancer_spec.lua
Outdated
| b.peers = function() return { { '127.0.0.2' } } end | ||
| ngx.var.proxy_pass = 'https://example.com' | ||
| local balancer = setmetatable({ | ||
| set_current_peer = spy.new(function(...) return true end), |
There was a problem hiding this comment.
unused variable length argument
spec/luassert_helper.lua
Outdated
| end | ||
|
|
||
| local function contains(state, arguments, level) | ||
| local level = (level or 1) + 1 |
There was a problem hiding this comment.
variable 'level' was previously defined as an argument on line 90
spec/resty/balancer_spec.lua
Outdated
| describe(':set_current_peer', function() | ||
| it('returns ok when it sets the peer', function() | ||
| local test = resty_balancer.new(function(peers) return peers[1] end) | ||
| local set_current_peer = stub.new(test.balancer, 'set_current_peer', function(...) return true end) |
There was a problem hiding this comment.
unused variable length argument
gateway/src/apicast/balancer.lua
Outdated
| balancer = balancer or _M.default_balancer | ||
| local host = ngx.var.proxy_host -- NYI: return to lower frame | ||
| local peers = balancer:peers(ngx.ctx[host]) | ||
| function _M:call(context, bal) |
There was a problem hiding this comment.
cyclomatic complexity of method '_M.call' is too high (10 > 7)
gateway/src/apicast/balancer.lua
Outdated
| balancer = balancer or _M.default_balancer | ||
| local host = ngx.var.proxy_host -- NYI: return to lower frame | ||
| local peers = balancer:peers(ngx.ctx[host]) | ||
| function _M:call(context, bal) |
mikz
force-pushed
the
use-http-proxy
branch
2 times, most recently
from
6d94e88 to
2376525
Compare
gateway/src/resty/http/proxy.lua
Outdated
| request.proxy = resty_url.parse(proxy_url) | ||
| request.uri = uri | ||
|
|
||
| local ok, err |
gateway/src/resty/http/proxy.lua
Outdated
| request.proxy = resty_url.parse(proxy_url) | ||
| request.uri = uri | ||
|
|
||
| local ok, err |
t/fixtures/proxy.lua
Outdated
| end | ||
|
|
||
| local match = tab_new(1,3) | ||
| match, err = ngx_re_match(data, [[^(?<method>[A-Z]+)\s(?<uri>\S+)\s(?<http>\S+)$]], 'oj', nil, match) |
There was a problem hiding this comment.
value assigned to variable 'err' is unused
t/fixtures/proxy.lua
Outdated
| return socket:send(data) | ||
| end | ||
|
|
||
| local function read_stream(name, input, output) |
There was a problem hiding this comment.
cyclomatic complexity of function 'read_stream' is too high (8 > 7)
t/fixtures/proxy.lua
Outdated
| if ngx.config.debug then | ||
| if ngx.ctx.upstream == socket then | ||
| -- ngx.log(ngx.DEBUG, 'sending: ', data , ' to upstream ', ngx.ctx.upstream_host, ':', ngx.ctx.upstream_port) | ||
| else |
t/fixtures/proxy.lua
Outdated
|
|
||
| local function send(socket, data) | ||
| if ngx.config.debug then | ||
| if ngx.ctx.upstream == socket then |
t/fixtures/proxy.lua
Outdated
| end | ||
|
|
||
| local match = tab_new(1,3) | ||
| match, err = ngx_re_match(data, [[^(?<method>[A-Z]+)\s(?<uri>\S+)\s(?<http>\S+)$]], 'oj', nil, match) |
There was a problem hiding this comment.
value assigned to variable 'err' is unused
t/fixtures/proxy.lua
Outdated
| return socket:send(data) | ||
| end | ||
|
|
||
| local function read_stream(name, input, output) |
There was a problem hiding this comment.
cyclomatic complexity of function 'read_stream' is too high (8 > 7)
t/fixtures/proxy.lua
Outdated
| if ngx.config.debug then | ||
| if ngx.ctx.upstream == socket then | ||
| -- ngx.log(ngx.DEBUG, 'sending: ', data , ' to upstream ', ngx.ctx.upstream_host, ':', ngx.ctx.upstream_port) | ||
| else |
t/fixtures/proxy.lua
Outdated
|
|
||
| local function send(socket, data) | ||
| if ngx.config.debug then | ||
| if ngx.ctx.upstream == socket then |
t/fixtures/proxy.lua
Outdated
| end | ||
|
|
||
| local match = tab_new(1,3) | ||
| match, err = ngx_re_match(data, [[^(?<method>[A-Z]+)\s(?<uri>\S+)\s(?<http>\S+)$]], 'oj', nil, match) |
There was a problem hiding this comment.
value assigned to variable 'err' is unused
t/fixtures/proxy.lua
Outdated
| return socket:send(data) | ||
| end | ||
|
|
||
| local function read_stream(name, input, output) |
There was a problem hiding this comment.
cyclomatic complexity of function 'read_stream' is too high (8 > 7)
t/fixtures/proxy.lua
Outdated
| if ngx.config.debug then | ||
| if ngx.ctx.upstream == socket then | ||
| -- ngx.log(ngx.DEBUG, 'sending: ', data , ' to upstream ', ngx.ctx.upstream_host, ':', ngx.ctx.upstream_port) | ||
| else |
t/fixtures/proxy.lua
Outdated
|
|
||
| local function send(socket, data) | ||
| if ngx.config.debug then | ||
| if ngx.ctx.upstream == socket then |
t/http-proxy.t
Outdated
|
|
||
| require("t/http_proxy.pl"); | ||
|
|
||
| # Can't run twice because one of the test checks the contents of the cache, and |
gateway/src/apicast/http_proxy.lua
Outdated
| return ngx.exit(ngx.HTTP_SERVICE_UNAVAILABLE) | ||
| end | ||
|
|
||
| local res, err = httpc:request(request) |
There was a problem hiding this comment.
variable 'err' was previously defined on line 130
| @@ -40,7 +43,15 @@ _M.async = function(request) | |||
| end | |||
| end | |||
|
|
|||
| local ok, err = httpc:connect(host, port) | |||
|
|
|||
| local proxy_uri = httpc:get_proxy_uri(scheme, host) | |||
mikz
changed the title
mikz
changed the title
mikz
force-pushed
the
use-http-proxy
branch
2 times, most recently
from
74725a8 to
42029d6
Compare
| @@ -22,7 +22,7 @@ local function init_config(config) | |||
| local upstream, err = Upstream.new(rule.url) | |||
|
|
|||
| if upstream then | |||
| tab_insert(res, { regex = rule.regex, upstream = upstream }) | |||
| tab_insert(res, { regex = rule.regex, upstream = upstream, url = rule.url }) | |||
There was a problem hiding this comment.
I think that because of the change below we no longer need to store upstream here.
| @@ -49,7 +49,8 @@ function _M:rewrite(context) | |||
| for _, rule in ipairs(self.rules) do | |||
| if match(req_uri, rule.regex) then | |||
| ngx.log(ngx.DEBUG, 'upstream policy uri: ', req_uri, ' regex: ', rule.regex, ' match: true') | |||
| context[self] = rule.upstream | |||
| -- better to allocate new object for each request as it is going to get mutated | |||
| context[self] = Upstream.new(rule.url) | |||
There was a problem hiding this comment.
In this case, it doesn't matter much because we're just storing one object, but it might be a better practice to namespace what we store in the context: context[self].url = ...
There was a problem hiding this comment.
Another option would be to store just the url instead of the Upstream instance.
The Upstream can be instantiated in content(). Not a big difference, though.
There was a problem hiding this comment.
We will have to try to reduce the allocations in the future, so I'm trying to not create objects when it is not absolutely necessary. In this case doing a namespace would require another table.
I'm parsing the upstream here, so potential failure will show early in the rewrite phase before other calls are made (like auth).
| @@ -61,7 +62,7 @@ function _M:content(context) | |||
| local upstream = context[self] | |||
|
|
|||
| if upstream then | |||
| upstream:set_request_host() | |||
| upstream:rewrite_request() | |||
There was a problem hiding this comment.
This is a bit confusing because the function in the call below, upstream:call(), already calls rewrite_request() at least in some cases.
| end | ||
|
|
||
| local mt = { | ||
| __index = _M | ||
| } | ||
|
|
||
| local function split_path(path) |
There was a problem hiding this comment.
Not something for now, but to take into account in future refactorings. I think we're already doing this in other modules, so it might be a good idea to extract methods like this one to a Request module that can be reused between policies.
There was a problem hiding this comment.
Yep, this is probably the most common helper function. We should offer performance optimized version and reuse it.
| local function split_path(path) | ||
| if not path then return end | ||
|
|
||
| local start = str_find(path, '?', 1, true) |
There was a problem hiding this comment.
Can the find + subs be replaced with a single "split by ?" ?
There was a problem hiding this comment.
I did some benchmarking, but I don't have the results and I think this was fully JIT compatible and faster.
| end) | ||
| end | ||
|
|
||
| describe(':rewrite_request', function() |
There was a problem hiding this comment.
I think we should add tests for this part of the code too:
if uri.query then
ngx.req.set_uri_args(uri.query)
end
Fixes #709
Builds on top of #835
By configuring standard environment variables
http_proxy,HTTP_PROXY,no_proxy,NO_PROXY,http_proxy,HTTPS_PROXYyou can instruct APIcast to use those instead of direct connection.Proxy does not support any authentication yet.
Tests
ngx.exec(@upstream)ngx.exec(@upstream)+ SSL