Return 503 from ext_authz on network failures#6148
Return 503 from ext_authz on network failures#6148hanyu-liu wants to merge 1 commit intoenvoyproxy:masterfrom
Conversation
hanyu-liu
force-pushed
the
503_for_gRPC_network_issue
branch
3 times, most recently
from
5e403c4 to
854cf21
Compare
hanyu-liu
force-pushed
the
503_for_gRPC_network_issue
branch
from
854cf21 to
4828588
Compare
Return 503 from ext_authz on network failures Risk Level: Low Testing: CI Docs Changes: n/a Release Notes: n/a envoyproxy#6119 Signed-off-by: Hanyu Liu <hanyu.liu@sendgrid.com>
hanyu-liu
force-pushed
the
503_for_gRPC_network_issue
branch
from
4828588 to
0c6097d
Compare
|
@hanyu-liu thanks for this, but I think you need to put this in the doc too? WDYT @gsagula? |
|
@hanyu-liu Thanks for this contribution. It looks good overall. |
gsagula
left a comment
gsagula
left a comment
There was a problem hiding this comment.
Just one minor comment based on our conversation in GH issues.
| Response response{}; | ||
| response.status = CheckStatus::Error; | ||
| response.status_code = Http::Code::Forbidden; | ||
| response.status_code = Http::Code::ServiceUnavailable; |
There was a problem hiding this comment.
As per our conversation in GH, please make ServiceUnavailable configurable and default it to false. This way we avoid breaking changes.
There was a problem hiding this comment.
Ah... I thought you said it's okay to hard code 502 as the response :) Seems it's getting more complex than what I thought.
Any idea on the name of the configuration? Any guidance/docs on introducing new configs?
There was a problem hiding this comment.
Sorry, I misread your last comment about hardcode the status. Unfortunatelly, we can't do that. I think something like status_code_on_error, and then let the user decide which status they want would be sufficient.
Normally, the filter config is done here:
There was a problem hiding this comment.
Thanks for the pointer. I searched for the code of "use_alpha" in "ext_authz" - seems a little bit complex. Do you mind working on top of the current change? I'm more than happy to code review your change. @gsagula
There was a problem hiding this comment.
I wouldn't mind doing it, but I can't promise when I will have time.
There was a problem hiding this comment.
Cool! We are blocked by this while integrating Envoy with our (Twilio) gateway. Your help is greatly appreciated!
There was a problem hiding this comment.
@gsagula do you actually think an enum can work well for status_code_on_error? Don't want users to abuse the system by returning like 200, when the ext_authz is down.
|
Does this only apply for gRPC authorization servers? I kind of have the same issue but for HTTP authorization servers, would really like to have a proper error code when auth. requests are failing and not a 403 (this issue/question is related to this #5974) |
This fix only applies to gRPC. But I can see the same issue for http. |
|
@dio It seems that the contributor is not able to finish the PR. I'm not sure what is the procedure here. I can look into this issue when I finish some other Datawire priorities, but it's hard to tell when. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
stale
bot
removed
the
stale
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
4 participants
Description: Return 503 from ext_authz on network failures
Risk Level: Low
Testing: CI
Docs Changes: n/a
Release Notes: n/a
Discussion thread: #6119