From 34b46ab6cd5f989f0f345cfd364eaddaa1010fa3 Mon Sep 17 00:00:00 2001 From: katelyn martin Date: Thu, 3 Jul 2025 11:56:14 -0400 Subject: [PATCH] refactor: `FmtLabels` impls use exhaustive bindings (#3988) this is based on #3987. in #3987 (_see https://github.com/linkerd/linkerd2/issues/13821_) we discovered that some of the types that implement [`FmtLabels`](https://github.com/linkerd/linkerd2-proxy/blob/085be9978d437800a1a8ff0c2457b9bb9712a166/linkerd/metrics/src/fmt.rs#L5) could collide when used in registry keys; i.e., they might emit identical label sets, but distinct `Hash` values. #3987 solves two bugs. this pull request proposes a follow-on change, introducing _exhaustive_ bindings to implementations of `FmtLabels`, to prevent this category of bug from reoccurring again in the future. this change means that the introduction of an additional field to any of these label structures, e.g. `OutboundEndpointLabels` or `HTTPLocalRateLimitLabels`, will cause a compilation error unless said new field is handled in the corresponding `FmtLabels` implementation. ### :bookmark: a note in writing this pull request, i noticed one label that i believe is unintentionally being elided. i've refrained from changing behavior in this pull request. i do note it though, as an example of this syntax identifying the category of bug i hope to hedge against here. --- * fix: do not key transport metrics registry on `ClientTls` Signed-off-by: katelyn martin * fix: do not key transport metrics registry on `ServerTls` Signed-off-by: katelyn martin * refactor(transport-metrics): exhaustive `Eos: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `ServerLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `TlsAccept: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `TargetAddr: FmtLabels` Signed-off-by: katelyn martin * refactor(metrics): exhaustive `Label: FmtLabels` Signed-off-by: katelyn martin * refactor(http/metrics): exhaustive `Status: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `ControlLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `ProfileRouteLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `InboundEndpointLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `ServerLabel: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `ServerAuthzLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `RouteLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `RouteAuthzLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `OutboundEndpointLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `Authority: FmtLabels` Signed-off-by: katelyn martin * refactor(app/core): exhaustive `StackLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/inbound): exhaustive `HTTPLocalRateLimitLabels: FmtLabels` Signed-off-by: katelyn martin * refactor(app/inbound): exhaustive `Key: FmtLabels` Signed-off-by: katelyn martin * nit(metrics): remove redundant banner comment these impl blocks are all `FmtLabels`, following another series of the same, above. we don't need another one of these comments. Signed-off-by: katelyn martin * nit(metrics): exhaustive `AndThen: FmtMetrics` Signed-off-by: katelyn martin * nit(app/core): note unused label see #3262 (618838ec7), which introduced this label. to preserve behavior, this label remains unused. X-Ref: #3262 Signed-off-by: katelyn martin --------- Signed-off-by: katelyn martin --- linkerd/app/core/src/metrics.rs | 102 +++++++++++++------- linkerd/app/core/src/transport/labels.rs | 25 +++-- linkerd/app/inbound/src/metrics/authz.rs | 22 ++++- linkerd/http/metrics/src/requests/report.rs | 3 +- linkerd/metrics/src/fmt.rs | 8 +- linkerd/metrics/src/histogram.rs | 3 +- linkerd/transport-metrics/src/lib.rs | 3 +- 7 files changed, 110 insertions(+), 56 deletions(-) diff --git a/linkerd/app/core/src/metrics.rs b/linkerd/app/core/src/metrics.rs index 3d4822cc2..c37ee1128 100644 --- a/linkerd/app/core/src/metrics.rs +++ b/linkerd/app/core/src/metrics.rs @@ -250,8 +250,10 @@ impl svc::Param for control::ControlAddr { impl FmtLabels for ControlLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "addr=\"{}\",", self.addr)?; - TlsConnect::from(&self.server_id).fmt_labels(f)?; + let Self { addr, server_id } = self; + + write!(f, "addr=\"{}\",", addr)?; + TlsConnect::from(server_id).fmt_labels(f)?; Ok(()) } @@ -281,10 +283,16 @@ impl ProfileRouteLabels { impl FmtLabels for ProfileRouteLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.direction.fmt_labels(f)?; - write!(f, ",dst=\"{}\"", self.addr)?; + let Self { + direction, + addr, + labels, + } = self; - if let Some(labels) = self.labels.as_ref() { + direction.fmt_labels(f)?; + write!(f, ",dst=\"{}\"", addr)?; + + if let Some(labels) = labels.as_ref() { write!(f, ",{}", labels)?; } @@ -317,16 +325,19 @@ impl FmtLabels for EndpointLabels { impl FmtLabels for InboundEndpointLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - if let Some(a) = self.authority.as_ref() { + let Self { + tls, + authority, + target_addr, + policy, + } = self; + + if let Some(a) = authority.as_ref() { Authority(a).fmt_labels(f)?; write!(f, ",")?; } - ( - (TargetAddr(self.target_addr), TlsAccept::from(&self.tls)), - &self.policy, - ) - .fmt_labels(f)?; + ((TargetAddr(*target_addr), TlsAccept::from(tls)), policy).fmt_labels(f)?; Ok(()) } @@ -334,13 +345,14 @@ impl FmtLabels for InboundEndpointLabels { impl FmtLabels for ServerLabel { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let Self(meta, port) = self; write!( f, "srv_group=\"{}\",srv_kind=\"{}\",srv_name=\"{}\",srv_port=\"{}\"", - self.0.group(), - self.0.kind(), - self.0.name(), - self.1 + meta.group(), + meta.kind(), + meta.name(), + port ) } } @@ -364,39 +376,45 @@ impl prom::EncodeLabelSetMut for ServerLabel { impl FmtLabels for ServerAuthzLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.server.fmt_labels(f)?; + let Self { server, authz } = self; + + server.fmt_labels(f)?; write!( f, ",authz_group=\"{}\",authz_kind=\"{}\",authz_name=\"{}\"", - self.authz.group(), - self.authz.kind(), - self.authz.name() + authz.group(), + authz.kind(), + authz.name() ) } } impl FmtLabels for RouteLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.server.fmt_labels(f)?; + let Self { server, route } = self; + + server.fmt_labels(f)?; write!( f, ",route_group=\"{}\",route_kind=\"{}\",route_name=\"{}\"", - self.route.group(), - self.route.kind(), - self.route.name(), + route.group(), + route.kind(), + route.name(), ) } } impl FmtLabels for RouteAuthzLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.route.fmt_labels(f)?; + let Self { route, authz } = self; + + route.fmt_labels(f)?; write!( f, ",authz_group=\"{}\",authz_kind=\"{}\",authz_name=\"{}\"", - self.authz.group(), - self.authz.kind(), - self.authz.name(), + authz.group(), + authz.kind(), + authz.name(), ) } } @@ -409,16 +427,25 @@ impl svc::Param for OutboundEndpointLabels { impl FmtLabels for OutboundEndpointLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - if let Some(a) = self.authority.as_ref() { + let Self { + server_id, + authority, + labels, + // TODO(kate): this label is not currently emitted. + zone_locality: _, + target_addr, + } = self; + + if let Some(a) = authority.as_ref() { Authority(a).fmt_labels(f)?; write!(f, ",")?; } - let ta = TargetAddr(self.target_addr); - let tls = TlsConnect::from(&self.server_id); + let ta = TargetAddr(*target_addr); + let tls = TlsConnect::from(server_id); (ta, tls).fmt_labels(f)?; - if let Some(labels) = self.labels.as_ref() { + if let Some(labels) = labels.as_ref() { write!(f, ",{}", labels)?; } @@ -443,7 +470,8 @@ impl FmtLabels for Direction { impl FmtLabels for Authority<'_> { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "authority=\"{}\"", self.0) + let Self(authority) = self; + write!(f, "authority=\"{}\"", authority) } } @@ -498,7 +526,13 @@ impl StackLabels { impl FmtLabels for StackLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.direction.fmt_labels(f)?; - write!(f, ",protocol=\"{}\",name=\"{}\"", self.protocol, self.name) + let Self { + direction, + protocol, + name, + } = self; + + direction.fmt_labels(f)?; + write!(f, ",protocol=\"{}\",name=\"{}\"", protocol, name) } } diff --git a/linkerd/app/core/src/transport/labels.rs b/linkerd/app/core/src/transport/labels.rs index c24042b5b..20ee456ae 100644 --- a/linkerd/app/core/src/transport/labels.rs +++ b/linkerd/app/core/src/transport/labels.rs @@ -99,14 +99,17 @@ impl ServerLabels { impl FmtLabels for ServerLabels { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.direction.fmt_labels(f)?; + let Self { + direction, + tls, + target_addr, + policy, + } = self; + + direction.fmt_labels(f)?; f.write_str(",peer=\"src\",")?; - ( - (TargetAddr(self.target_addr), TlsAccept(&self.tls)), - self.policy.as_ref(), - ) - .fmt_labels(f)?; + ((TargetAddr(*target_addr), TlsAccept(tls)), policy.as_ref()).fmt_labels(f)?; Ok(()) } @@ -122,7 +125,8 @@ impl<'t> From<&'t tls::ConditionalServerTlsLabels> for TlsAccept<'t> { impl FmtLabels for TlsAccept<'_> { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self.0 { + let Self(tls) = self; + match tls { Conditional::None(tls::NoServerTls::Disabled) => { write!(f, "tls=\"disabled\"") } @@ -170,12 +174,13 @@ impl FmtLabels for TlsConnect<'_> { impl FmtLabels for TargetAddr { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let Self(target_addr) = self; write!( f, "target_addr=\"{}\",target_ip=\"{}\",target_port=\"{}\"", - self.0, - self.0.ip(), - self.0.port() + target_addr, + target_addr.ip(), + target_addr.port() ) } } diff --git a/linkerd/app/inbound/src/metrics/authz.rs b/linkerd/app/inbound/src/metrics/authz.rs index 0e78d56f6..6c47d2989 100644 --- a/linkerd/app/inbound/src/metrics/authz.rs +++ b/linkerd/app/inbound/src/metrics/authz.rs @@ -251,18 +251,24 @@ impl FmtMetrics for TcpAuthzMetrics { impl FmtLabels for HTTPLocalRateLimitLabels { fn fmt_labels(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.server.fmt_labels(f)?; - if let Some(rl) = &self.rate_limit { + let Self { + server, + rate_limit, + scope, + } = self; + + server.fmt_labels(f)?; + if let Some(rl) = rate_limit { write!( f, ",ratelimit_group=\"{}\",ratelimit_kind=\"{}\",ratelimit_name=\"{}\",ratelimit_scope=\"{}\"", rl.group(), rl.kind(), rl.name(), - self.scope, + scope, ) } else { - write!(f, ",ratelimit_scope=\"{}\"", self.scope) + write!(f, ",ratelimit_scope=\"{}\"", scope) } } } @@ -281,7 +287,13 @@ impl Key { impl FmtLabels for Key { fn fmt_labels(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - (self.target, (&self.labels, TlsAccept(&self.tls))).fmt_labels(f) + let Self { + target, + tls, + labels, + } = self; + + (target, (labels, TlsAccept(tls))).fmt_labels(f) } } diff --git a/linkerd/http/metrics/src/requests/report.rs b/linkerd/http/metrics/src/requests/report.rs index 9b4b7e576..96384d49a 100644 --- a/linkerd/http/metrics/src/requests/report.rs +++ b/linkerd/http/metrics/src/requests/report.rs @@ -146,6 +146,7 @@ where impl FmtLabels for Status { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "status_code=\"{}\"", self.0.as_u16()) + let Self(status) = self; + write!(f, "status_code=\"{}\"", status.as_u16()) } } diff --git a/linkerd/metrics/src/fmt.rs b/linkerd/metrics/src/fmt.rs index 7ded04ed2..919b7fa9c 100644 --- a/linkerd/metrics/src/fmt.rs +++ b/linkerd/metrics/src/fmt.rs @@ -188,8 +188,6 @@ impl FmtLabels for (Option, B) { } } -// === impl FmtMetrics === - impl<'a, A: FmtMetrics + 'a> FmtMetrics for &'a A { #[inline] fn fmt_metrics(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -210,8 +208,10 @@ impl FmtMetrics for Option { impl FmtMetrics for AndThen { #[inline] fn fmt_metrics(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.0.fmt_metrics(f)?; - self.1.fmt_metrics(f)?; + let Self(a, b) = self; + + a.fmt_metrics(f)?; + b.fmt_metrics(f)?; Ok(()) } diff --git a/linkerd/metrics/src/histogram.rs b/linkerd/metrics/src/histogram.rs index 360fd6dbe..96fe154c6 100644 --- a/linkerd/metrics/src/histogram.rs +++ b/linkerd/metrics/src/histogram.rs @@ -224,7 +224,8 @@ impl, F: Factor> FmtMetric for Histogram { impl FmtLabels for Label { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}=\"{}\"", self.0, self.1) + let Self(k, v) = self; + write!(f, "{}=\"{}\"", k, v) } } diff --git a/linkerd/transport-metrics/src/lib.rs b/linkerd/transport-metrics/src/lib.rs index f84e019e6..2f20aa6b3 100644 --- a/linkerd/transport-metrics/src/lib.rs +++ b/linkerd/transport-metrics/src/lib.rs @@ -82,7 +82,8 @@ impl Registry { impl FmtLabels for Eos { fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self.0 { + let Self(errno) = self; + match errno { None => f.pad("errno=\"\""), Some(errno) => write!(f, "errno=\"{}\"", errno), }