feat(fault-injection): support conditional fault injection using nginx variables#3363
Merged
membphis merged 8 commits intoJan 25, 2021
Merged
Conversation
Member
|
ci failed |
Contributor
Author
I need to adjust the code logic and I will solve the ci problem later. |
Firstsawyou
marked this pull request as ready for review
January 23, 2021 12:28
spacewander
requested changes
Jan 25, 2021
| local plugin_name = "fault-injection" | ||
|
|
||
|
|
||
| local vars_schema = { |
Member
There was a problem hiding this comment.
I think we need to remove the vars_schema.
- we can do the check better with
expr.newwhen checking the schema. - when
not/oris supported directly in the expr, this schema is outdated. - this schema contains many incorrect hardcoded limitations, like the
maxLengthof operator is2(not enough forhas), and an expression may contain 4 elements if!is used, etc.
Member
There was a problem hiding this comment.
I think just keep:
local vars_schema = {
type = "array",
maxItems = 20
}
is enough. The remain can be validated via expr.new.
Contributor
Author
There was a problem hiding this comment.
Okay, let me try it.
| for _, var in ipairs(vars) do | ||
| local expr, err = expr.new(var) | ||
| if err then | ||
| core.log.error("vars expression does not match: ", err) |
Member
There was a problem hiding this comment.
Should be failed to create vars expression, we haven't matched it yet.
spacewander
reviewed
Jan 25, 2021
|
|
||
|
|
||
| function _M.check_schema(conf) | ||
| local ok, err = core.schema.check(schema, conf) |
Member
There was a problem hiding this comment.
Better to check the expression here and add a test for it.
Member
There was a problem hiding this comment.
No, I mean checking the expression via:
local expr, err = expr.new(var)
if err then
core.log.error("failed to create vars expression: ", err)
return nil, err
end
spacewander
approved these changes
Jan 25, 2021
membphis
approved these changes
Jan 25, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
We can add new features to the fault-injection plugin for the following reasons:
Currently, the
fault-injectionplugin does not support custom conditions to implement themockfunction. I think we can add the request header as a conditional judgment in the request (there may be a better way) to realize the mock function.E.g:
If
varspasses the verification, fault injection is performed on the request. Otherwise, no processing is done on the request. In order to maintain backward compatibility, the fault injection operation is still performed when the vars condition is not configured.close #2511
Pre-submission checklist: