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 2021/04/28 08:56:17 UTC
[GitHub] [apisix] nanamikon opened a new pull request #4152: feat:redirect plugin support regex
nanamikon opened a new pull request #4152:
URL: https://github.com/apache/apisix/pull/4152
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
Redirect plugin support regex, similar to the regex_uri in the proxy_rewrite_plugin
Refer to #3722 and #4140
### Pre-submission checklist:
* [1] Did you explain what problem does this PR solve? Or what new features have been added?
* [1] Have you added corresponding test cases?
* [1] Have you modified the corresponding document?
* [1] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix/tree/master#community) first**
--
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 #4152: feat: redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#issuecomment-829869316
> Thanks for review, how can I find this bad style before submit a pr? I have tried luacheck and learned from proxy-rewrite.lua, is there any other tools or docs that I can refer to?
You can refer to this doc: https://github.com/apache/apisix/blob/master/CODE_STYLE.md
--
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] nanamikon commented on a change in pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r622071554
##########
File path: apisix/plugins/redirect.lua
##########
@@ -35,10 +36,26 @@ local schema = {
properties = {
ret_code = {type = "integer", minimum = 200, default = 302},
uri = {type = "string", minLength = 2, pattern = reg},
+ regex_uri = {
+ description = "new uri that substitute from client uri " ..
Review comment:
get it
--
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 #4152: feat: redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r623036783
##########
File path: apisix/plugins/redirect.lua
##########
@@ -17,7 +17,9 @@
local core = require("apisix.core")
local tab_insert = table.insert
local tab_concat = table.concat
+local string_format = string.format
local re_gmatch = ngx.re.gmatch
+local re_sub = ngx.re.sub
Review comment:
Please remove extra spaces
##########
File path: t/plugin/redirect.t
##########
@@ -79,7 +79,68 @@ done
-=== TEST 3: add plugin with new uri: /test/add
+=== TEST 3: add plugin with new regex_uri: /test/1 redirect to http://test.com/1
Review comment:
Please add new test at the bottom of the file
--
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] nanamikon commented on a change in pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r622072795
##########
File path: apisix/plugins/redirect.lua
##########
@@ -35,10 +36,26 @@ local schema = {
properties = {
ret_code = {type = "integer", minimum = 200, default = 302},
uri = {type = "string", minLength = 2, pattern = reg},
+ regex_uri = {
+ description = "new uri that substitute from client uri " ..
+ "for upstream, lower priority than uri property",
+ type = "array",
+ maxItems = 2,
+ minItems = 2,
+ items = {
+ description = "regex uri",
+ type = "string",
+ }
+ },
http_to_https = {type = "boolean"},
},
oneOf = {
- {required = {"uri"}},
+ {
+ anyOf = {
Review comment:
I think so
--
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] nanamikon commented on pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
nanamikon commented on pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#issuecomment-828882746
I have fix it， but how to do with this problem (Semantic Pull Request — add a semantic PR title)
--
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] nanamikon commented on a change in pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r622070431
##########
File path: apisix/plugins/redirect.lua
##########
@@ -129,17 +161,35 @@ function _M.rewrite(conf, ctx)
end
end
- if uri and ret_code then
- local new_uri, err = concat_new_uri(uri, ctx)
- if not new_uri then
- core.log.error("failed to generate new uri by: ", uri, " error: ",
- err)
- return 500
+ if ret_code then
+ if uri then
+ local new_uri, err = concat_new_uri(uri, ctx)
+ if not new_uri then
+ local msg = "failed to generate new uri by: " .. uri .. err
+ core.log.error(msg)
+ return 500
+ end
+
+ core.response.set_header("Location", new_uri)
+ return ret_code
+ elseif regex_uri then
+ local new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
+ regex_uri[2], "jo")
+ if not new_uri then
+ local msg = "failed to substitute the uri " .. ctx.var.uri ..
Review comment:
ok
--
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] tokers commented on a change in pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r622017204
##########
File path: apisix/plugins/redirect.lua
##########
@@ -35,10 +36,26 @@ local schema = {
properties = {
ret_code = {type = "integer", minimum = 200, default = 302},
uri = {type = "string", minLength = 2, pattern = reg},
+ regex_uri = {
+ description = "new uri that substitute from client uri " ..
+ "for upstream, lower priority than uri property",
+ type = "array",
+ maxItems = 2,
+ minItems = 2,
+ items = {
+ description = "regex uri",
+ type = "string",
+ }
+ },
http_to_https = {type = "boolean"},
},
oneOf = {
- {required = {"uri"}},
+ {
+ anyOf = {
Review comment:
What about using `oneOf` so `uri` and `regex_uri` cannot co-exist.
##########
File path: apisix/plugins/redirect.lua
##########
@@ -129,17 +161,35 @@ function _M.rewrite(conf, ctx)
end
end
- if uri and ret_code then
- local new_uri, err = concat_new_uri(uri, ctx)
- if not new_uri then
- core.log.error("failed to generate new uri by: ", uri, " error: ",
- err)
- return 500
+ if ret_code then
+ if uri then
+ local new_uri, err = concat_new_uri(uri, ctx)
+ if not new_uri then
+ local msg = "failed to generate new uri by: " .. uri .. err
+ core.log.error(msg)
+ return 500
+ end
+
+ core.response.set_header("Location", new_uri)
+ return ret_code
+ elseif regex_uri then
+ local new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
+ regex_uri[2], "jo")
+ if not new_uri then
+ local msg = "failed to substitute the uri " .. ctx.var.uri ..
Review comment:
Use `string.format` to make it clearer.
##########
File path: apisix/plugins/redirect.lua
##########
@@ -129,17 +161,35 @@ function _M.rewrite(conf, ctx)
end
end
- if uri and ret_code then
- local new_uri, err = concat_new_uri(uri, ctx)
- if not new_uri then
- core.log.error("failed to generate new uri by: ", uri, " error: ",
- err)
- return 500
+ if ret_code then
+ if uri then
+ local new_uri, err = concat_new_uri(uri, ctx)
+ if not new_uri then
+ local msg = "failed to generate new uri by: " .. uri .. err
+ core.log.error(msg)
+ return 500
+ end
+
+ core.response.set_header("Location", new_uri)
+ return ret_code
+ elseif regex_uri then
+ local new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
+ regex_uri[2], "jo")
Review comment:
```suggestion
local new_uri, n, err = re_sub(ctx.var.uri, regex_uri[1],
regex_uri[2], "jo")
```
##########
File path: apisix/plugins/redirect.lua
##########
@@ -79,7 +96,21 @@ end
function _M.check_schema(conf)
- return core.schema.check(schema, conf)
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return false, err
+ end
+
+ if conf.regex_uri and #conf.regex_uri > 0 then
+ local _, _, err = re_sub("/fake_uri", conf.regex_uri[1],
+ conf.regex_uri[2], "jo")
+ if err then
+ return false, "invalid regex_uri(" .. conf.regex_uri[1] ..
+ ", " .. conf.regex_uri[2] .. "): " .. err
Review comment:
You can use `string.format` to make the error message composition clearer.
##########
File path: apisix/plugins/redirect.lua
##########
@@ -35,10 +36,26 @@ local schema = {
properties = {
ret_code = {type = "integer", minimum = 200, default = 302},
uri = {type = "string", minLength = 2, pattern = reg},
+ regex_uri = {
+ description = "new uri that substitute from client uri " ..
Review comment:
term "new uri" is not accurate to describe the `regex_uri` as it's actually a tuple that contains two elements.
--
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] nanamikon commented on a change in pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
nanamikon commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r622070688
##########
File path: apisix/plugins/redirect.lua
##########
@@ -79,7 +96,21 @@ end
function _M.check_schema(conf)
- return core.schema.check(schema, conf)
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return false, err
+ end
+
+ if conf.regex_uri and #conf.regex_uri > 0 then
+ local _, _, err = re_sub("/fake_uri", conf.regex_uri[1],
+ conf.regex_uri[2], "jo")
+ if err then
+ return false, "invalid regex_uri(" .. conf.regex_uri[1] ..
+ ", " .. conf.regex_uri[2] .. "): " .. err
Review comment:
ok
--
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 a change in pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r622872055
##########
File path: apisix/plugins/redirect.lua
##########
@@ -35,10 +37,22 @@ local schema = {
properties = {
ret_code = {type = "integer", minimum = 200, default = 302},
uri = {type = "string", minLength = 2, pattern = reg},
+ regex_uri = {
+ description = "params array for generating new uri that substitute from client uri " ..
+ "fist param is regular expression, second one is uri template",
Review comment:
Bad style, the description below should remain aligned with the top.
--
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] nanamikon edited a comment on pull request #4152: feat: redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
nanamikon edited a comment on pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#issuecomment-829309579
Thanks for review, how can I find this bad style before submit a pr? I have tried luacheck and learned from proxy-rewrite.lua, is there any other tools or docs that I can refer to?
--
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] nanamikon commented on pull request #4152: feat: redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
nanamikon commented on pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#issuecomment-829309579
Thanks for review, how can I find this bad style before submit a pr? I have tried luacheck and learned from proxy-rewrite.lua, is there any other tools that I can refer to?
--
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] juzhiyuan commented on a change in pull request #4152: feat: redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r627981772
##########
File path: docs/zh/latest/plugins/redirect.md
##########
@@ -29,6 +29,7 @@ URI 重定向插件。
| ------------- | ------- | ----------- | ------- | ---------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| http_to_https | boolean | 可选 | false | | 当设置为 `true` 并且请求是 http 时，会自动 301 重定向为 https，uri 保持不变 |
| uri | string | 可选 | | | 可以包含 Nginx 变量的 URI，例如：`/test/index.html`, `$uri/index.html`。你可以通过类似于 `$ {xxx}` 的方式引用变量，以避免产生歧义，例如：`${uri}foo/index.html`。若你需要保留 `$` 字符，那么使用如下格式：`/\$foo/index.html` |
+| regex_uri | array[string] | 可选 | | | 转发到上游的新 `uri` 地址, 使用正则表达式匹配来自客户端的uri，当匹配成功后使用模板替换发送重定向到客户端, 未匹配成功时将客户端请求的uri转发至上游。`uri`和`regex_uri`不可以同时存在。例如：["^/iresty/(.*)/(.*)/(.*)","/$1-$2-$3"] 第一个元素代表匹配来自客户端请求的uri正则表达式，第二个元素代表匹配成功后发送重定向到客户端的uri模板。 |
Review comment:
```suggestion
| regex_uri | array[string] | 可选 | | | 转发到上游的新 `uri` 地址, 使用正则表达式匹配来自客户端的 `uri`，当匹配成功后使用模板替换发送重定向到客户端, 未匹配成功时将客户端请求的 `uri` 转发至上游。`uri` 和 `regex_uri` 不可以同时存在。例如：["^/iresty/(.*)/(.*)/(.*)","/$1-$2-$3"] 第一个元素代表匹配来自客户端请求的 `uri` 正则表达式，第二个元素代表匹配成功后发送重定向到客户端的 `uri` 模板。 |
```
--
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] Yiyiyimu merged pull request #4152: feat: redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
Yiyiyimu merged pull request #4152:
URL: https://github.com/apache/apisix/pull/4152
--
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] Gary-Airwallex commented on pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
Gary-Airwallex commented on pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#issuecomment-828989602
> I have fixed it， but how to do with this problem (Semantic Pull Request — add a semantic PR title)
Could it be a missing space between `feat:` and `redirect`? LOL
--
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 a change in pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r622890894
##########
File path: apisix/plugins/redirect.lua
##########
@@ -129,17 +159,34 @@ function _M.rewrite(conf, ctx)
end
end
- if uri and ret_code then
- local new_uri, err = concat_new_uri(uri, ctx)
- if not new_uri then
- core.log.error("failed to generate new uri by: ", uri, " error: ",
- err)
- return 500
+ if ret_code then
+ if uri then
+ local new_uri, err = concat_new_uri(uri, ctx)
+ if not new_uri then
+ local msg = "failed to generate new uri by: " .. uri .. err
+ core.log.error(msg)
Review comment:
I think it's better that way.
```suggestion
core.log.error("failed to generate new uri by: " .. uri .. err)
```
--
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] tokers commented on pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
tokers commented on pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#issuecomment-828858914
@nanamikon CI failed, please check it out.
--
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] nanamikon edited a comment on pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
nanamikon edited a comment on pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#issuecomment-828882746
I have fixed it， but how to do with this problem (Semantic Pull Request — add a semantic PR title)
--
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 a change in pull request #4152: feat:redirect plugin support regex
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #4152:
URL: https://github.com/apache/apisix/pull/4152#discussion_r622887617
##########
File path: apisix/plugins/redirect.lua
##########
@@ -79,7 +93,22 @@ end
function _M.check_schema(conf)
- return core.schema.check(schema, conf)
+ local ok, err = core.schema.check(schema, conf)
+ if not ok then
+ return false, err
+ end
+
+ if conf.regex_uri and #conf.regex_uri > 0 then
+ local _, _, err = re_sub("/fake_uri", conf.regex_uri[1],
+ conf.regex_uri[2], "jo")
Review comment:
Ditto, please fix similar problems.
--
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