Add upstream policy #562
Add upstream policy #562
Conversation
| local new = _M.new | ||
|
|
||
| local function change_upstream(url) | ||
| ngx.ctx.upstream = resty_resolver:instance():get_servers( |
There was a problem hiding this comment.
This code is very similar to what we already have in Proxy.get_upstream() https://github.com/3scale/apicast/blob/6049ab33581483b0e69fc1ff6ab2fea38aa4a696/gateway/src/apicast/proxy.lua#L180
If we think that there might be other policies that need the same functionality, we should refactor what we have there to avoid duplicating code in the policies.
There was a problem hiding this comment.
I'd rather address this on a separate PR as this is an internal change that does not affect the schema of the policy.
davidor
force-pushed
the
upstream-policy
branch
from
bf415f1
to
c1fe155
Compare
|
|
||
| local function change_upstream(url) | ||
| ngx.ctx.upstream = resty_resolver:instance():get_servers( | ||
| url.host, { port = tonumber(url.port) }) |
There was a problem hiding this comment.
I need to check if tonumber() here is needed or if the resty_url.parse() call guarantees that we'll get a number here.
There was a problem hiding this comment.
davidor
force-pushed
the
upstream-policy
branch
3 times, most recently
from
a20434f
to
95eb0d2
Compare
t/apicast-policy-upstream.t
Outdated
| --- upstream | ||
| location / { | ||
| content_by_lua_block { | ||
| ngx.say('yay, api backend'); |
There was a problem hiding this comment.
Maybe we could print here the ngx.var.uri to verify what was received?
There was a problem hiding this comment.
The path in the location already checks schema, host, and port.
For the path we'll need to use a location different from / to be sure. I think that would be enough.
However, we could also use luassert inside the content_by_lua block to make these checks more explicit.
What do you think?
t/apicast-policy-upstream.t
Outdated
| --- upstream | ||
| location / { | ||
| content_by_lua_block { | ||
| ngx.say('yay, api backend'); |
There was a problem hiding this comment.
The same comment as https://github.com/3scale/apicast/pull/562/files#r163527794. Would be nice to verify the upstream uri+host+ whatever is important.
| { | ||
| "rules": [ | ||
| { "regex": "/i_dont_match", "url": "http://example.com" }, | ||
| { "regex": "/", "url": "http://test:$TEST_NGINX_SERVER_PORT" }, |
There was a problem hiding this comment.
I think we should test a case when the url has a path to see what is forwarded upstream.
There was a problem hiding this comment.
Agreed. Added in new commit.
davidor
force-pushed
the
upstream-policy
branch
from
de1b021
to
c80a6ba
Compare
| ] | ||
| } | ||
| --- upstream | ||
| location /a_path { |
There was a problem hiding this comment.
This can match /a_path/whatever. Would be good to at least define it as location = /a_path. But I'd also go the extra step and print ngx.var.uri in the response. So we can for example verify the args (which would not be in the uri anyway, but still something we should check).
There was a problem hiding this comment.
I added a luassert test that checks the value of ngx.var.request. It turns out that args are not sent when the url of the rule contains a path. Working on a fix.
There was a problem hiding this comment.
👍 I knew this was going to be a bit tricky to send everything correctly.
There was a problem hiding this comment.
I solved this, but it looks like I have a similar problem with POST bodies.
davidor
force-pushed
the
upstream-policy
branch
3 times, most recently
from
3cfaf8c
to
0a0b7bf
Compare
| url.host, { port = url.port }) | ||
|
|
||
| if url.path then | ||
| ngx.var.proxy_pass = format('%s://upstream%s?%s', url.scheme, url.path, ngx.var.args) |
There was a problem hiding this comment.
This is going to always append ?, right? Even though there are no args.
There was a problem hiding this comment.
I haved extracted this part of the code to its own method to make it clearer.
davidor
force-pushed
the
upstream-policy
branch
2 times, most recently
from
4474007
to
5711d3f
Compare
davidor
force-pushed
the
upstream-policy
branch
from
5711d3f
to
e4696e0
Compare
|
@mikz This is ready. I added asserts in the |
davidor
force-pushed
the
upstream-policy
branch
from
b81c112
to
f8db4b4
Compare
davidor
force-pushed
the
upstream-policy
branch
from
f8db4b4
to
110bbc5
Compare
This is a first version of the upstream policy. For now, it only allows to modify the host of a request based on a set of rules that act on its path, but we can add more functionality later.