http: using CONNECT_ERROR for HTTP/2#13519
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
left a comment
There was a problem hiding this comment.
Nice, LGTM with small comment.
/wait
| // Per https://tools.ietf.org/html/rfc7540#section-8.3 if there was an error | ||
| // with the TCP connection during a CONNECT request, it should be | ||
| // communicated via CONNECT_ERROR | ||
| if (requestWasConnect(stream.request_headers_, codec_->protocol()) && |
There was a problem hiding this comment.
I have a vague sense that we do many header type checks to see if something is a connect request. Would it be better for perf if did this check once and stored this somewhere? Not a big deal but maybe something to think about in a follow up.
There was a problem hiding this comment.
We do, but this case is quite special as most are looking at HTTP/1 style CONNECT requests (HTTP/1 connect and HTTP/2 CONNECTs-excluding-upgrades) and this particular one is doing all CONNECT methods (including HTTP/2 upgrades)
| stream->setDetails(Http2ResponseCodeDetails::get().remote_refused); | ||
| } else if (error_code == NGHTTP2_CONNECT_ERROR) { | ||
| reason = StreamResetReason::ConnectError; | ||
| stream->setDetails(Http2ResponseCodeDetails::get().remote_reset); |
There was a problem hiding this comment.
Should we use a different details string here?
There was a problem hiding this comment.
eh, I think it's not worth the detail here (I only split out the reason because otherwise it was impossible to unit test) but I will restructure the code so we only setDetails with it once as a matter of principle :-)
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
* master: (22 commits) http: using CONNECT_ERROR for HTTP/2 (envoyproxy#13519) listener: respect address.pipe.mode (it didn't work) (envoyproxy#13493) examples: Fix more deprecations/warnings in configs (envoyproxy#13529) overload: tcp connection refusal overload action (envoyproxy#13311) tcp: towards pluggable upstreams (envoyproxy#13331) conn_pool: fixing comments (envoyproxy#13520) Prevent SEGFAULT when disabling listener (envoyproxy#13515) Convert overload manager config literals to YAML (envoyproxy#13518) Fix runtime feature variable name (envoyproxy#13533) dependencies: refactor repository location schema utils, cleanups. (envoyproxy#13452) router: fix an invalid ASSERT when encoding metadata frames in the router. (envoyproxy#13511) http2: Proactively disconnect connections flooded when resetting stream (envoyproxy#13482) ci use azp to sync filter example (envoyproxy#13501) mongo_proxy: support configurable command list for metrics (envoyproxy#13494) http local rate limit: note token bucket is shared (envoyproxy#13525) wasm/extensions: Wasm extension policy. (envoyproxy#13526) http: removing envoy.reloadable_features.http1_flood_protection (envoyproxy#13508) build: update ppc64le CI build status shield (envoyproxy#13521) dependencies: enforce dependency shepherd sign-off via RepoKitteh. (envoyproxy#13522) Add no_traffic_healthy_interval (envoyproxy#13336) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Commit Message: http: using CONNECT_ERROR for HTTP/2
Risk Level: Low (changing rst code on the wire)
Testing: new unit, integration tests
Docs Changes: n/a
Release Notes: inline
Fixes #13055