dns: Use typed errors #1670

olix0r · 2022-05-16T17:09:27Z

The dns module uses boxed errors, which makes it difficult to reason
about the types of errors we may encounter.

This change adds error newtypes so that we can at least print more
detailed information about the type of resolution that failed.

Signed-off-by: Oliver Gould ver@buoyant.io

Signed-off-by: Alex Leong <alex@buoyant.io>

The `dns` module uses boxed errors, which makes it difficult to reason about the types of errors we may encounter. This change adds error newtypes so that we can at least print more detailed information about the type of resolution that failed. Signed-off-by: Oliver Gould <ver@buoyant.io>

adleong

Neat!

hawkw

this looks great, one tiny nit

hawkw · 2022-05-16T17:46:42Z

linkerd/app/core/src/errors.rs

+/// Determines if the provided error was caused by an `E` typed error.
+pub fn caused_by<'e, E: std::error::Error + 'static>(
+    mut error: &'e (dyn std::error::Error + 'static),
+) -> Option<&'e E> {
+    while let Some(src) = error.source() {
+        if let Some(e) = src.downcast_ref::<E>() {
+            return Some(e);
+        }
+        error = src;
+    }
+    None
+}


very minor nit: i might consider putting this in linkerd-error, so it could later be used in other crates. also, we already have

linkerd2-proxy/linkerd/error/src/lib.rs

Lines 18 to 20 in a8e4a2e

pub fn is_error<E: std::error::Error + 'static>(e: &(dyn std::error::Error + 'static)) -> bool {

e.is::<E>() || e.source().map(is_error::<E>).unwrap_or(false)

}

which i think could be replaced with this function in a few places.

