-
Notifications
You must be signed in to change notification settings - Fork 117
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
Issue 6901: set invalid request status if request parsing fails #573
Issue 6901: set invalid request status if request parsing fails #573
Conversation
8693ff6
to
cce020e
Compare
cce020e
to
db4889b
Compare
db4889b
to
c429525
Compare
@rudrakhp please squash your commits and we can get this in. Also have you tested this end-to-end? |
b476322
to
3e2efe9
Compare
ece3b4e
to
143b712
Compare
I added a case for invalid URL in the e2e test suite, but somehow it's getting a 404 before 400 bad request. @ashutosh-narkar if you are aware of this do let me know, otherwise will come back to this tomorrow. |
143b712
to
b131114
Compare
Is the request even reaching OPA? |
@rudrakhp did you get a chance to look into the test failures. This would be a good one to get in. |
@ashutosh-narkar was out on vacation, will get something out in the next couple of days. |
010abc1
to
88839fd
Compare
@ashutosh-narkar looks like the place where I was adding the test was for testing istio setup with the plugin enabled but requests were not going via OPA. I don't see any E2E suite where I can add this test, let me know if there is any. I think unit tests should suffice to test this behaviour as we rely on go-control-plane anyways. WDYT? |
@rudrakhp this looks good. If you could just update the commit message with a note explaining the change that would be great. We can then get this in. Thanks for the contribution. |
If request parsing fails, instead of an error send a denied HTTP response with status 400 Bad Request. Signed-off-by: Rudrakh Panigrahi <[email protected]>
88839fd
to
32b691e
Compare
@ashutosh-narkar updated commit message |
Resolves open-policy-agent/opa#6901