-
Notifications
You must be signed in to change notification settings - Fork 134
fix: return user understandable response for request parse and validation #1621
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: yxia216 <[email protected]>
Signed-off-by: yxia216 <[email protected]>
Signed-off-by: yxia216 <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1621 +/- ##
==========================================
+ Coverage 83.26% 83.29% +0.03%
==========================================
Files 137 137
Lines 12051 12069 +18
==========================================
+ Hits 10034 10053 +19
- Misses 1410 1411 +1
+ Partials 607 605 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/retest |
mathetake
left a comment
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.
You can't just return the internal error. That will expose the security risk and implementation detail. Imagine a SaaS service running this project. You have to explicitly define error type you want to return vs others.
|
@mathetake could you please tell me why it's |
@hustxiayang we need to separate out the validation errors, process body can return other internal gateway errors which should not return immediate response. |
Signed-off-by: yxia216 <[email protected]>
|
@mathetake @yuzisun Thanks a lot for your comments! This is only to return immediate response when the request is not parsable or not able to be processed. I reverted the changes in ai-gateway/internal/extproc/processor_impl.go Line 282 in 932c4e2
|
Description
Motivation: Users complain they just got a 500 and None when they have bugs. There is not any useful information.
Thus, instead of returning nil, I think it's better to return some useful information and valid status code to users
I updated 3 places, other places I think it's totally controlled by ai-gateway, thus I feel it's ok to still return nil:
https://github.com/envoyproxy/ai-gateway/blob/a2ba2c9ed8e7488d6fd76dbcbe08ac7008170e5d/internal/extproc/processor_impl.go#L328C15-L329C55
ai-gateway/internal/extproc/processor_impl.go
Line 359 in a2ba2c9
ai-gateway/internal/extproc/processor_impl.go
Line 368 in a2ba2c9
ai-gateway/internal/extproc/processor_impl.go
Line 394 in a2ba2c9
ai-gateway/internal/extproc/processor_impl.go
Line 435 in a2ba2c9
after discussion, this place
ai-gateway/internal/extproc/processor_impl.go
Line 282 in 932c4e2
should still return nil too as this is not real auth