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 2022/10/25 02:11:58 UTC
[GitHub] [apisix] soulbird opened a new pull request, #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
soulbird opened a new pull request, #8162:
URL: https://github.com/apache/apisix/pull/8162
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes #7852 #8087
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] 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)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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 #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1005287893
##########
apisix/plugin.lua:
##########
@@ -415,6 +415,12 @@ local function meta_filter(ctx, plugin_name, plugin_conf)
return true
end
+ local match_cache_key =
+ ctx.conf_type .. "#" .. plugin_name .. "#meta_filter_matched"
Review Comment:
Does this key work with multiple global_rules? They have the same conf_type.
--
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] starsz commented on a diff in pull request #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
starsz commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1004280692
##########
apisix/plugin.lua:
##########
@@ -433,6 +438,8 @@ local function meta_filter(ctx, plugin_name, plugin_conf)
" plugin_name: ", plugin_name)
return true
end
+
+ ctx[match_cache_key] = ok
Review Comment:
I think we can't cache the result of the filter. Since we have a different phase for every plugin like "rewrite", "access" e.g.
--
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 #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1006459258
##########
t/plugin/plugin.t:
##########
@@ -28,6 +28,7 @@ add_block_preprocessor(sub {
$block;
});
+repeat_each(1);
Review Comment:
https://github.com/apache/apisix/blob/e523132f35fe2eb7e1d06990d037f18f89c9dcba/t/APISIX.pm#L23
We already set it by default?
--
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 #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1006443008
##########
apisix/plugin.lua:
##########
@@ -415,6 +415,12 @@ local function meta_filter(ctx, plugin_name, plugin_conf)
return true
end
+ local match_cache_key =
+ ctx.conf_type .. "#" .. plugin_name .. "#meta_filter_matched"
Review Comment:
There is indeed a problem, I adjusted `match_cache_key`, added some test case：https://github.com/apache/apisix/pull/8162/files#diff-29e2373df60fd8fc082317d17e636b6fa17c60459c1a691c5838f36b31cc486aR532-R567
--
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 #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #8162:
URL: https://github.com/apache/apisix/pull/8162
--
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 pull request #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #8162:
URL: https://github.com/apache/apisix/pull/8162#issuecomment-1293269467
@soulbird
> Fixes https://github.com/apache/apisix/issues/7852 https://github.com/apache/apisix/issues/8087
Need to use double fixes
--
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 #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1005155313
##########
apisix/plugin.lua:
##########
@@ -433,6 +438,8 @@ local function meta_filter(ctx, plugin_name, plugin_conf)
" plugin_name: ", plugin_name)
return true
end
+
+ ctx[match_cache_key] = ok
Review Comment:
`filter` works better at the plugin level and is executed at each phase, which will increase the difficulty for users to use.
--
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] tokers commented on a diff in pull request #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
tokers commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1004243260
##########
apisix/plugin.lua:
##########
@@ -415,6 +415,11 @@ local function meta_filter(ctx, plugin_name, plugin_conf)
return true
end
+ local match_cache_key = ctx.conf_type .. plugin_name .. "meta_filter_matched"
Review Comment:
Use some separator like `#` before `meta_filter_matched` ?
##########
apisix/plugin.lua:
##########
@@ -920,6 +925,8 @@ function _M.run_plugin(phase, plugins, api_ctx)
return api_ctx
end
+ --local matched = meta_filter(ctx, name, plugin_conf)
Review Comment:
Remove this?
##########
t/plugin/plugin.t:
##########
@@ -525,3 +525,170 @@ passed
GET /hello1?version=v4
--- response_headers
x-api-version: v4
+
+
+
+=== TEST 19: use _meta.filter in response-rewrite plugin
+--- 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": {
+ "response-rewrite": {
+ "_meta": {
+ "filter": [
+ ["upstream_status", "~=", 200]
+ ]
+ },
+ "headers": {
+ "set": {
+ "test-header": "error"
+ }
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 20: upstream_status = 502, enable response-rewrite plugin
+--- request
+GET /specific_status
+--- more_headers
+x-test-upstream-status: 502
+--- response_headers
+test-header: error
+--- error_code: 502
+
+
+
+=== TEST 21: upstream_status = 200, disable response-rewrite plugin
+--- request
+GET /hello
+
Review Comment:
Should check if the `x-test-upstream-status` doesn't exist.
--
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 #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1005452509
##########
apisix/plugin.lua:
##########
@@ -415,6 +415,12 @@ local function meta_filter(ctx, plugin_name, plugin_conf)
return true
end
+ local match_cache_key =
+ ctx.conf_type .. "#" .. plugin_name .. "#meta_filter_matched"
Review Comment:
@soulbird
Multiple global rules will share the same conf_type
https://github.com/apache/apisix/blob/e2d0f0b16bb157412684a38b129d12acb4628d39/apisix/plugin.lua#L1003
Will this affect the cache of eval result?
--
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 pull request #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
soulbird commented on PR #8162:
URL: https://github.com/apache/apisix/pull/8162#issuecomment-1294286943
> @soulbird
>
> > Fixes #7852 #8087
>
> Need to use double fixes
My mistake, these two don't seem to be the same problem.
--
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 #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1007513144
##########
t/plugin/plugin.t:
##########
@@ -520,8 +520,220 @@ passed
-=== TEST 18: hit route: run proxy-rewrite plugin
+=== TEST 18: hit route: run global proxy-rewrite plugin
--- request
GET /hello1?version=v4
--- response_headers
x-api-version: v4
+
+
+
+=== TEST 19: different global_rules with the same plugin will not use the same meta.filter cache
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/global_rules/3',
+ ngx.HTTP_PUT,
+ {
+ plugins = {
+ ["proxy-rewrite"] = {
+ _meta = {
+ filter = {
+ {"arg_version", "==", "v5"}
+ }
+ },
+ uri = "/echo",
+ headers = {
+ ["X-Api-Version"] = "v5"
+ }
+ }
+ }
+ }
+ )
+ if code >= 300 then
+ ngx.print(body)
+ else
+ ngx.say(body)
+ end
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 20: hit global_rules which has the same plugin with different meta.filter
+--- pipelined_requests eval
+["GET /hello1?version=v4", "GET /hello1?version=v5"]
+--- response_headers eval
+["x-api-version: v4", "x-api-version: v5"]
+
+
+
+=== TEST 21: use _meta.filter in response-rewrite plugin
+--- 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": {
+ "response-rewrite": {
+ "_meta": {
+ "filter": [
+ ["upstream_status", "~=", 200]
+ ]
+ },
+ "headers": {
+ "set": {
+ "test-header": "error"
+ }
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 22: upstream_status = 502, enable response-rewrite plugin
+--- request
+GET /specific_status
+--- more_headers
+x-test-upstream-status: 502
+--- response_headers
+test-header: error
+--- error_code: 502
+
+
+
+=== TEST 23: upstream_status = 200, disable response-rewrite plugin
+--- request
+GET /hello
+--- response_headers
+!test-header
+
+
+
+=== TEST 24: use _meta.filter in response-rewrite plugin
+--- 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": {
+ "headers": {
+ "foo-age": "$arg_age"
+ }
+ },
+ "response-rewrite": {
+ "_meta": {
+ "filter": [
+ ["http_foo_age", "==", "18"]
+ ]
+ },
+ "status_code": 403
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 25: proxy-rewrite plugin will set $http_foo_age, response-rewrite plugin return 403
+--- request
+GET /hello?age=18
+--- error_code: 403
+
+
+
+=== TEST 26: response-rewrite plugin disable, return 200
+--- request
+GET /hello
Review Comment:
By default, the 200 status code will be checked, so there is no need to repeatedly write.
--
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 #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1005384573
##########
apisix/plugin.lua:
##########
@@ -415,6 +415,12 @@ local function meta_filter(ctx, plugin_name, plugin_conf)
return true
end
+ local match_cache_key =
+ ctx.conf_type .. "#" .. plugin_name .. "#meta_filter_matched"
Review Comment:
Do you mean the scene where the `con_type` is the same and the `plugin_name` is different? I think my test case already contains. The `conf_type` is `route`.
--
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 #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
soulbird commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1005154323
##########
t/plugin/plugin.t:
##########
@@ -525,3 +525,170 @@ passed
GET /hello1?version=v4
--- response_headers
x-api-version: v4
+
+
+
+=== TEST 19: use _meta.filter in response-rewrite plugin
+--- 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": {
+ "response-rewrite": {
+ "_meta": {
+ "filter": [
+ ["upstream_status", "~=", 200]
+ ]
+ },
+ "headers": {
+ "set": {
+ "test-header": "error"
+ }
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 20: upstream_status = 502, enable response-rewrite plugin
+--- request
+GET /specific_status
+--- more_headers
+x-test-upstream-status: 502
+--- response_headers
+test-header: error
+--- error_code: 502
+
+
+
+=== TEST 21: upstream_status = 200, disable response-rewrite plugin
+--- request
+GET /hello
+
Review Comment:
It doesn't make sense to check this, it's better to check if `test-header` exists.
--
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] starsz commented on a diff in pull request #8162: fix: meta.filter in plugin doesn't work in response-rewrite plugin
Posted by GitBox <gi...@apache.org>.
starsz commented on code in PR #8162:
URL: https://github.com/apache/apisix/pull/8162#discussion_r1007505508
##########
t/plugin/plugin.t:
##########
@@ -520,8 +520,220 @@ passed
-=== TEST 18: hit route: run proxy-rewrite plugin
+=== TEST 18: hit route: run global proxy-rewrite plugin
--- request
GET /hello1?version=v4
--- response_headers
x-api-version: v4
+
+
+
+=== TEST 19: different global_rules with the same plugin will not use the same meta.filter cache
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/global_rules/3',
+ ngx.HTTP_PUT,
+ {
+ plugins = {
+ ["proxy-rewrite"] = {
+ _meta = {
+ filter = {
+ {"arg_version", "==", "v5"}
+ }
+ },
+ uri = "/echo",
+ headers = {
+ ["X-Api-Version"] = "v5"
+ }
+ }
+ }
+ }
+ )
+ if code >= 300 then
+ ngx.print(body)
+ else
+ ngx.say(body)
+ end
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 20: hit global_rules which has the same plugin with different meta.filter
+--- pipelined_requests eval
+["GET /hello1?version=v4", "GET /hello1?version=v5"]
+--- response_headers eval
+["x-api-version: v4", "x-api-version: v5"]
+
+
+
+=== TEST 21: use _meta.filter in response-rewrite plugin
+--- 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": {
+ "response-rewrite": {
+ "_meta": {
+ "filter": [
+ ["upstream_status", "~=", 200]
+ ]
+ },
+ "headers": {
+ "set": {
+ "test-header": "error"
+ }
+ }
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 22: upstream_status = 502, enable response-rewrite plugin
+--- request
+GET /specific_status
+--- more_headers
+x-test-upstream-status: 502
+--- response_headers
+test-header: error
+--- error_code: 502
+
+
+
+=== TEST 23: upstream_status = 200, disable response-rewrite plugin
+--- request
+GET /hello
+--- response_headers
+!test-header
+
+
+
+=== TEST 24: use _meta.filter in response-rewrite plugin
+--- 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": {
+ "headers": {
+ "foo-age": "$arg_age"
+ }
+ },
+ "response-rewrite": {
+ "_meta": {
+ "filter": [
+ ["http_foo_age", "==", "18"]
+ ]
+ },
+ "status_code": 403
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/*"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 25: proxy-rewrite plugin will set $http_foo_age, response-rewrite plugin return 403
+--- request
+GET /hello?age=18
+--- error_code: 403
+
+
+
+=== TEST 26: response-rewrite plugin disable, return 200
+--- request
+GET /hello
Review Comment:
Missing any check ？
--
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