Skip to content

Commit 34b46ab

Browse files
cratelyncratelyn
authored
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. ### 🔖 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 <kate@buoyant.io> * fix: do not key transport metrics registry on `ServerTls` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(transport-metrics): exhaustive `Eos: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ServerLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `TlsAccept: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `TargetAddr: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(metrics): exhaustive `Label: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(http/metrics): exhaustive `Status: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ControlLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ProfileRouteLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `InboundEndpointLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ServerLabel: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `ServerAuthzLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `RouteLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `RouteAuthzLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `OutboundEndpointLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `Authority: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/core): exhaustive `StackLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): exhaustive `HTTPLocalRateLimitLabels: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): exhaustive `Key<L>: FmtLabels` Signed-off-by: katelyn martin <kate@buoyant.io> * 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 <kate@buoyant.io> * nit(metrics): exhaustive `AndThen: FmtMetrics` Signed-off-by: katelyn martin <kate@buoyant.io> * nit(app/core): note unused label see #3262 (618838e), which introduced this label. to preserve behavior, this label remains unused. X-Ref: #3262 Signed-off-by: katelyn martin <kate@buoyant.io> --------- Signed-off-by: katelyn martin <kate@buoyant.io>
1 parent 288fc74 commit 34b46ab

File tree

7 files changed

+110
-56
lines changed

7 files changed

+110
-56
lines changed

‎linkerd/app/core/src/metrics.rs

