-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Failure to parse request should result in a bad request response. #305
Failure to parse request should result in a bad request response. #305
Conversation
ab76d5f
to
2c2b41a
Compare
I have two outstanding questions about this change:
|
Thanks for picking this up 🎉
I suppose one new configurable we could introduce is "error on bad input" and it could default to StatusCode_BadRequest. But if you somehow depend on the other behaviour, or want this to be 403, you could configure it accordingly...? 🤔
What would you have in mind there? The error is logged, so that seems OK. |
HttpResponse: &ext_authz_v3.CheckResponse_DeniedResponse{ | ||
DeniedResponse: deniedResponse, | ||
}, | ||
Status: &rpc_status.Status{Code: int32(code.Code_INVALID_ARGUMENT)}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just now strikes me as odd that you've got to define both the http status and this one. 🤔
From the docs (https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-checkresponse), the inner one would overwrite the outer one. So let's only specify one of the two? This would also make it simpler to configure this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought this was slightly odd but I am not 100% sure how to clean it up. We have to set something in CheckResponse.Status
or it defaults to 200. Setting this field to anything other than 200 instructs Envoy to send the default response of 403.
Since DeniedHttpResponse.Status
is where we are able to override Envoy's HTTP response status, we must set that value at a minimum.
For CheckResponse.Status
we could continue to set that to 403 as we do below (e.g. if we introduced config, we would never configure this part of the response) but i'm not sure that makes it any more clear?
2c2b41a
to
771f9ee
Compare
Yeah I guess we could either let users explicitly configure things as you suggested above or we could have something more generic like I guess the question is whether we want this to be the default behavior going forward or not. If so, we could merge this as-is to keep things simple. (If folks have issues we could add the configuration in a future release.) If not, I am happy to add one of the config options above and default the behavior to returning 403 as it works today... |
Signed-off-by: Charlie Crawford <[email protected]>
19ded0b
to
415366e
Compare
Sorry for the radio silence. I'll get back to this next week! |
I think this is preferable. Everyone is happiest if they can just upgrade without any seconds thoughts, and that's what we'd need to do to achieve this. Sorry it took me so long to come up with that 😅 |
Let's reopen when you get a chance to get back to this. 😃 |
A potential solution to #646