policy: refactor opaq stack to use client-policy #3306

zaharidichev · 2024-10-29T18:20:46Z

This work (based on the early sketch seen here) changes the opaq routing stack to use not only profiles routing but to consider client policy responses as well. The way this is done is that we use the opaq:Routes type to express the routing information our stack contains. This type is geared towards the client-policy API. Whenever we need to use the profiles response, we simply translate its contents to a client-policy one.

Notable changes in this PR are:

we introduce an OpaqSidecar type that holds opaq::Routes objects which are routes that have been constructed from either profiles or policy response
during the construction of the OpaqSidecar we consider profiles responses only if there are extra targets in the response (in order to support TrafficSplit functionality). In any other case we prefer the policy response.

Signed-off-by: Zahari Dichev [email protected]

Signed-off-by: Zahari Dichev <[email protected]>

olix0r · 2024-10-30T15:08:26Z

linkerd/app/outbound/Cargo.toml

@@ -45,6 +45,7 @@ linkerd-proxy-client-policy = { path = "../../proxy/client-policy", features = [
 ] }
 linkerd-retry = { path = "../../retry" }
 linkerd-tls-route = { path = "../../tls/route" }
+linkerd-opaq-route = { path = "../../opaq-route" }


nit: move above linkerd-proxy-client-policy (sorting)

olix0r · 2024-10-30T15:38:02Z

linkerd/opaq-route/src/lib.rs

@@ -0,0 +1,49 @@
+//! An TLS route matching library for Linkerd to support the TLSRoute


Suggested change

//! An TLS route matching library for Linkerd to support the TLSRoute

//! A TCP route matching library for Linkerd to support the TCPRoute

olix0r · 2024-10-30T15:38:38Z

linkerd/opaq-route/src/match.rs

+
+/// Matches TCP sessions. For now, this is a placeholder
+#[derive(Clone, Debug, Default, Hash, PartialEq, Eq)]
+pub struct MatchSession(());


I suspect this is coming from the TLSRoute type--do we expect TCP routes to use "session" terminology?

Should we think of a better name. MatchTCPConnection ? Or try to get rid of this whole machinery here. And yes, this is coming from the TLSRoute type.

My suggestion is that we remove the parts of this that aren't really coherent for opaque routing -- since all of our policy is already hung off of an ip:port, there's nothing else worth matching on; and this means that we can only ever expect to support a single route per policy.

My branch reverts to using an Option and denying policies that return more than one route. We may need to update the control plane to only return the first route in its list.

This makes sense indeed. Will merge and update control plane to return only one route always.

linkerd/app/outbound/src/opaq/concrete.rs

Signed-off-by: Zahari Dichev <[email protected]>

olix0r · 2024-11-04T22:59:08Z

linkerd/proxy/client-policy/src/lib.rs

                    // TODO(eliza): eventually, can we configure the opaque
                    // policy to fail conns?
-                    policy: None,
+                    routes: std::sync::Arc::new([]),


We can remove this TODO now, right?

olix0r · 2024-11-04T23:03:44Z

linkerd/proxy/client-policy/src/lib.rs

                    // TODO(eliza): eventually, can we configure the opaque
                    // policy to fail conns?
-                    policy: None,
+                    routes: std::sync::Arc::new([]),
                },


Similarly, this TODO is addressed.

olix0r · 2024-11-05T00:53:52Z

linkerd/app/outbound/src/opaq/logical.rs

+                .push_on_service(svc::layer::mk(move |concrete: N| {
+                    svc::stack(concrete.clone())
+                        .push(router::Router::layer())
+                        .push(svc::NewMapErr::layer_from_target::<LogicalError, _>())
+                        .arc_new_clone_tcp()
+                        .into_inner()
+                }))


Can simplify this:

Suggested change

.push_on_service(svc::layer::mk(move |concrete: N| {

svc::stack(concrete.clone())

.push(router::Router::layer())

.push(svc::NewMapErr::layer_from_target::<LogicalError, _>())

.arc_new_clone_tcp()

.into_inner()

}))

.push_on_service(router::Router::layer())

.push_on_service(svc::NewMapErr::layer_from_target::<LogicalError, _>())

// Rebuild the inner router stack every time the watch changes.

.push(svc::NewSpawnWatch::<Routes, _>::layer_into::<

router::Router<T>,

>())

.arc_new_clone_tcp()

olix0r · 2024-11-05T05:21:58Z

linkerd/app/outbound/src/opaq/concrete.rs

+        // we are trying to preserve the original metrics format here.
+        // for that purpose we utilize the logical address that will be
+        // present only if we have used profiles for routing. This means:
+        //
+        // 1. If we are using policy logical and concrete will be the same
+        // 2. If we are using profiles we are doing so because the profiles
+        //    contains traffic split data. In this case the logical and
+        //    concrete are potentially different based on the backend choice.
+        let logical = match bal.parent.param() {
+            Some(profiles::LogicalAddr(addr)) => addr.to_string(),
+            None => bal.addr.to_string(),
+        };
+
        self.metrics(&ConcreteLabels {
-            logical: bal.logical.to_string().into(),
-            concrete: bal.concrete.to_string().into(),
+            logical: logical.into(),
+            concrete: bal.addr.to_string().into(),
        })


