Policy: add on_failed policy #1286
Conversation
eloycoto
force-pushed
the
THREESCALE-6705
branch
from
38fddb8
to
24ee92f
Compare
eloycoto
changed the title
eloycoto
force-pushed
the
THREESCALE-6705
branch
from
24ee92f
to
815f6f7
Compare
eloycoto
changed the title
| policy_error_callback = function(policy_name, error_message) | ||
| ngx.log(ngx.DEBUG, "Stop request because policy: '", policy_name, "', failed") | ||
| ngx.exit(self.error_status_code or ngx.HTTP_SERVICE_UNAVAILABLE) | ||
| end |
| return { | ||
| policy_error_callback = function(policy_name, error_message) | ||
| ngx.log(ngx.DEBUG, "Stop request because policy: '", policy_name, "', failed") | ||
| ngx.exit(self.error_status_code or ngx.HTTP_SERVICE_UNAVAILABLE) |
There was a problem hiding this comment.
This or is not necessary because it's already in the new function.
| function _M:export() | ||
| return { | ||
| policy_error_callback = function(policy_name, error_message) | ||
| ngx.log(ngx.DEBUG, "Stop request because policy: '", policy_name, "', failed") |
There was a problem hiding this comment.
Shouldn't this log also the error_message received in the params?
| return policy | ||
| end | ||
|
|
||
| function _M:access() | ||
| if self.fail_access then | ||
| self.fail() |
There was a problem hiding this comment.
Where is this fail function defined?
There was a problem hiding this comment.
I guess this is intentional to force an error.
t/apicast-policy-on_failed.t
Outdated
| GET /test | ||
| --- error_code: 503 | ||
| --- error_log | ||
| Policy example_policy crashed in .new() |
There was a problem hiding this comment.
Is this error message correct? In the export function of the on failed policy, it doesn't mention crashed, and it does not mention the name of the function where the error happens.
t/apicast-policy-on_failed.t
Outdated
| 'APICAST_POLICY_LOAD_PATH' => abs_path($ENV{TEST_NGINX_APICAST_POLICY_LOAD_PATH}), | ||
| ); | ||
|
|
||
| repeat_each(1); |
There was a problem hiding this comment.
Is there a reason why these tests cannot run twice, as defined by default?
| @@ -178,9 +178,13 @@ function _M:add_policy(name, version, ...) | |||
| return self:insert(policy) | |||
|
|
|||
| elseif err then | |||
| -- This will only report the last one that failed, but at least users | |||
There was a problem hiding this comment.
I find it a bit counter-intuitive that it returns the last one instead of the first one that fails.
There was a problem hiding this comment.
Errors are logged, but only on request time we log the last one.
| @@ -178,9 +178,13 @@ function _M:add_policy(name, version, ...) | |||
| return self:insert(policy) | |||
|
|
|||
| elseif err then | |||
| -- This will only report the last one that failed, but at least users | |||
| -- can be aware of the issue | |||
| self.init_failed = true | |||
There was a problem hiding this comment.
These 3 attrs are related, would it make sense to store just one table that contains a policy_name and an error ?
|
I wonder if providing an option in the service config instead of the "on_failed" policy could be a better option. The only downside I see is that it would require changes in Porta (add a bool param). |
|
Porta is having soo many pending things, that this cannot be achieved at all. This is the reason of moving to a policy. |
eloycoto
force-pushed
the
THREESCALE-6705
branch
2 times, most recently
from
7675442
to
225c63d
Compare
eloycoto
force-pushed
the
THREESCALE-6705
branch
from
225c63d
to
a6c6734
Compare
Some users complained that if a policy fails, the request didn't terminate. This problem is strategic for some users because it can raise a security flaw if a policy is not executed correctly(jwt_claim_check policy as an example) This pull request adds some "pcalls" on policy_chain, where the error is checked and, if the context has a callback function defined, is called. Fix THREESCALE-6705 Signed-off-by: Eloy Coto <eloy.coto@acalustra.com>
eloycoto
force-pushed
the
THREESCALE-6705
branch
from
a6c6734
to
a72255b
Compare
Some users complained that if a policy fails, the request didn't
terminate. This problem is strategic for some users because it can raise
a security flaw if a policy is not executed correctly(jwt_claim_check
policy as an example)
This pull request adds some "pcalls" on policy_chain, where the error is
checked and, if the context has a callback function defined, is called.
Fix THREESCALE-6705
Signed-off-by: Eloy Coto eloy.coto@acalustra.com