policy: Serve EgressNetwork responses #13206

zaharidichev · 2024-10-21T14:02:39Z

This PR adds a few notable changes associated with the egress functionality of Linkerd:

EgressNetwork objects are indexed into the outbound index
outbound policy lookups are classfieid as either in-cluster or egress based on the ip:port combination
TCPRoute, TLSRoute, GRPCRoute and HTTPRoute attachments are reflected for both EgressNetwork and Service targets
the default traffic policy for EgressNetwork is honored by returning the appropriate default (failure/success) routes for all protocols

Note that this PR depends on an unreleased version of the linkerd2-proxy-api repo.

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

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

zaharidichev · 2024-10-21T15:31:09Z

policy-controller/k8s/index/src/outbound/index/metrics.rs

@@ -33,7 +33,7 @@ impl Collector for Instrumented {
            None,
            MetricType::Gauge,
        )?;
-        let service_infos = ConstGauge::new(this.service_info.len() as u32);


Will come back and address metrics correctness in a follow-up if that is a non-blocker for now.

adleong · 2024-10-21T23:56:35Z

policy-controller/core/src/outbound.rs

+pub enum Kind {
+    EgressNetwork {
+        original_dst: SocketAddr,
+        traffic_policy: TrafficPolicy,


I'm surprised to see TrafficPolicy here in the discover target. I think this means that the TrafficPolicy is looked up once at the beginning of the discovery lookup but then never updated. If the EgressNetwork resource is edited to change the TrafficPolicy, I don't think the discovery watches will see that change.

This actually leads to a more difficult question about EgressNetwork mutability and ip address.

For services, a service's IP address will never change for the life of that services which means we can look up the service by IP once at the beginning of the discovery stream and never again.

For EgressNetworks, on the other hand, they encompass a range of IP addresses and that range can change if the EgressNetwork resource is edited. What happens when a discovery stream is started on an IP address that is in an EgressNetwork, but then the EgressNetwork is edited to no longer include that IP in it's network range? Or vice versa, what if an EgressNetwork is edited to include an IP address that was used for an already started discovery stream?

It really seems like we need to take a different approach for EgressNetworks compared to the approach we have for services in the outbound index. Perhaps we can take advantage of the fact that cluster networks are immutable. When a discovery lookup comes in, we can look at if it's in the cluster network and send it to the service index or if it's out of the cluster network, then we can send it to the egress index.

The egress index will likely need to have watches indexed by ip address so that we can update the appropriate watches whenever an EgressNetwork resource changes and may encompass watches that it did not before or stop encompassing watches that it used to.

Good points.. Will think about it a bit more.

adleong · 2024-10-22T00:20:05Z

policy-controller/grpc/src/outbound.rs

+        // if we have no explicit routes attached to the parent, always attempt protocol detection
+        if no_explicit_routes(&outbound) {


How does this compare to the current approach of just starting with the most specific route type and falling back to less and less specific? ie if there are grpcRoutes, use those, else if there are httproutes, use those, else if there are tls routes, use those, else if there are tcp routes, use those, ELSE do http detect.

good point. My approach is essentially the same but more complicated. Will incorporate your suggestion

adleong · 2024-10-22T00:20:56Z

policy-controller/grpc/src/outbound.rs

-fn timestamp_then_name<LM, LR, RM, RR>(
-    (left_id, left_route): &(GroupKindNamespaceName, OutboundRoute<LM, LR>),
-    (right_id, right_route): &(GroupKindNamespaceName, OutboundRoute<RM, RR>),
+fn timestamp_then_name<L: Route, R: Route>(


I think now we never compare routes of different types, so we can simplify this to only use one type parameter.

This will enforce, at the type level, that routes cannot be mixed.

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

adleong

To help myself follow all the logic here and see how it all fits together, I drew a picture as I went.

adleong · 2024-10-23T19:55:13Z

policy-controller/k8s/index/src/outbound/index.rs

+        }
+    }
+
+    fn update_resource(


I think this function needs to accept the traffic policy so that it can update its ParentMeta if the traffic policy changes.

good catch, will add a test as well

adleong · 2024-10-23T22:20:47Z

policy-controller/core/src/outbound/policy.rs

+pub enum OutboundPolicyKind {
+    Fallback(SocketAddr),
+    Resource(ResourceOutboundPolicy),
+}


It took me a long time to fully understand the mapping of OutboundPolicy to OutboundPolicyKind and then finally to proxy-api protobuf. It seems to me that OutboundPolicy + orig_dst already contains enough information to construct the proxy-api response (since the OutboundPolicy contains ParentMeta which stores TrafficPolicy).

Can we simplify this all by skipping the step of transforming OutboundPolicy into OutboundPolicyKind and instead just directly transform OutboundPolicy into proxy-api response? This will require passing in the orig_dst as supplemental information, but I think that's okay.

To put this another way, OutboundPolicyKind::Fallback and OutboundPolicyKind::Resource will never be co-mingled in a single stream. Fallback will only be created if the OutboundDiscoverTarget is Fallback and Resource will only be created if the OutboundDiscoverTarget is Resource. So I think these can be total separate codepaths.

adleong · 2024-10-23T22:24:27Z

policy-controller/core/src/outbound/policy.rs

+// ParentMeta carries information resource-specific
+// information about the parent to which outbound policy
+// is associated.


If I understand correctly, the purpose of this is that it's the information necessary to construct the default backend (minus the orig_dst since that needs to be filled in outside of the index). It might be helpful to have a comment explaining that that is it's purpose.

I might consider calling this something other than "Meta" too since, metadata makes me think of the resource metadata (i.e. group, kind, name)

adleong · 2024-10-23T22:26:10Z

policy-controller/core/src/outbound/target.rs

+#[derive(Clone, Debug)]
+pub enum OutboundDiscoverTarget {
+    Resource(ResourceTarget),
+    Fallback(SocketAddr),


A name like "External" might be more descriptive than Fallback.

adleong · 2024-10-23T22:42:37Z

policy-controller/grpc/src/outbound.rs

+        }),
+
+        protocol: Some(outbound::ProxyProtocol {
+            kind: Some(outbound::proxy_protocol::Kind::Opaque(


This means that for external targets that are not covered by an EgressNetwork, we'll return Opaque.

Does this differ from the current behavior? I assume that today we return NotFound for these and then the proxy uses the profile resolution. Does this amount to being Detect? Should we return Detect here instead of Opaque?

So what happens today is:

for addresses that we cannot find here and are potentially in the cluster we return NotFound. The proxy may then use profiles information if available. So this behavior is preserved with this logic.

for addresses that are outside of the cluster we return NotFound and the proxy will not be able to find a profile. So what happens is that we will synthesize a default Detect policy that uses forward backends. So here we need to do the same as opposed to returning an opaque.

So you are right. Good catch. For context, here is where we do the synthesizing in the proxy: https://github.com/linkerd/linkerd2-proxy/blob/ef779d09191713c5f25682fc7a61212bb6752d21/linkerd/app/outbound/src/discover.rs#L144

adleong · 2024-10-23T22:47:24Z

policy-controller/grpc/src/outbound/tls.rs

+        ),
+    };
+
+    // This encoder sets deprecated timeouts for older proxies.


I don't think this comment is accurate for TLSRoute

adleong · 2024-10-23T22:47:42Z

policy-controller/grpc/src/outbound/tcp.rs

+        ),
+    };
+
+    // This encoder sets deprecated timeouts for older proxies.


I don't think this comment is accurate for TCPRoute

adleong · 2024-10-23T22:50:53Z

policy-controller/k8s/index/src/outbound/index.rs

+    // holds information about resources. currently EgressNetworks and Services
+    resource_info: HashMap<ResourceRef, ResourceInfo>,
+    cluster_networks: Vec<linkerd_k8s_api::Cidr>,
+    fallback_polcy_tx: watch::Sender<()>,


Might be worth some comments on the semantics of this Sender and what it's purpose is.

adleong · 2024-10-23T23:03:38Z

policy-test/src/lib.rs

@@ -435,7 +435,7 @@ where
        drop(_tracing);
    }

-    if std::env::var("POLICY_TEST_NO_CLEANUP").is_err() {
+    if std::env::var("POLICY_TEST_NO_CLEANUP").is_ok() {


is this right? it seems backwards. POLICY_TEST_NO_CLEANUP being ok means we should NOT delete

adleong · 2024-10-23T23:03:54Z

policy-test/tests/outbound_api_linkerd.rs

@@ -26,6 +26,7 @@ async fn service_does_not_exist() {
        let mut policy_api = grpc::OutboundPolicyClient::port_forwarded(&client).await;
        let rsp = policy_api.watch(&ns, &svc, 4191).await;

+        println!("{:?}", rsp);


stray println

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

adleong · 2024-10-24T20:42:24Z

policy-controller/core/src/outbound/policy.rs

+/// specific metadata that is used when putting together the final
+/// policy response.
+#[derive(Clone, Debug, PartialEq)]
+pub enum ResourceOutboundPolicy {


I think this type is unused and can be removed.

adleong · 2024-10-24T20:45:03Z

policy-controller/grpc/src/outbound.rs

+    // This encoder sets deprecated timeouts for older proxies.
+    #![allow(deprecated)]


I think this can be removed if we use ..Default::default() to fill in default values for request_timeout instead of setting to None explicitly.

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

serve EgressNetwork responses

130050d

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

zaharidichev requested a review from a team as a code owner October 21, 2024 14:02

zaharidichev commented Oct 21, 2024

View reviewed changes

adleong reviewed Oct 22, 2024

View reviewed changes

DavidMcLaughlin assigned adleong Oct 22, 2024

handle egress network changes

4144b19

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

zaharidichev force-pushed the zd/egress-network-policy-3 branch from bf26d69 to 4144b19 Compare October 23, 2024 14:38

zaharidichev added 2 commits October 23, 2024 16:27

simplify types

903005e

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

fix not found integration test

c288874

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

adleong reviewed Oct 23, 2024

View reviewed changes

zaharidichev added 4 commits October 24, 2024 07:50

address feedback

a53f573

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

remove PolicyKind type

501c646

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

update proxy api

8b29501

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

switching test

3e1573a

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

adleong approved these changes Oct 24, 2024

View reviewed changes

zaharidichev added 3 commits October 27, 2024 18:34

more feedback

1ffaa3b

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

depend on proxy-api c5648ae2a1e405cc6b8aca20522356ebdf20f1ea

e0b58fe

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

clippy

06e2d3a

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

zaharidichev merged commit b0f087c into main Oct 27, 2024
42 checks passed

zaharidichev deleted the zd/egress-network-policy-3 branch October 27, 2024 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

policy: Serve EgressNetwork responses #13206

policy: Serve EgressNetwork responses #13206

zaharidichev commented Oct 21, 2024 •

edited

Loading

zaharidichev Oct 21, 2024

adleong Oct 21, 2024

adleong Oct 22, 2024

adleong Oct 22, 2024

zaharidichev Oct 22, 2024

adleong Oct 22, 2024

zaharidichev Oct 22, 2024

adleong Oct 22, 2024

adleong Oct 22, 2024

adleong left a comment

adleong Oct 23, 2024

zaharidichev Oct 24, 2024

adleong Oct 23, 2024

adleong Oct 23, 2024

adleong Oct 23, 2024

adleong Oct 23, 2024

adleong Oct 23, 2024

adleong Oct 23, 2024

zaharidichev Oct 24, 2024

adleong Oct 23, 2024

adleong Oct 23, 2024

adleong Oct 23, 2024

adleong Oct 23, 2024

zaharidichev Oct 24, 2024

adleong Oct 23, 2024

adleong Oct 24, 2024

adleong Oct 24, 2024

		// if we have no explicit routes attached to the parent, always attempt protocol detection
		if no_explicit_routes(&outbound) {

		// This encoder sets deprecated timeouts for older proxies.
		#![allow(deprecated)]

policy: Serve EgressNetwork responses #13206

policy: Serve EgressNetwork responses #13206

Conversation

zaharidichev commented Oct 21, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adleong left a 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

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

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

Choose a reason for hiding this comment

zaharidichev commented Oct 21, 2024 •

edited

Loading