errors: Set correct HTTP version on responses#424
Conversation
When synthesizing error responses, all errors were sent with the default HTTP/1.1 response. This change ensures that the request's version is used when synthesizing a response.
Use the request's version if the orig-proto header is not set on a response.
hawkw
left a comment
There was a problem hiding this comment.
this looks good to me! i commented on some minor nits, but feel free to ignore them
| .headers() | ||
| .get(http::header::CONTENT_TYPE) | ||
| .and_then(|v| v.to_str().ok().map(|s| s.starts_with("application/grpc"))) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Nit/TIOLI: might be slightly neater to express this as
let is_grpc = req
.headers()
.get(http::header::CONTENT_TYPE)
.and_then(|v| v.to_str().ok())
.iter()
.contains(|s| s.starts_with("application/grpc"));it's the same amount of code, but i think separating the conversion to a string and the testing the string for the predicate shows the flow more clearly.
admittedly, this doesn't really matter at all! just a thought
kleimkuhler
left a comment
There was a problem hiding this comment.
Looks good, but left a question on UpgradeFuture version matching.
| let version = res | ||
| .headers_mut() | ||
| .remove(L5D_ORIG_PROTO) | ||
| .and_then(|orig_proto| { |
There was a problem hiding this comment.
From above it looks like orig_proto could be either HTTP/1.1 or HTTP/1.1; absolute-form (same for 1.0).
If the L5D_ORIG_PROTO header has the ..; absolute-form ending then the else statement would set it back to None. That behavior isn't changed here, but is that an issue?
There was a problem hiding this comment.
I also noticed this, but after taking a closer look, I believe this isn't an issue. This future is the response future, not the request, and "absolute form" only applies to requests. The request handling side does handle the presence of ; absolute-form in the l5d-orig-proto header.
It would be nice if there was a comment here so future readers don't have to hunt for this...
There was a problem hiding this comment.
Okay cool I agree it doesn't sound like an issue if "absolute form" only applies to requests. Thanks! And yes a comment on this definitely wouldn't hurt.
This release fixes a bug in the proxy's logging subsystem that could cause the proxy to consume memory until the process is OOMKilled, especially when the proxy was configured to log diagnostic information. The proxy also now properly emits `grpc-status` headers when signaling proxy errors to gRPC clients. This release upgrades the proxy's Rust version, the `http` crate dependency to address RUSTSEC-2019-0033 and RUSTSEC-2019-0034, and the `prost` crate dependency has been patched to address RUSTSEC-2020-02. --- * internal: Introduce a locking middleware (linkerd/linkerd2-proxy#408) * Update to Rust 1.40 with new Cargo.lock format (linkerd/linkerd2-proxy#410) * Update http to v0.1.21 (linkerd/linkerd2-proxy#412) * internal: Split retry, http-classify, and http-metrics (linkerd/linkerd2-proxy#409) * Actually update http to v0.1.21 (linkerd/linkerd2-proxy#413) * patch `prost` 0.5 to pick up security fix (linkerd/linkerd2-proxy#414) * metrics: Make Counter & Gauge atomic (linkerd/linkerd2-proxy#415) * Set grpc-status headers on dispatch errors (linkerd/linkerd2-proxy#416) * trace: update `tracing-subscriber` to 0.2.0-alpha.4 (linkerd/linkerd2-proxy#418) * discover: Warn on discovery error (linkerd/linkerd2-proxy#422) * router: Avoid large up-front allocations (linkerd/linkerd2-proxy#421) * errors: Set correct HTTP version on responses (linkerd/linkerd2-proxy#424) * app: initialize tracing prior to parsing env vars (linkerd/linkerd2-proxy#425) * trace: update tracing-subscriber to 0.2.0-alpha.6 (linkerd/linkerd2-proxy#423)
This release fixes a bug in the proxy's logging subsystem that could cause the proxy to consume memory until the process is OOMKilled, especially when the proxy was configured to log diagnostic information. The proxy also now properly emits `grpc-status` headers when signaling proxy errors to gRPC clients. This release upgrades the proxy's Rust version, the `http` crate dependency to address RUSTSEC-2019-0033 and RUSTSEC-2019-0034, and the `prost` crate dependency has been patched to address RUSTSEC-2020-02. --- * internal: Introduce a locking middleware (linkerd/linkerd2-proxy#408) * Update to Rust 1.40 with new Cargo.lock format (linkerd/linkerd2-proxy#410) * Update http to v0.1.21 (linkerd/linkerd2-proxy#412) * internal: Split retry, http-classify, and http-metrics (linkerd/linkerd2-proxy#409) * Actually update http to v0.1.21 (linkerd/linkerd2-proxy#413) * patch `prost` 0.5 to pick up security fix (linkerd/linkerd2-proxy#414) * metrics: Make Counter & Gauge atomic (linkerd/linkerd2-proxy#415) * Set grpc-status headers on dispatch errors (linkerd/linkerd2-proxy#416) * trace: update `tracing-subscriber` to 0.2.0-alpha.4 (linkerd/linkerd2-proxy#418) * discover: Warn on discovery error (linkerd/linkerd2-proxy#422) * router: Avoid large up-front allocations (linkerd/linkerd2-proxy#421) * errors: Set correct HTTP version on responses (linkerd/linkerd2-proxy#424) * app: initialize tracing prior to parsing env vars (linkerd/linkerd2-proxy#425) * trace: update tracing-subscriber to 0.2.0-alpha.6 (linkerd/linkerd2-proxy#423)
When synthesizing error responses, all errors were sent with the default
HTTP/1.1 response. This change ensures that the request's version is
used when synthesizing a response.