Fix an infinite loop when downgrading HTTP/2 errors#1318
Merged
Conversation
578d979 introduced a bug: when the proxy handles errors on HTTP/2-upgraded connections, it can get stuck in an infinite loop when searching the error chain for an HTTP/2 error. This change fixes this inifite loop and adds a test that exercises this behavior to ensure that `downgrade_h2_error` behaves as expected. Fixes linkerd/linkerd2#7103
hawkw
approved these changes
Oct 19, 2021
Contributor
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
we should, uh, probably double check that no other error downcasting loops do this 🙃
Comment on lines
+158
to
+190
| fn test_downgrade_h2_error() { | ||
| assert!( | ||
| downgrade_h2_error(h2::H2Error::from(h2::Reason::PROTOCOL_ERROR)).is::<DowngradedH2Error>(), | ||
| "h2 errors must be downgraded" | ||
| ); | ||
|
|
||
| #[derive(Debug, Error)] | ||
| #[error("wrapped h2 error: {0}")] | ||
| struct WrapError(#[source] Error); | ||
| assert!( | ||
| downgrade_h2_error(WrapError( | ||
| h2::H2Error::from(h2::Reason::PROTOCOL_ERROR).into() | ||
| )) | ||
| .is::<DowngradedH2Error>(), | ||
| "wrapped h2 errors must be downgraded" | ||
| ); | ||
|
|
||
| assert!( | ||
| downgrade_h2_error(WrapError( | ||
| WrapError(h2::H2Error::from(h2::Reason::PROTOCOL_ERROR).into()).into() | ||
| )) | ||
| .is::<DowngradedH2Error>(), | ||
| "double-wrapped h2 errors must be downgraded" | ||
| ); | ||
|
|
||
| assert!( | ||
| !downgrade_h2_error(std::io::Error::new( | ||
| std::io::ErrorKind::Other, | ||
| "non h2 error" | ||
| )) | ||
| .is::<DowngradedH2Error>(), | ||
| "other h2 errors must not be downgraded" | ||
| ); |
Contributor
There was a problem hiding this comment.
nit/TIOLI: it might be nice to have these cases as separate tests, so that we can see all the failures if there are multiples, rather than just the first one? not a big deal
Member
Author
|
@hawkw quick spot check shows that the other error handlers look okay. most of them are recursive, which is probably better in this case ;) |
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Oct 19, 2021
This release fixes a bug where the outbound proxy could loop infinitely while handling errors on meshed HTTP/1 connections. This would typically cause proxies to be fail health checks and be restarted. Furthermore, the proxy now requires identity. Proxies will log an error and fail to start if identity is disabled. --- * dns: Avoid allocating in `Name::is_localhost` (linkerd/linkerd2-proxy#1303) * metrics: Implement FmtMetrics for Option (linkerd/linkerd2-proxy#1302) * tracing: simplify subscriber construction with `Box`ed layers (linkerd/linkerd2-proxy#1304) * Require identity configuration (linkerd/linkerd2-proxy#1305) * build(deps): bump thiserror from 1.0.29 to 1.0.30 (linkerd/linkerd2-proxy#1306) * build(deps): bump tower from 0.4.8 to 0.4.9 (linkerd/linkerd2-proxy#1308) * build(deps): bump trust-dns-resolver (linkerd/linkerd2-proxy#1311) * build(deps): bump actions/checkout from 2.3.4 to 2.3.5 (linkerd/linkerd2-proxy#1313) * dns-name: Remove `webpki` dependency (linkerd/linkerd2-proxy#1316) * build(deps): bump libc from 0.2.103 to 0.2.104 (linkerd/linkerd2-proxy#1315) * inbound: Add a box layer to reduce compile times (linkerd/linkerd2-proxy#1317) * Split cryptographic dependencies into a dedicated crate (linkerd/linkerd2-proxy#1307) * Fix an infinite loop when downgrading HTTP/2 errors (linkerd/linkerd2-proxy#1318)
olix0r
added a commit
to linkerd/linkerd2
that referenced
this pull request
Oct 20, 2021
This release fixes a bug where the outbound proxy could loop infinitely while handling errors on meshed HTTP/1 connections. This would typically cause proxies to be fail health checks and be restarted. Furthermore, the proxy now requires identity. Proxies will log an error and fail to start if identity is disabled. --- * dns: Avoid allocating in `Name::is_localhost` (linkerd/linkerd2-proxy#1303) * metrics: Implement FmtMetrics for Option (linkerd/linkerd2-proxy#1302) * tracing: simplify subscriber construction with `Box`ed layers (linkerd/linkerd2-proxy#1304) * Require identity configuration (linkerd/linkerd2-proxy#1305) * build(deps): bump thiserror from 1.0.29 to 1.0.30 (linkerd/linkerd2-proxy#1306) * build(deps): bump tower from 0.4.8 to 0.4.9 (linkerd/linkerd2-proxy#1308) * build(deps): bump trust-dns-resolver (linkerd/linkerd2-proxy#1311) * build(deps): bump actions/checkout from 2.3.4 to 2.3.5 (linkerd/linkerd2-proxy#1313) * dns-name: Remove `webpki` dependency (linkerd/linkerd2-proxy#1316) * build(deps): bump libc from 0.2.103 to 0.2.104 (linkerd/linkerd2-proxy#1315) * inbound: Add a box layer to reduce compile times (linkerd/linkerd2-proxy#1317) * Split cryptographic dependencies into a dedicated crate (linkerd/linkerd2-proxy#1307) * Fix an infinite loop when downgrading HTTP/2 errors (linkerd/linkerd2-proxy#1318)
olix0r
added a commit
that referenced
this pull request
Oct 22, 2021
578d979 introduced a bug: when the proxy handles errors on HTTP/2-upgraded connections, it can get stuck in an infinite loop when searching the error chain for an HTTP/2 error. This change fixes this inifite loop and adds a test that exercises this behavior to ensure that `downgrade_h2_error` behaves as expected. Fixes linkerd/linkerd2#7103
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
578d979 introduced a bug: when the proxy handles errors on
HTTP/2-upgraded connections, it can get stuck in an infinite loop when
searching the error chain for an HTTP/2 error.
This change fixes this inifite loop and adds a test that exercises this
behavior to ensure that
downgrade_h2_errorbehaves as expected.Fixes linkerd/linkerd2#7103