You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org> on 2023/03/20 06:31:45 UTC
[GitHub] [apisix] monkeyDluffy6017 opened a new pull request, #9112: feat: support variable when rewrite header in proxy rewrite plugin
monkeyDluffy6017 opened a new pull request, #9112:
URL: https://github.com/apache/apisix/pull/9112
### Description
Support for referencing the match result of the `regex_uri` when rewrite header in the proxy-rewrite plugin, and the match result can be referenced as a variable by other plugins too.
Nginx:
```
location ~* ^/aaa/(.*)$ {
...
proxy_set_header HDR_TEST $1;
}
```
Apisix:
```
"plugins": {
"proxy-rewrite": {
"regex_uri": ["^/test/(.*)/(.*)/(.*)", "/$1_$2_$3"],
"headers": {
"X-Api:Version": "v2-$1"
}
}
},
```
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [x] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145055981
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
+ core.log.error("match error in proxy-rewrite plugin, please check: ", err)
+ return
Review Comment:
I prefer 500, tt's an unexpected condition
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145055981
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
+ core.log.error("match error in proxy-rewrite plugin, please check: ", err)
+ return
Review Comment:
I prefer 500, it's an unexpected condition
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1142838545
##########
apisix/core/utils.lua:
##########
@@ -335,4 +335,45 @@ end
_M.resolve_var = resolve_var
+local resolve_var_with_captures
+do
+ local _captures
+ -- escape is not supported very well, like there si a redundant '\' after escape "$1"
Review Comment:
```suggestion
-- escape is not supported very well, like there is a redundant '\' after escape "$1"
```
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
+ core.log.error("match error in proxy-rewrite plugin, please check: ", err)
+ return
Review Comment:
Do we need to return a 503 for this error?
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
Review Comment:
We can merge the two conditions above?
##########
apisix/core/utils.lua:
##########
@@ -335,4 +335,45 @@ end
_M.resolve_var = resolve_var
+local resolve_var_with_captures
+do
+ local _captures
+ -- escape is not supported very well, like there si a redundant '\' after escape "$1"
+ local pat = [[ (?<! \\) \$ \{? (\d+) \}? ]]
+
+ local function resolve(m)
+ local v = _captures[tonumber(m[1])]
Review Comment:
Do we need to support capture group name?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander merged PR #9112:
URL: https://github.com/apache/apisix/pull/9112
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145582982
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
Review Comment:
Done
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
Review Comment:
Done
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "spacewander (via GitHub)" <gi...@apache.org>.
spacewander commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145566910
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
Review Comment:
See https://github.com/apache/apisix/pull/9112#discussion_r1145538773
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145761174
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -41,6 +42,10 @@ local lrucache = core.lrucache.new({
type = "plugin",
})
+core.ctx.register_var("proxy_rewrite_regex_uri_captures", function(ctx)
+ return ctx.proxy_rewrite_regex_uri_captures
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "leslie-tsang (via GitHub)" <gi...@apache.org>.
leslie-tsang commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1144130026
##########
apisix/core/utils.lua:
##########
@@ -335,4 +335,45 @@ end
_M.resolve_var = resolve_var
+local resolve_var_with_captures
+do
+ local _captures
+ -- escape is not supported very well, like there si a redundant '\' after escape "$1"
+ local pat = [[ (?<! \\) \$ \{? (\d+) \}? ]]
+
+ local function resolve(m)
+ local v = _captures[tonumber(m[1])]
Review Comment:
The current implementation meets the needs, seems no need to support the capture group name. :)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] leslie-tsang commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "leslie-tsang (via GitHub)" <gi...@apache.org>.
leslie-tsang commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145538773
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
Review Comment:
```suggestion
if not m and err then
```
Would be better ? Seems no necessary to separate them.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145061111
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
Review Comment:
how? use the `re_match` first, then use the match result to replace?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145582889
##########
apisix/core/utils.lua:
##########
@@ -335,4 +335,45 @@ end
_M.resolve_var = resolve_var
+local resolve_var_with_captures
+do
+ local _captures
+ -- escape is not supported very well, like there si a redundant '\' after escape "$1"
Review Comment:
Done
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] soulbird commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "soulbird (via GitHub)" <gi...@apache.org>.
soulbird commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145759545
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -41,6 +42,10 @@ local lrucache = core.lrucache.new({
type = "plugin",
})
+core.ctx.register_var("proxy_rewrite_regex_uri_captures", function(ctx)
+ return ctx.proxy_rewrite_regex_uri_captures
Review Comment:
Why need this?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145760279
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -41,6 +42,10 @@ local lrucache = core.lrucache.new({
type = "plugin",
})
+core.ctx.register_var("proxy_rewrite_regex_uri_captures", function(ctx)
+ return ctx.proxy_rewrite_regex_uri_captures
Review Comment:
It's for other plugin cc @leslie-tsang
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #9112: feat: support variable when rewrite header in proxy rewrite plugin
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #9112:
URL: https://github.com/apache/apisix/pull/9112#discussion_r1145055981
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -278,15 +284,24 @@ function _M.rewrite(conf, ctx)
local uri, _, err = re_sub(upstream_uri, conf.regex_uri[1],
conf.regex_uri[2], "jo")
- if uri then
- upstream_uri = uri
- else
+ if not uri then
local msg = "failed to substitute the uri " .. ctx.var.uri ..
" (" .. conf.regex_uri[1] .. ") with " ..
conf.regex_uri[2] .. " : " .. err
core.log.error(msg)
return 500, {message = msg}
end
+
+ local m, err = re_match(upstream_uri, conf.regex_uri[1], "jo")
+ if not m then
+ if err then
+ core.log.error("match error in proxy-rewrite plugin, please check: ", err)
+ return
Review Comment:
I prefer 500
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org