Open curiouserrandy opened 4 years ago
mattklein123
commented
4 years ago I agree that we should revisit this short circuit logic for a variety of reasons, not only this one. I think in the future we also want these error responses to be customizable which would be a lot easier if the flow went through the HCM somehow. With that said, I don't think this will be trivial to solve but it's worth it IMO. cc @alyssawilk for further comment.
alyssawilk
commented
4 years ago I took a look earlier at this (when I was doing rc details) and was concerned about corner cases, but I think @htuch and company are planning some codec refactors over Q1/Q2 which would radically simplify the odds of corner cases and make this much more tenable.
ikonst
commented
3 years ago Would this issue also be a precursor for logging those requests? I was recently debugging an issue that stemmed from a lowercase method being used ('post' instead of 'POST'). This resulted in a 400 response from Envoy without any logs. Ideally I'd want it to end up in error logs with a response flag.
alyssawilk
commented
3 years ago yeah, I think stream creation would solve this. I was hoping the move to the new HTTP parser would be the first step towards addressing this, but it has been moving more slowly than expected :-/
Title: Envoy should register invalid HTTP as an attempt to send a request
Description: If a malformed HTTP1 request is sent (e.g. No headers, just "a\r\n\r\n") the HTTP1.1 code will reject it (https://github.com/envoyproxy/envoy/blob/master/source/common/http/http1/codec_impl.cc#L497) and send a 400 without creating a ConnectionManagerImpl::ActiveStream. This means that the stats around total requests (downstream_rqtotal & similar) will not be modified, as they are modified by ConnectionManagerImpl::ActiveStream. It seems like an invalid request should still be counted in those statistics.