You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/12/27 16:53:16 UTC
[GitHub] [apisix] BFergerson opened a new pull request #3139: ctx.var in upstream_uri on proxy-rewrite plugin
BFergerson opened a new pull request #3139:
URL: https://github.com/apache/apisix/pull/3139
### What this PR does / why we need it:
Adds the ability to use `ctx.var` in `upstream_uri` on the proxy-rewrite plugin.
Fixes #3133
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] BFergerson commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
BFergerson commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r551056690
##########
File path: t/plugin/proxy-rewrite.t
##########
@@ -1249,3 +1249,41 @@ version: nginx_var_does_not_exist
x-real-ip: 127.0.0.1
--- no_error_log
[error]
+
+
+
+=== TEST 42: set route(rewrite uri based on ctx.var)
Review comment:
Apologies for the delay. Thanks for the help with understanding how tests work.
I've added some information to the documentation which I think would have been enough for me to know how to setup this test myself. Let me know if it's acceptable.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] BFergerson commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
BFergerson commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r549708890
##########
File path: t/plugin/proxy-rewrite.t
##########
@@ -1249,3 +1249,41 @@ version: nginx_var_does_not_exist
x-real-ip: 127.0.0.1
--- no_error_log
[error]
+
+
+
+=== TEST 42: set route(rewrite uri based on ctx.var)
Review comment:
I think this is the end of my understanding of how to contribute to APISIX. I never heard of Lua before this project so I'm relying on the documentation and contributors of this project to help me understand what to do.
Can you point to other tests (or documentation) which does everything I need to do in my test?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] BFergerson commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
BFergerson commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r551093024
##########
File path: doc/plugin-develop.md
##########
@@ -285,6 +285,10 @@ When we request __/t__, which config in the configuration file, the Nginx will c
"__no_error_log__" means to check the "__error.log__" of Nginx. There must be no ERROR level record. The log files for the unit test
are located in the following folder: 't/servroot/logs'.
+The above test case represents a simple scenario. Most scenarios will require multiple tests to validate. To do this, create multiple tests `=== TEST 1`, `=== TEST 2`, and so on. These tests will be executed sequentially, allowing you to break down scenarios into a sequence of atomic steps.
Review comment:
```suggestion
The above test case represents a simple scenario. Most scenarios will require multiple steps to validate. To do this, create multiple tests `=== TEST 1`, `=== TEST 2`, and so on. These tests will be executed sequentially, allowing you to break down scenarios into a sequence of atomic steps.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] BFergerson commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
BFergerson commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r549770858
##########
File path: t/plugin/proxy-rewrite.t
##########
@@ -1249,3 +1249,41 @@ version: nginx_var_does_not_exist
x-real-ip: 127.0.0.1
--- no_error_log
[error]
+
+
+
+=== TEST 42: set route(rewrite uri based on ctx.var)
Review comment:
Thanks for the help. I'm still not sure how your tests cover anything more than what my test covers though.
Based on your tests it appears tests are executed sequentially and share a context between them. The first test creates a route with the `proxy-rewrite` plugin, rewriting the uri to `$arg_new_uri`. This appears to be the same thing my test covers with just a different variable to subsitute.
Your second test then appears to continue work from the first test and now hits `/test?new_uri=hello` and expects `hello world` as a response. That doesn't make sense to me. Where does `hello world` come from and how does hitting `/test?new_uri=hello` verify that `proxy-rewrite` has worked correctly? There should be some way to set the value of `$arg_new_uri` and verify the resulting value of uri after `proxy-rewrite` is equiavlent. Also, if `$arg_new_uri` is really the value of `new_uri` that is a bit confusing. I wouldn't have guessed adding `arg_` makes a variable come from url get params.
Apologies if I'm making this more difficult than it needs to be. I've tried reading the [documention](https://github.com/apache/apisix/blob/master/doc/plugin-develop.md#write-test-case) but it doesn't really explain to me how testing works.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming commented on pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#issuecomment-754339371
ping @membphis
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on pull request #3139: ctx.var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#issuecomment-751547606
We need test cases to cover this feature.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#issuecomment-751641383
You can write test like
https://github.com/apache/apisix/blob/45e909a0b122b8cda5a75e95adcfaf62378f7a30/t/plugin/proxy-rewrite.t#L477-L526
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r549716648
##########
File path: t/plugin/proxy-rewrite.t
##########
@@ -1249,3 +1249,41 @@ version: nginx_var_does_not_exist
x-real-ip: 127.0.0.1
--- no_error_log
[error]
+
+
+
+=== TEST 42: set route(rewrite uri based on ctx.var)
Review comment:
I write a full test case example, you can make a try.
^_^
```
=== TEST 42: set route(rewrite uri based on ctx.var)
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/routes/1',
ngx.HTTP_PUT,
[[{
"plugins": {
"proxy-rewrite": {
"uri": "/$arg_new_uri"
}
},
"upstream": {
"nodes": {
"127.0.0.1:1980": 1
},
"type": "roundrobin"
},
"uri": "/test"
}]]
)
if code >= 300 then
ngx.status = code
end
ngx.say(body)
}
}
--- request
GET /t
--- response_body
passed
--- no_error_log
[error]
=== TEST 43: hit route(upstream uri: should be /hello)
--- request
GET /test?new_uri=hello
--- response_body
hello world
--- no_error_log
[error]
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] moonming merged pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
moonming merged pull request #3139:
URL: https://github.com/apache/apisix/pull/3139
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r549698258
##########
File path: t/plugin/proxy-rewrite.t
##########
@@ -1249,3 +1249,41 @@ version: nginx_var_does_not_exist
x-real-ip: 127.0.0.1
--- no_error_log
[error]
+
+
+
+=== TEST 42: set route(rewrite uri based on ctx.var)
Review comment:
only set route is not enough. we need to make a test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] BFergerson commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
BFergerson commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r549708890
##########
File path: t/plugin/proxy-rewrite.t
##########
@@ -1249,3 +1249,41 @@ version: nginx_var_does_not_exist
x-real-ip: 127.0.0.1
--- no_error_log
[error]
+
+
+
+=== TEST 42: set route(rewrite uri based on ctx.var)
Review comment:
Unfortunately, I think this is the end of my understanding of how to contribute to APISIX. I never heard of Lua before this project so I'm relying on the documentation and contributors of this project to help me understand what to do.
Can you point to other tests (or documentation) which does everything I need to do in my test?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on pull request #3139: ctx.var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#issuecomment-751547564
Need to add semantic "PR" title, for example: `feat: ctx.var in upstream_uri on proxy-rewrite plugin`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r551079516
##########
File path: doc/plugin-develop.md
##########
@@ -285,6 +285,10 @@ When we request __/t__, which config in the configuration file, the Nginx will c
"__no_error_log__" means to check the "__error.log__" of Nginx. There must be no ERROR level record. The log files for the unit test
are located in the following folder: 't/servroot/logs'.
+The above test case represents a simple scenario. Most scenarios will require multiple tests to validate. To do this, create multiple tests `=== TEST 1`, `=== TEST 2`, and so on. These tests will be executed sequentially, allowing you to break down scenarios into a sequence of atomic steps.
Review comment:
"require multiple tests" should be "require multiple steps"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #3139: ctx.var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#issuecomment-751637373
Please merge master to make CI pass.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r550227045
##########
File path: t/plugin/proxy-rewrite.t
##########
@@ -1249,3 +1249,41 @@ version: nginx_var_does_not_exist
x-real-ip: 127.0.0.1
--- no_error_log
[error]
+
+
+
+=== TEST 42: set route(rewrite uri based on ctx.var)
Review comment:
'arg_****': argument name in the request line
nginx variable link: http://nginx.org/en/docs/http/ngx_http_core_module.html#var_arg_
`$arg_new_uri`, it means to fetch the value from query string which name is `new_uri`.
1. the current request is `/test?new_uri=hello`, so the `$arg_new_uri` should equal `hello`
2. `"uri": "/$arg_new_uri"` -> `"uri": "/hello"`
3. response body is 'hello world' if the request is `/hello`: https://github.com/apache/apisix/blob/master/t/lib/server.lua#L36
4. check the response body
```
--- response_body
hello world
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] BFergerson commented on a change in pull request #3139: feat: support var in upstream_uri on proxy-rewrite plugin
Posted by GitBox <gi...@apache.org>.
BFergerson commented on a change in pull request #3139:
URL: https://github.com/apache/apisix/pull/3139#discussion_r549770858
##########
File path: t/plugin/proxy-rewrite.t
##########
@@ -1249,3 +1249,41 @@ version: nginx_var_does_not_exist
x-real-ip: 127.0.0.1
--- no_error_log
[error]
+
+
+
+=== TEST 42: set route(rewrite uri based on ctx.var)
Review comment:
Thanks for the help. I'm still not sure how your tests cover anything more than what my test covers though.
Based on your tests it appears tests are executed sequentially and share a context between them. The first test creates a route with the `proxy-rewrite` plugin rewriting the uri to `$arg_new_uri`. This appears to be the same thing my test covers with just a different variable to subsitute.
Your second test then appears to continue work from the first test and now hits `/test?new_uri=hello` and expects `hello world` as a response. That doesn't make sense to me. Where does `hello world` come from and how does hitting `/test?new_uri=hello` verify that `proxy-rewrite` has worked correctly? There should be some way to set the value of `$arg_new_uri` and verify the resulting value of uri after `proxy-rewrite` is equiavlent. Also, if `$arg_new_uri` is really the value of `new_uri` that is a bit confusing. I wouldn't have guessed adding `arg_` makes a variable come from url get params.
Apologies if I'm making this more difficult than it needs to be. I've tried reading the [documention](https://github.com/apache/apisix/blob/master/doc/plugin-develop.md#write-test-case) but it doesn't really explain to me how testing works.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org