I started a branch but haven't got it sensible yet, but we also need to consider the case where a balancer is provided by policy -- in that case we want to include parent and backend references (as we do in HTTP). The logical and concrete values cannot be removed due to backwards compatibility, but we should provide reference-oriented labels in the policy case to be consistent.

@ver This makes total sense. I have put together a branch that does that. So in my proposal, for the two cases we balance we get the following results. I do not see a point to special casing profiles. We can still do our best to populate refs data based on the logical addrs that we get from the profiles api

The case when we use the policy response when we target a Service

outbound_tcp_balancer_queue_requests_total{ parent_group="core" parent_kind="Service" parent_namespace="mixed-env" parent_name="legacy-app-cluster" parent_port="80" parent_section_name="" backend_group="core" backend_kind="Service" backend_namespace="mixed-env" backend_name="legacy-app-cluster" backend_port="80" backend_section_name="" logical="legacy-app-cluster.mixed-env.svc.cluster.local:80" concrete="legacy-app-cluster.mixed-env.svc.cluster.local:80" } 1

The case when we use the profiles response because a TrafficSplit is configured

outbound_tcp_balancer_queue_requests_total{ parent_group="core" parent_kind="Service" parent_namespace="trafficsplit-sample" parent_name="backend-svc" parent_port="8080" parent_section_name="" backend_group="core" backend_kind="Service" backend_namespace="trafficsplit-sample" backend_name="failing-svc" backend_port="8080" backend_section_name="" logical="backend-svc.trafficsplit-sample.svc.cluster.local:8080" concrete="failing-svc.trafficsplit-sample.svc.cluster.local:8080" } 2

Because the opaq route mocks its selection logic, the first route returned by the API will always be used. This means that all subsequent routes are incoherent and cannot be selected. There's no value in preserving incoherent states in the proxy, so it's preferable to initially require that the control plane only returns one opaq route. This change removes some of the unused matching types and simplifies the opaq router to only handle one route.

Signed-off-by: Zahari Dichev <[email protected]>

policy: use client-policy responses in opaq stack

dc5e710

Signed-off-by: Zahari Dichev <[email protected]>

zaharidichev mentioned this pull request Oct 29, 2024

policy: use client-policy responses in opaq stack #3304

Closed

olix0r reviewed Oct 30, 2024

View reviewed changes

zaharidichev added 3 commits November 1, 2024 11:23

remove opaq:LogicalAddr (#3309)

b58c169

Signed-off-by: Zahari Dichev <[email protected]>

use ServerAddr in place of Addr

00bf497

Signed-off-by: Zahari Dichev <[email protected]>

nits

7831eac

Signed-off-by: Zahari Dichev <[email protected]>

olix0r self-assigned this Nov 4, 2024

olix0r reviewed Nov 4, 2024

View reviewed changes

olix0r reviewed Nov 5, 2024

View reviewed changes

olix0r and others added 2 commits November 5, 2024 09:34

simplify logical

29f43b9

Signed-off-by: Zahari Dichev <[email protected]>

zaharidichev mentioned this pull request Nov 5, 2024

policy: limit TCPRoute to one per policy response linkerd/linkerd2#13272

Open

zaharidichev changed the title ~~policy: refactor opaq stack to use use client-policy~~ policy: refactor opaq stack to use client-policy Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

policy: refactor opaq stack to use client-policy #3306

policy: refactor opaq stack to use client-policy #3306

zaharidichev commented Oct 29, 2024

olix0r Oct 30, 2024

olix0r Oct 30, 2024

olix0r Oct 30, 2024

zaharidichev Oct 30, 2024

olix0r Nov 4, 2024

zaharidichev Nov 5, 2024

olix0r Nov 4, 2024 •

edited

Loading

olix0r Nov 4, 2024 •

edited

Loading

olix0r Nov 5, 2024

olix0r Nov 5, 2024

zaharidichev Nov 5, 2024 •

edited

Loading

		@@ -0,0 +1,49 @@
		//! An TLS route matching library for Linkerd to support the TLSRoute

	//! An TLS route matching library for Linkerd to support the TLSRoute
	//! A TCP route matching library for Linkerd to support the TCPRoute

policy: refactor opaq stack to use client-policy #3306

Are you sure you want to change the base?

policy: refactor opaq stack to use client-policy #3306

Conversation

zaharidichev commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olix0r Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

olix0r Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaharidichev Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

The case when we use the policy response when we target a Service

The case when we use the profiles response because a TrafficSplit is configured

olix0r Nov 4, 2024 •

edited

Loading

olix0r Nov 4, 2024 •

edited

Loading

zaharidichev Nov 5, 2024 •

edited

Loading

The case when we use the profiles response because a `TrafficSplit` is configured