refactor(app/inbound): router metrics middleware are T-agnostic (#4181)

cratelyn · web-flow · commit f2e5da37eca1 · 2025-09-23T21:19:10.000-04:00
#4119 introduced a `Permitted<T>` structure, to provide a more formal notion of a `(HttpRoutePermit, T)` tuple. during review of #4180, we had a discussion about how the inbound proxy's http router had metrics layers which were tightly coupled to the notion of a `Permitted<T>`: > This isn't a hard blocker--but this is unfortunate that we're coupling > this impl to the Permitted target type -- target types are usually an > _internal implementation detail_ and not a public api that is depended > on elsewhere. > > ... > > and then have our Permitted impl `Param<MetricsVariant> for Permitted<T>`. > > then this module remains decoupled from the concrete target types. > > That is, the metrics module is decoupled from _where it's called_--as > long as the caller provides a target that can give us what we need to > build our extractors, we're good to go. this branch performs a series of changes to decouple our metric layers' extractors from `Permitted<T>`. importantly, this has the effect of leaving the inbound metrics layers' extractors agnostic to their target: they now provide implementations like `svc::ExtractParam<BodyDataMetrics, T>` rather than `svc::ExtractParam<BodyDataMetrics, Permitted<T>>`. see #4180. --- * refactor(app/inbound): `Permitted<T>` is a struct this restructures `Permitted<T>` so that it is no longer an enum. we instead define a "dumb" enum that represents the grpc/http dichotomy, and return to a model in which `Permitted<T>` is a `struct`. Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): `Permitted<T>` is not `T: Param<P>` in order to carve out space to allow us to define other `Param<P>` implementations for `Permitted<T>`, we remove this blanket deference to the inner `T` target. Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): `Permitted<T>` exposes `Param<P>` impls we cannot expose `T` or any of its own parameters, but we can expose the permit, its variant, and its labels. Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): `family` lookups accept `PermitVariant` these now accept specifically the variant, rather than the permitted target. Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): `ExtractRecordBodyDataMetrics` is `T`-agnostic this type now can extract metrics from arbitrary targets. Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): `ExtractRequestCount` is `T`-agnostic this type now can extract metrics from arbitrary targets. Signed-off-by: katelyn martin <kate@buoyant.io> * refactor(app/inbound): `LogicalPerRequest::from` is `T`-agnostic Signed-off-by: katelyn martin <kate@buoyant.io> --------- Signed-off-by: katelyn martin <kate@buoyant.io>
diff --git a/linkerd/app/inbound/src/http/router.rs b/linkerd/app/inbound/src/http/router.rs
@@ -1,4 +1,7 @@
-use crate::{policy, stack_labels, Inbound};
+use crate::{
+    policy::{self, HttpRoutePermit},
+    stack_labels, Inbound,
+};
 use linkerd_app_core::{
     classify, errors, http_tracing, profiles,
     proxy::{http, tap},
@@ -259,14 +262,14 @@ impl<C> Inbound<C> {
 
 // === impl LogicalPerRequest ===
 
-impl<'a, T> From<&'a policy::Permitted<T>> for LogicalPerRequest
+impl<'a, T> From<&'a T> for LogicalPerRequest
 where
     T: Param<Remote<ServerAddr>>,
     T: Param<tls::ConditionalServerTls>,
+    T: Param<HttpRoutePermit>,
 {
-    fn from(permitted: &'a policy::Permitted<T>) -> Self {
-        let target = permitted.target_ref();
-        let permit = permitted.permit_ref();
+    fn from(target: &'a T) -> Self {
+        let permit: HttpRoutePermit = target.param();
 
         let labels = [
             ("srv", &permit.labels.route.server.0),
@@ -286,7 +289,7 @@ where
         Self {
             server: target.param(),
             tls: target.param(),
-            permit: permit.clone(),
+            permit,
             labels: labels.into(),
         }
     }
diff --git a/linkerd/app/inbound/src/http/router/metrics.rs b/linkerd/app/inbound/src/http/router/metrics.rs
@@ -1,4 +1,4 @@
-use crate::{policy::Permitted, InboundMetrics};
+use crate::{policy::PermitVariant, InboundMetrics};
 use linkerd_app_core::{
     metrics::{
         prom::{
@@ -90,25 +90,30 @@ impl RequestCountFamilies {
     }
 
     /// Fetches the proper request counting family, given a permitted target.
-    fn family<T>(
+    fn family(
         &self,
-        permitted: &Permitted<T>,
+        variant: PermitVariant,
     ) -> &linkerd_http_prom::count_reqs::RequestCountFamilies<RequestCountLabels> {
         let Self { grpc, http } = self;
-        match permitted {
-            Permitted::Grpc { .. } => grpc,
-            Permitted::Http { .. } => http,
+        match variant {
+            PermitVariant::Grpc => grpc,
+            PermitVariant::Http => http,
         }
     }
 }
 
 // === impl ExtractRequestCount ===
 
-impl<T> svc::ExtractParam<RequestCount, Permitted<T>> for ExtractRequestCount {
-    fn extract_param(&self, permitted: &Permitted<T>) -> RequestCount {
+impl<T> svc::ExtractParam<RequestCount, T> for ExtractRequestCount
+where
+    T: svc::Param<PermitVariant> + svc::Param<RouteLabels>,
+{
+    fn extract_param(&self, target: &T) -> RequestCount {
         let Self(families) = self;
-        let family = families.family(permitted);
-        let route = permitted.route_labels();
+        let variant = target.param();
+        let route = target.param();
+
+        let family = families.family(variant);
 
         family.metrics(&RequestCountLabels { route })
     }
@@ -166,14 +171,14 @@ impl ResponseBodyFamilies {
     }
 
     /// Fetches the proper body frame metrics family, given a permitted target.
-    fn family<T>(
+    fn family(
         &self,
-        permitted: &Permitted<T>,
+        variant: PermitVariant,
     ) -> &linkerd_http_prom::body_data::response::ResponseBodyFamilies<ResponseBodyDataLabels> {
         let Self { grpc, http } = self;
-        match permitted {
-            Permitted::Grpc { .. } => grpc,
-            Permitted::Http { .. } => http,
+        match variant {
+            PermitVariant::Grpc => grpc,
+            PermitVariant::Http => http,
         }
     }
 }
@@ -213,11 +218,16 @@ impl EncodeLabelSet for ResponseBodyDataLabels {
 
 // === impl ExtractRecordBodyDataMetrics ===
 
-impl<T> svc::ExtractParam<BodyDataMetrics, Permitted<T>> for ExtractRecordBodyDataMetrics {
-    fn extract_param(&self, permitted: &Permitted<T>) -> BodyDataMetrics {
+impl<T> svc::ExtractParam<BodyDataMetrics, T> for ExtractRecordBodyDataMetrics
+where
+    T: svc::Param<PermitVariant> + svc::Param<RouteLabels>,
+{
+    fn extract_param(&self, target: &T) -> BodyDataMetrics {
         let Self(families) = self;
-        let family = families.family(permitted);
-        let route = permitted.route_labels();
+        let variant = target.param();
+        let route = target.param();
+
+        let family = families.family(variant);
 
         family.metrics(&ResponseBodyDataLabels { route })
     }
diff --git a/linkerd/app/inbound/src/policy.rs b/linkerd/app/inbound/src/policy.rs
@@ -12,7 +12,7 @@ pub use self::{
     config::Config,
     http::{
         HttpInvalidPolicy, HttpRouteInvalidRedirect, HttpRouteNotFound, HttpRouteRedirect,
-        HttpRouteUnauthorized, NewHttpPolicy, Permitted,
+        HttpRouteUnauthorized, NewHttpPolicy, PermitVariant, Permitted,
     },
     tcp::NewTcpPolicy,
 };
diff --git a/linkerd/app/inbound/src/policy/http.rs b/linkerd/app/inbound/src/policy/http.rs
@@ -7,8 +7,8 @@ use futures::{future, TryFutureExt};
 use linkerd_app_core::{
     metrics::{RouteAuthzLabels, RouteLabels},
     svc::{self, ServiceExt},
-    tls,
-    transport::{ClientAddr, OrigDstAddr, Remote},
+    tls::{self, ConditionalServerTls},
+    transport::{ClientAddr, OrigDstAddr, Remote, ServerAddr},
     Conditional, Error, Result,
 };
 use linkerd_proxy_server_policy::{grpc, http, route::RouteMatch};
@@ -48,9 +48,16 @@ struct ConnectionMeta {
 
 /// A `T`-typed target with policy enforced by a [`NewHttpPolicy<N>`] layer.
 #[derive(Debug)]
-pub enum Permitted<T> {
-    Grpc { permit: HttpRoutePermit, target: T },
-    Http { permit: HttpRoutePermit, target: T },
+pub struct Permitted<T> {
+    permit: HttpRoutePermit,
+    protocol: PermitVariant,
+    target: T,
+}
+
+#[derive(Clone, Copy, Debug)]
+pub enum PermitVariant {
+    Grpc,
+    Http,
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -170,12 +177,20 @@ where
             Some(Routes::Http(routes)) => {
                 let (permit, mtch, route) = try_fut!(self.authorize(&routes, &req));
                 try_fut!(apply_http_filters(mtch, route, &mut req));
-                Permitted::Http { permit, target }
+                Permitted {
+                    permit,
+                    target,
+                    protocol: PermitVariant::Http,
+                }
             }
             Some(Routes::Grpc(routes)) => {
                 let (permit, _, route) = try_fut!(self.authorize(&routes, &req));
                 try_fut!(apply_grpc_filters(route, &mut req));
-                Permitted::Grpc { permit, target }
+                Permitted {
+                    permit,
+                    target,
+                    protocol: PermitVariant::Grpc,
+                }
             }
         };
 
@@ -394,47 +409,72 @@ fn apply_grpc_filters<B>(route: &grpc::Policy, req: &mut ::http::Request<B>) ->
 
 // === impl Permitted ===
 
-/// An authorized `T`-typed target can produce `P`-typed parameters.
-impl<T, P> svc::Param<P> for Permitted<T>
+impl<T> svc::Param<Remote<ServerAddr>> for Permitted<T>
+where
+    T: svc::Param<Remote<ServerAddr>>,
+{
+    fn param(&self) -> Remote<ServerAddr> {
+        self.target.param()
+    }
+}
+
+impl<T> svc::Param<ConditionalServerTls> for Permitted<T>
 where
-    T: svc::Param<P>,
+    T: svc::Param<ConditionalServerTls>,
 {
-    fn param(&self) -> P {
-        self.target_ref().param()
+    fn param(&self) -> ConditionalServerTls {
+        self.target.param()
+    }
+}
+
+impl<T> svc::Param<HttpRoutePermit> for Permitted<T> {
+    fn param(&self) -> HttpRoutePermit {
+        self.permit_ref().clone()
+    }
+}
+
+impl<T> svc::Param<PermitVariant> for Permitted<T> {
+    fn param(&self) -> PermitVariant {
+        self.variant()
+    }
+}
+
+impl<T> svc::Param<RouteLabels> for Permitted<T> {
+    fn param(&self) -> RouteLabels {
+        self.route_labels()
     }
 }
 
 impl<T> Permitted<T> {
     /// Returns a reference to the [`HttpRoutePermit`] authorizing this `T`.
     pub fn permit_ref(&self) -> &HttpRoutePermit {
-        match self {
-            Self::Grpc { permit, .. } => permit,
-            Self::Http { permit, .. } => permit,
-        }
+        &self.permit
+    }
+
+    /// Returns the [`PermitVariant`] of the permitting policy.
+    pub fn variant(&self) -> PermitVariant {
+        self.protocol
     }
 
     /// Returns a reference to the underlying `T`-typed target.
     pub fn target_ref(&self) -> &T {
-        match self {
-            Self::Grpc { target, .. } => target,
-            Self::Http { target, .. } => target,
-        }
+        &self.target
     }
 
     /// Consumes this permitted `T`, returning the inner `T`.
     pub fn into_target(self) -> T {
-        match self {
-            Self::Grpc { target, .. } => target,
-            Self::Http { target, .. } => target,
-        }
+        self.target
     }
 
     /// Consumes this permitted `T`, returning the `T` and its permit.
     pub fn into_parts(self) -> (T, HttpRoutePermit) {
-        match self {
-            Self::Grpc { target, permit } => (target, permit),
-            Self::Http { target, permit } => (target, permit),
-        }
+        let Self {
+            permit,
+            protocol: _,
+            target,
+        } = self;
+
+        (target, permit)
     }
 
     /// Returns the [`RouteLabels`] from the underlying permit.