Lines changed: 68 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -250,8 +250,10 @@ impl svc::Param<ControlLabels> for control::ControlAddr {
250250

251251
impl FmtLabels for ControlLabels {
252252
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
253-
write!(f, "addr=\"{}\",", self.addr)?;
254-
TlsConnect::from(&self.server_id).fmt_labels(f)?;
253+
let Self { addr, server_id } = self;
254+
255+
write!(f, "addr=\"{}\",", addr)?;
256+
TlsConnect::from(server_id).fmt_labels(f)?;
255257

256258
Ok(())
257259
}
@@ -281,10 +283,16 @@ impl ProfileRouteLabels {
281283

282284
impl FmtLabels for ProfileRouteLabels {
283285
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
284-
self.direction.fmt_labels(f)?;
285-
write!(f, ",dst=\"{}\"", self.addr)?;
286+
let Self {
287+
direction,
288+
addr,
289+
labels,
290+
} = self;
291+
292+
direction.fmt_labels(f)?;
293+
write!(f, ",dst=\"{}\"", addr)?;
286294

287-
if let Some(labels) = self.labels.as_ref() {
295+
if let Some(labels) = labels.as_ref() {
288296
write!(f, ",{}", labels)?;
289297
}
290298

@@ -317,30 +325,34 @@ impl FmtLabels for EndpointLabels {
317325

318326
impl FmtLabels for InboundEndpointLabels {
319327
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
320-
if let Some(a) = self.authority.as_ref() {
328+
let Self {
329+
tls,
330+
authority,
331+
target_addr,
332+
policy,
333+
} = self;
334+
335+
if let Some(a) = authority.as_ref() {
321336
Authority(a).fmt_labels(f)?;
322337
write!(f, ",")?;
323338
}
324339

325-
(
326-
(TargetAddr(self.target_addr), TlsAccept::from(&self.tls)),
327-
&self.policy,
328-
)
329-
.fmt_labels(f)?;
340+
((TargetAddr(*target_addr), TlsAccept::from(tls)), policy).fmt_labels(f)?;
330341

331342
Ok(())
332343
}
333344
}
334345

335346
impl FmtLabels for ServerLabel {
336347
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
348+
let Self(meta, port) = self;
337349
write!(
338350
f,
339351
"srv_group=\"{}\",srv_kind=\"{}\",srv_name=\"{}\",srv_port=\"{}\"",
340-
self.0.group(),
341-
self.0.kind(),
342-
self.0.name(),
343-
self.1
352+
meta.group(),
353+
meta.kind(),
354+
meta.name(),
355+
port
344356
)
345357
}
346358
}
@@ -364,39 +376,45 @@ impl prom::EncodeLabelSetMut for ServerLabel {
364376

365377
impl FmtLabels for ServerAuthzLabels {
366378
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
367-
self.server.fmt_labels(f)?;
379+
let Self { server, authz } = self;
380+
381+
server.fmt_labels(f)?;
368382
write!(
369383
f,
370384
",authz_group=\"{}\",authz_kind=\"{}\",authz_name=\"{}\"",
371-
self.authz.group(),
372-
self.authz.kind(),
373-
self.authz.name()
385+
authz.group(),
386+
authz.kind(),
387+
authz.name()
374388
)
375389
}
376390
}
377391

378392
impl FmtLabels for RouteLabels {
379393
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
380-
self.server.fmt_labels(f)?;
394+
let Self { server, route } = self;
395+
396+
server.fmt_labels(f)?;
381397
write!(
382398
f,
383399
",route_group=\"{}\",route_kind=\"{}\",route_name=\"{}\"",
384-
self.route.group(),
385-
self.route.kind(),
386-
self.route.name(),
400+
route.group(),
401+
route.kind(),
402+
route.name(),
387403
)
388404
}
389405
}
390406

391407
impl FmtLabels for RouteAuthzLabels {
392408
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
393-
self.route.fmt_labels(f)?;
409+
let Self { route, authz } = self;
410+
411+
route.fmt_labels(f)?;
394412
write!(
395413
f,
396414
",authz_group=\"{}\",authz_kind=\"{}\",authz_name=\"{}\"",
397-
self.authz.group(),
398-
self.authz.kind(),
399-
self.authz.name(),
415+
authz.group(),
416+
authz.kind(),
417+
authz.name(),
400418
)
401419
}
402420
}
@@ -409,16 +427,25 @@ impl svc::Param<OutboundZoneLocality> for OutboundEndpointLabels {
409427

410428
impl FmtLabels for OutboundEndpointLabels {
411429
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
412-
if let Some(a) = self.authority.as_ref() {
430+
let Self {
431+
server_id,
432+
authority,
433+
labels,
434+
// TODO(kate): this label is not currently emitted.
435+
zone_locality: _,
436+
target_addr,
437+
} = self;
438+
439+
if let Some(a) = authority.as_ref() {
413440
Authority(a).fmt_labels(f)?;
414441
write!(f, ",")?;
415442
}
416443

417-
let ta = TargetAddr(self.target_addr);
418-
let tls = TlsConnect::from(&self.server_id);
444+
let ta = TargetAddr(*target_addr);
445+
let tls = TlsConnect::from(server_id);
419446
(ta, tls).fmt_labels(f)?;
420447

421-
if let Some(labels) = self.labels.as_ref() {
448+
if let Some(labels) = labels.as_ref() {
422449
write!(f, ",{}", labels)?;
423450
}
424451

@@ -443,7 +470,8 @@ impl FmtLabels for Direction {
443470

444471
impl FmtLabels for Authority<'_> {
445472
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
446-
write!(f, "authority=\"{}\"", self.0)
473+
let Self(authority) = self;
474+
write!(f, "authority=\"{}\"", authority)
447475
}
448476
}
449477

@@ -498,7 +526,13 @@ impl StackLabels {
498526

499527
impl FmtLabels for StackLabels {
500528
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
501-
self.direction.fmt_labels(f)?;
502-
write!(f, ",protocol=\"{}\",name=\"{}\"", self.protocol, self.name)
529+
let Self {
530+
direction,
531+
protocol,
532+
name,
533+
} = self;
534+
535+
direction.fmt_labels(f)?;
536+
write!(f, ",protocol=\"{}\",name=\"{}\"", protocol, name)
503537
}
504538
}

‎linkerd/app/core/src/transport/labels.rs

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -99,14 +99,17 @@ impl ServerLabels {
9999

100100
impl FmtLabels for ServerLabels {
101101
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
102-
self.direction.fmt_labels(f)?;
102+
let Self {
103+
direction,
104+
tls,
105+
target_addr,
106+
policy,
107+
} = self;
108+
109+
direction.fmt_labels(f)?;
103110
f.write_str(",peer=\"src\",")?;
104111

105-
(
106-
(TargetAddr(self.target_addr), TlsAccept(&self.tls)),
107-
self.policy.as_ref(),
108-
)
109-
.fmt_labels(f)?;
112+
((TargetAddr(*target_addr), TlsAccept(tls)), policy.as_ref()).fmt_labels(f)?;
110113

111114
Ok(())
112115
}
@@ -122,7 +125,8 @@ impl<'t> From<&'t tls::ConditionalServerTlsLabels> for TlsAccept<'t> {
122125

123126
impl FmtLabels for TlsAccept<'_> {
124127
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
125-
match self.0 {
128+
let Self(tls) = self;
129+
match tls {
126130
Conditional::None(tls::NoServerTls::Disabled) => {
127131
write!(f, "tls=\"disabled\"")
128132
}
@@ -170,12 +174,13 @@ impl FmtLabels for TlsConnect<'_> {
170174

171175
impl FmtLabels for TargetAddr {
172176
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
177+
let Self(target_addr) = self;
173178
write!(
174179
f,
175180
"target_addr=\"{}\",target_ip=\"{}\",target_port=\"{}\"",
176-
self.0,
177-
self.0.ip(),
178-
self.0.port()
181+
target_addr,
182+
target_addr.ip(),
183+
target_addr.port()
179184
)
180185
}
181186
}

‎linkerd/app/inbound/src/metrics/authz.rs

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -251,18 +251,24 @@ impl FmtMetrics for TcpAuthzMetrics {
251251

252252
impl FmtLabels for HTTPLocalRateLimitLabels {
253253
fn fmt_labels(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
254-
self.server.fmt_labels(f)?;
255-
if let Some(rl) = &self.rate_limit {
254+
let Self {
255+
server,
256+
rate_limit,
257+
scope,
258+
} = self;
259+
260+
server.fmt_labels(f)?;
261+
if let Some(rl) = rate_limit {
256262
write!(
257263
f,
258264
",ratelimit_group=\"{}\",ratelimit_kind=\"{}\",ratelimit_name=\"{}\",ratelimit_scope=\"{}\"",
259265
rl.group(),
260266
rl.kind(),
261267
rl.name(),
262-
self.scope,
268+
scope,
263269
)
264270
} else {
265-
write!(f, ",ratelimit_scope=\"{}\"", self.scope)
271+
write!(f, ",ratelimit_scope=\"{}\"", scope)
266272
}
267273
}
268274
}
@@ -281,7 +287,13 @@ impl<L> Key<L> {
281287

282288
impl<L: FmtLabels> FmtLabels for Key<L> {
283289
fn fmt_labels(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
284-
(self.target, (&self.labels, TlsAccept(&self.tls))).fmt_labels(f)
290+
let Self {
291+
target,
292+
tls,
293+
labels,
294+
} = self;
295+
296+
(target, (labels, TlsAccept(tls))).fmt_labels(f)
285297
}
286298
}
287299

‎linkerd/http/metrics/src/requests/report.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ where
146146

147147
impl FmtLabels for Status {
148148
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
149-
write!(f, "status_code=\"{}\"", self.0.as_u16())
149+
let Self(status) = self;
150+
write!(f, "status_code=\"{}\"", status.as_u16())
150151
}
151152
}

‎linkerd/metrics/src/fmt.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -188,8 +188,6 @@ impl<A: FmtLabels, B: FmtLabels> FmtLabels for (Option<A>, B) {
188188
}
189189
}
190190

191-
// === impl FmtMetrics ===
192-
193191
impl<'a, A: FmtMetrics + 'a> FmtMetrics for &'a A {
194192
#[inline]
195193
fn fmt_metrics(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
@@ -210,8 +208,10 @@ impl<M: FmtMetrics> FmtMetrics for Option<M> {
210208
impl<A: FmtMetrics, B: FmtMetrics> FmtMetrics for AndThen<A, B> {
211209
#[inline]
212210
fn fmt_metrics(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
213-
self.0.fmt_metrics(f)?;
214-
self.1.fmt_metrics(f)?;
211+
let Self(a, b) = self;
212+
213+
a.fmt_metrics(f)?;
214+
b.fmt_metrics(f)?;
215215

216216
Ok(())
217217
}

‎linkerd/metrics/src/histogram.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -224,7 +224,8 @@ impl<V: Into<u64>, F: Factor> FmtMetric for Histogram<V, F> {
224224

225225
impl<K: fmt::Display, V: fmt::Display> FmtLabels for Label<K, V> {
226226
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
227-
write!(f, "{}=\"{}\"", self.0, self.1)
227+
let Self(k, v) = self;
228+
write!(f, "{}=\"{}\"", k, v)
228229
}
229230
}
230231

‎linkerd/transport-metrics/src/lib.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,8 @@ impl<K: Eq + Hash + FmtLabels> Registry<K> {
8282

8383
impl FmtLabels for Eos {
8484
fn fmt_labels(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
85-
match self.0 {
85+
let Self(errno) = self;
86+
match errno {
8687
None => f.pad("errno=\"\""),
8788
Some(errno) => write!(f, "errno=\"{}\"", errno),
8889
}

0 commit comments

Comments
 (0)