(not a big deal, i just feel like we've written a bunch of variants of this function in different places...)

I'll followup with a cleanup

This release fixes a problem with HTTP/1.1 `CONNECT` requests. When a server responds to a `CONNECT` request with `content-length` or `transfer-encoding` headers (in violation of RFC 7231), the proxy must actively strip these headers to avoid making the Hyper server fail the response. This release also fixes an issue with the way proxies discover control plane components via DNS. When `SRV` records cannot be resolved, the proxy would no not necessarily fall back to resolving `A` records. This has been fixed. Finally, the inbound proxy can now discover policies dynamically. Ports that are configured in the `LINKERD2_PROXY_INBOUND_PORTS` are discovered as the proxy starts up; but now the proxy will discover policies for ports that are not in this list. The pod's default policy is used initially, but once a policy is received from the control plane it is used. --- * build(deps): bump syn from 1.0.93 to 1.0.94 (linkerd/linkerd2-proxy#1664) * build(deps): bump tj-actions/changed-files from 19 to 20 (linkerd/linkerd2-proxy#1665) * build(deps): bump rustls from 0.20.4 to 0.20.5 (linkerd/linkerd2-proxy#1666) * build(deps): bump ryu from 1.0.9 to 1.0.10 (linkerd/linkerd2-proxy#1667) * build(deps): bump tokio-util from 0.7.1 to 0.7.2 (linkerd/linkerd2-proxy#1668) * build(deps): bump itoa from 1.0.1 to 1.0.2 (linkerd/linkerd2-proxy#1669) * build(deps): bump tonic from 0.7.1 to 0.7.2 (linkerd/linkerd2-proxy#1652) * dns: Fall back to A record when SRV resolution fails (linkerd/linkerd2-proxy#1670) * Consolidate error chain inspection with `cause_ref` (linkerd/linkerd2-proxy#1671) * ci: change how warnings are denied on CI (linkerd/linkerd2-proxy#1662) * build(deps): bump proc-macro2 from 1.0.38 to 1.0.39 (linkerd/linkerd2-proxy#1673) * build(deps): bump libc from 0.2.125 to 0.2.126 (linkerd/linkerd2-proxy#1674) * build(deps): bump syn from 1.0.94 to 1.0.95 (linkerd/linkerd2-proxy#1675) * ci: Use the cargo-action-fmt setup action (linkerd/linkerd2-proxy#1672) * opencensus: Include empty generated protobuf (linkerd/linkerd2-proxy#1676) * build(deps): bump rustls from 0.20.5 to 0.20.6 (linkerd/linkerd2-proxy#1679) * Revert "build(deps): bump socket2 from 0.4.4 to 0.4.5 (linkerd/linkerd2-proxy#1654)" (#1681) * build(deps): bump EmbarkStudios/cargo-deny-action from 1.2.17 to 1.3.0 (linkerd/linkerd2-proxy#1678) * build(deps): bump clang-sys from 1.3.1 to 1.3.2 (linkerd/linkerd2-proxy#1680) * cache: generalize `Cache` into a key-value cache (linkerd/linkerd2-proxy#1683) * build(deps): bump once_cell from 1.10.0 to 1.11.0 (linkerd/linkerd2-proxy#1687) * build(deps): bump EmbarkStudios/cargo-deny-action from 1.3.0 to 1.3.1 (linkerd/linkerd2-proxy#1686) * trace: add `/logs.json` endpoint to admin server (linkerd/linkerd2-proxy#1642) * Dynamically discover policies for undocumented ports (linkerd/linkerd2-proxy#1677) * Don't allow a policy to be used if `check_port_allowed` fails (linkerd/linkerd2-proxy#1689) * ci: Simplify release workflow (linkerd/linkerd2-proxy#1690) * build(deps): bump petgraph from 0.6.0 to 0.6.1 (linkerd/linkerd2-proxy#1696) * build(deps): bump actions/upload-artifact from 3.0.0 to 3.1.0 (linkerd/linkerd2-proxy#1692) * build(deps): bump tj-actions/changed-files from 20 to 20.1 (linkerd/linkerd2-proxy#1693) * build(deps): bump http-body from 0.4.4 to 0.4.5 (linkerd/linkerd2-proxy#1694) * build(deps): bump regex from 1.5.5 to 1.5.6 (linkerd/linkerd2-proxy#1695) * build(deps): bump regex-syntax from 0.6.25 to 0.6.26 (linkerd/linkerd2-proxy#1697) * http: Strip illegal headers from CONNECT responses (linkerd/linkerd2-proxy#1699) * dev: Replace `Makefile` with `justfile` (linkerd/linkerd2-proxy#1691) Signed-off-by: Oliver Gould <ver@buoyant.io>

When resolving control plane components, we first resolve a SRV record. If we're unable to parse IPv4 addresses from the record, then we fall back to an A record lookup. But if the initial resolution fails completely (i.e. doesn't produce any record data), then we don't fall back to an A record. This change fixes this fallback behavior so that we attempt A record lookups whenever a SRV lookup fails. In doing so, we change the dns module to use typed errors (avoiding boxing) so that we can print better error messages and have static guarantees about the error types being handled. Fixes linkerd/linkerd2#8296 Signed-off-by: Oliver Gould <ver@buoyant.io> Co-authored-by: Alex Leong <alex@buoyant.io>

large number of service discovery updates, for incorrect handling of `CONNECT` requests with `Content-Length` headers, and for a failure to correctly resolve DNS SRV records for the control plane. --- 1817b3f3 update Tower to 0.4.13 to fix load balancer panic (linkerd/linkerd2-proxy#1758) bc512765 dns: Fall back to A record when SRV resolution fails (linkerd/linkerd2-proxy#1670) ac1039b0 http: Strip illegal headers from CONNECT responses (linkerd/linkerd2-proxy#1699) ebcf7cbd Dedupe discovery updates (linkerd/linkerd2-proxy#1759)

This release cherry-picks commits from the `main` branch into the `release/v2.161` branch, including fixes for a panic when processing a large number of service discovery updates, for incorrect handling of `CONNECT` requests with `Content-Length` headers, and for a failure to correctly resolve DNS SRV records for the control plane. --- 1817b3f3 update Tower to 0.4.13 to fix load balancer panic (linkerd/linkerd2-proxy#1758) bc512765 dns: Fall back to A record when SRV resolution fails (linkerd/linkerd2-proxy#1670) ac1039b0 http: Strip illegal headers from CONNECT responses (linkerd/linkerd2-proxy#1699) ebcf7cbd Dedupe discovery updates (linkerd/linkerd2-proxy#1759)

adleong and others added 2 commits May 12, 2022 23:11

Fall back to A record when SRV resolution fails

60ab783

Signed-off-by: Alex Leong <alex@buoyant.io>

olix0r requested a review from a team as a code owner May 16, 2022 17:09

olix0r mentioned this pull request May 16, 2022

Fall back to A record when SRV resolution fails #1663

Closed

adleong approved these changes May 16, 2022

View reviewed changes

olix0r changed the base branch from alex/srv-error to main May 16, 2022 17:27

hawkw approved these changes May 16, 2022

View reviewed changes

olix0r merged commit a5d7f0e into main May 16, 2022

olix0r deleted the ver/alex/srv-error branch May 16, 2022 17:52

olix0r mentioned this pull request May 24, 2022

proxy: v2.179.0 linkerd/linkerd2#8543

Merged

kleimkuhler mentioned this pull request Jun 30, 2022

stable-2.11.3 release linkerd/linkerd2#8779

Closed

13 tasks

hawkw mentioned this pull request Jul 1, 2022

proxy: v2.161.4 linkerd/linkerd2#8789

Merged

kleimkuhler mentioned this pull request Jul 21, 2022

linkerd-destination Failed to connect error=Connection refused (os error 111) linkerd/linkerd2#8943

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: Use typed errors #1670

dns: Use typed errors #1670

olix0r commented May 16, 2022

adleong left a comment

hawkw left a comment

hawkw May 16, 2022

olix0r May 16, 2022

	pub fn is_error<E: std::error::Error + 'static>(e: &(dyn std::error::Error + 'static)) -> bool {
	e.is::<E>() \|\| e.source().map(is_error::<E>).unwrap_or(false)
	}

dns: Use typed errors #1670

dns: Use typed errors #1670

Conversation

olix0r commented May 16, 2022

adleong left a comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw May 16, 2022

Choose a reason for hiding this comment

olix0r May 16, 2022

Choose a reason for hiding this comment