spacewander commented on a change in pull request #3363:
URL: https://github.com/apache/apisix/pull/3363#discussion_r563416326
##########
File path: apisix/plugins/fault-injection.lua
##########
@@ -15,13 +15,66 @@
-- limitations under the License.
--
local core = require("apisix.core")
+local expr = require("resty.expr.v1")
local sleep = core.sleep
local random = math.random
+local ipairs = ipairs
local plugin_name = "fault-injection"
+local vars_schema = {
Review comment:
I think we need to remove the `vars_schema`.
1. we can do the check better with `expr.new` when checking the schema.
2. when `not`/`or` is supported directly in the expr, this schema is
outdated.
3. this schema contains many incorrect hardcoded limitations, like the
`maxLength` of operator is `2` (not enough for `has`), and an expression may
contain 4 elements if `!` is used, etc.
##########
File path: doc/plugins/fault-injection.md
##########
@@ -30,11 +30,43 @@ Fault injection plugin, this plugin can be used with other
plugins and will be e
| abort.http_status | integer | required | | [200, ...] |
user-specified http code returned to the client. |
| abort.body | string | optional | | | response
data returned to the client. Nginx variable can be used inside, like `client
addr: $remote_addr\n` |
| abort.percentage | integer | optional | | [0, 100] |
percentage of requests to be aborted. |
+| abort.vars | array[] | optional | | | The rules
for executing fault injection will only be executed when the rules are matched.
`vars` is a list consisting of one or more [[var, operator, val]] elements,
like this: [[[var, operator, val],[var, operator, val]], ...]. For example:
[[["arg_name", "==", "json"]]], indicating that the current request parameter
name is json. The var here is consistent with the naming of Nginx internal
variables, so request_uri, host, etc. can also be used; for the operator part,
the currently supported operators are ==, ~=, ~~, >, <, in, has and !. For
specific usage of operators, please see the `operator-list` part of
[lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). |
| delay.duration | number | required | | | delay
time (can be decimal). |
| delay.percentage | integer | optional | | [0, 100] |
percentage of requests to be delayed. |
+| delay.vars | array[] | optional | | | Execute
the request delay rule, and the request will be delayed only after the rule
matches. `vars` is a list consisting of one or more [[var, operator, val]]
elements, like this: [[[var, operator, val],[var, operator, val]], ...]. For
example: [[["arg_name", "==", "json"]]], indicating that the current request
parameter name is json. The var here is consistent with the naming of Nginx
internal variables, so request_uri, host, etc. can also be used; for the
operator part, the currently supported operators are ==, ~=, ~~, >, <, in, has
and !. For specific usage of operators, please see the `operator-list` part of
[lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). |
Note: One of `abort` and `delay` must be specified.
+`vars` is composed of a three-level array structure, as shown below:
+
+```json
+array[
+ array[
+ array[]
+ ],
+ array[
+ array[]
+ ],
+ ......
+]
+```
+
+It can implement the `and/or` relationship between rules flexibly, such as the
following three expressions:
+
+```json
+[
+ [
+ [ "arg_name","==","jack" ],
+ [ "arg_age","==",18 ]
+ ],
+ [
+ [ "arg_name2","==","allen" ]
+ ]
+]
+```
+
+Indicates that the relationship between the first two expressions is `and`,
and the relationship between the first two expressions and the third expression
is `or`.
Review comment:
What `Indicates`? There is a grammar error in the sentence.
##########
File path: apisix/plugins/fault-injection.lua
##########
@@ -77,11 +151,30 @@ end
function _M.rewrite(conf, ctx)
core.log.info("plugin rewrite phase, conf: ", core.json.delay_encode(conf))
- if conf.delay and sample_hit(conf.delay.percentage) then
+ local err
+ local abort_vars = true
+ if conf.abort and conf.abort.vars then
+ abort_vars, err = vars_match(conf.abort.vars, ctx)
+ if err then
+ return 500, err
+ end
+ end
+ core.log.info("abort_vars: ", abort_vars)
+
+ local delay_vars = true
+ if conf.delay and conf.delay.vars then
+ delay_vars, err = vars_match(conf.delay.vars, ctx)
+ if err then
+ return 500, err
Review comment:
Ditto
##########
File path: doc/plugins/fault-injection.md
##########
@@ -30,11 +30,43 @@ Fault injection plugin, this plugin can be used with other
plugins and will be e
| abort.http_status | integer | required | | [200, ...] |
user-specified http code returned to the client. |
| abort.body | string | optional | | | response
data returned to the client. Nginx variable can be used inside, like `client
addr: $remote_addr\n` |
| abort.percentage | integer | optional | | [0, 100] |
percentage of requests to be aborted. |
+| abort.vars | array[] | optional | | | The rules
for executing fault injection will only be executed when the rules are matched.
`vars` is a list consisting of one or more [[var, operator, val]] elements,
like this: [[[var, operator, val],[var, operator, val]], ...]. For example:
[[["arg_name", "==", "json"]]], indicating that the current request parameter
name is json. The var here is consistent with the naming of Nginx internal
variables, so request_uri, host, etc. can also be used; for the operator part,
the currently supported operators are ==, ~=, ~~, >, <, in, has and !. For
specific usage of operators, please see the `operator-list` part of
[lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). |
Review comment:
I think the description can be simpler. It has been repeated for too
many times.
We can say it is a list of `expression`, which is from the lua-resty-expr.
That is all.
##########
File path: apisix/plugins/fault-injection.lua
##########
@@ -64,6 +119,25 @@ local function sample_hit(percentage)
end
+local function vars_match(vars, ctx)
+ local match_result
+ for _, var in ipairs(vars) do
+ local expr, err = expr.new(var)
+ if err then
+ core.log.error("vars expression does not match: ", err)
Review comment:
Should be `failed to create vars expression`, we haven't matched it yet.
##########
File path: doc/plugins/fault-injection.md
##########
@@ -30,11 +30,43 @@ Fault injection plugin, this plugin can be used with other
plugins and will be e
| abort.http_status | integer | required | | [200, ...] |
user-specified http code returned to the client. |
| abort.body | string | optional | | | response
data returned to the client. Nginx variable can be used inside, like `client
addr: $remote_addr\n` |
| abort.percentage | integer | optional | | [0, 100] |
percentage of requests to be aborted. |
+| abort.vars | array[] | optional | | | The rules
for executing fault injection will only be executed when the rules are matched.
`vars` is a list consisting of one or more [[var, operator, val]] elements,
like this: [[[var, operator, val],[var, operator, val]], ...]. For example:
[[["arg_name", "==", "json"]]], indicating that the current request parameter
name is json. The var here is consistent with the naming of Nginx internal
variables, so request_uri, host, etc. can also be used; for the operator part,
the currently supported operators are ==, ~=, ~~, >, <, in, has and !. For
specific usage of operators, please see the `operator-list` part of
[lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). |
| delay.duration | number | required | | | delay
time (can be decimal). |
| delay.percentage | integer | optional | | [0, 100] |
percentage of requests to be delayed. |
+| delay.vars | array[] | optional | | | Execute
the request delay rule, and the request will be delayed only after the rule
matches. `vars` is a list consisting of one or more [[var, operator, val]]
elements, like this: [[[var, operator, val],[var, operator, val]], ...]. For
example: [[["arg_name", "==", "json"]]], indicating that the current request
parameter name is json. The var here is consistent with the naming of Nginx
internal variables, so request_uri, host, etc. can also be used; for the
operator part, the currently supported operators are ==, ~=, ~~, >, <, in, has
and !. For specific usage of operators, please see the `operator-list` part of
[lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). |
Note: One of `abort` and `delay` must be specified.
+`vars` is composed of a three-level array structure, as shown below:
Review comment:
The `vars` is a list of `expression` which is from the lua-resty-expr.
##########
File path: doc/plugins/fault-injection.md
##########
@@ -30,11 +30,43 @@ Fault injection plugin, this plugin can be used with other
plugins and will be e
| abort.http_status | integer | required | | [200, ...] |
user-specified http code returned to the client. |
| abort.body | string | optional | | | response
data returned to the client. Nginx variable can be used inside, like `client
addr: $remote_addr\n` |
| abort.percentage | integer | optional | | [0, 100] |
percentage of requests to be aborted. |
+| abort.vars | array[] | optional | | | The rules
for executing fault injection will only be executed when the rules are matched.
`vars` is a list consisting of one or more [[var, operator, val]] elements,
like this: [[[var, operator, val],[var, operator, val]], ...]. For example:
[[["arg_name", "==", "json"]]], indicating that the current request parameter
name is json. The var here is consistent with the naming of Nginx internal
variables, so request_uri, host, etc. can also be used; for the operator part,
the currently supported operators are ==, ~=, ~~, >, <, in, has and !. For
specific usage of operators, please see the `operator-list` part of
[lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). |
| delay.duration | number | required | | | delay
time (can be decimal). |
| delay.percentage | integer | optional | | [0, 100] |
percentage of requests to be delayed. |
+| delay.vars | array[] | optional | | | Execute
the request delay rule, and the request will be delayed only after the rule
matches. `vars` is a list consisting of one or more [[var, operator, val]]
elements, like this: [[[var, operator, val],[var, operator, val]], ...]. For
example: [[["arg_name", "==", "json"]]], indicating that the current request
parameter name is json. The var here is consistent with the naming of Nginx
internal variables, so request_uri, host, etc. can also be used; for the
operator part, the currently supported operators are ==, ~=, ~~, >, <, in, has
and !. For specific usage of operators, please see the `operator-list` part of
[lua-resty-expr](https://github.com/api7/lua-resty-expr#operator-list). |
Note: One of `abort` and `delay` must be specified.
+`vars` is composed of a three-level array structure, as shown below:
+
+```json
+array[
Review comment:
Better to use a real world example. And this one is too far away from
JSON.
##########
File path: apisix/plugins/fault-injection.lua
##########
@@ -77,11 +151,30 @@ end
function _M.rewrite(conf, ctx)
core.log.info("plugin rewrite phase, conf: ", core.json.delay_encode(conf))
- if conf.delay and sample_hit(conf.delay.percentage) then
+ local err
+ local abort_vars = true
+ if conf.abort and conf.abort.vars then
+ abort_vars, err = vars_match(conf.abort.vars, ctx)
+ if err then
+ return 500, err
Review comment:
Better to log the 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:
us...@infra.apache.org
