Add `EgressNetwork` and routes statuses #13181

zaharidichev · 2024-10-14T11:27:23Z

This PR adds an EgressNetwork CRD, which purpose is to describe networks that are external to the cluster.
In addition to that it also adds TLSRoute and TCPRoute gateway api CRDs.

Most of the work in this change is focused on introducing these CRDs and correctly setting their status based on route specificity rules described in: https://gateway-api.sigs.k8s.io/geps/gep-1426/#route-types.

Notable changes include:

ability to attach TCP and TLS routes to both EgressNetworks and Service objects
implemented conflict resolutions between routes
admission validation on the newly introduced resources
module + integration tests

Signed-off-by: Zahari Dichev zaharidichev@gmail.com

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

adleong · 2024-10-15T21:42:19Z

charts/linkerd-crds/templates/policy/egress-network.yaml

+                - AllowAll
+                - DenyAll


Naming bikeshed: I think "Allow" and "Deny" are better here. I don't think the word "All" is semantically meaningful, especially considering that the behavior can be changed by matching an attached route.

i.e. the "Deny" policy doesn't deny ALL traffic, it denies traffic that doesn't match any attached routes.

adleong · 2024-10-15T21:51:27Z

charts/linkerd-crds/values.yaml

@@ -1 +1,3 @@
 enableHttpRoutes: true
+enableTlsRoutes: true
+enableTcpRoutes: true


missing trailing newline

adleong · 2024-10-15T21:53:05Z

charts/linkerd-crds/values.yaml

@@ -1 +1,3 @@
 enableHttpRoutes: true
+enableTlsRoutes: true


it's a little weird that enableHttpRoutes controls both the HTTPRoute and GRPCRoute CRDs but then TLSRoute and TCPRoute each get their own value. but this is probably fine, more flexibility probably doesn't hurt.

adleong · 2024-10-15T22:31:09Z

policy-controller/k8s/status/src/routes.rs

    UnknownKind,
 }

 #[derive(Clone, Eq, PartialEq)]
 pub enum BackendReference {
    Service(ResourceId),
+    EgressNetwork(ResourceId),


I don't think an EgressNetwork can be a backend.

I would say an EgressNetwork has to be able to be a backend. The reason for that is that if you look at the TCPRoute and TLSRoute CRD definitions, you will notice that they mandate that there should be at least 1 rule with at least 1 backend.

If that is a requirement and a user wants to attach a TCPRoute to an EgressNetwork, then what backend would it specify explicitly? Well, it has to be the EgressNetwork itself then. And an EgressNetwork backend in our proxy world will build a Forward stack?

I would argue that... it does not make sense for a route that is attached to a Service to have an EgressNetwork backend, although I cannot come up with a strong argument apart from.. "it does not make sense to do that". But in general.. I do not see a way around having an EgressNetwork work as a backend for EgressNetwork parents.

Does that make sense?

For context: https://github.com/kubernetes-sigs/gateway-api/blob/main/config/crd/experimental/gateway.networking.k8s.io_tcproutes.yaml#L436

adleong · 2024-10-15T22:48:26Z

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

@@ -945,3 +1316,21 @@ fn eq_time_insensitive(
            })
    })
 }
+
+fn eq_time_insensitive_conditions(


Can we use this function in the above eq_time_insensitive_route_parent_statuses to avoid repetition?

adleong · 2024-10-15T22:50:58Z

policy-controller/k8s/status/src/index/conflict.rs

+// attach to has any routes attached that are in conflict with the one
+// that we are about to attach. This is done following the logs outlined in:
+// https://gateway-api.sigs.k8s.io/geps/gep-1426/#route-types
+pub(super) fn parent_has_conflicting_routes<'p>(


TIOLI but I golfed this down a bit. Same idea but with an inline if instead of building a HashSet

pub(super) fn parent_has_conflicting_routes<'p>( existing_routes: impl Iterator<Item = (&'p NamespaceGroupKindName, &'p RouteRef)>, parent_ref: &routes::ParentReference, candidate_kind: &str, ) -> bool { let grpc_kind = k8s_gateway_api::GrpcRoute::kind(&()); let http_kind = k8s_gateway_api::HttpRoute::kind(&()); let tls_kind = k8s_gateway_api::TlsRoute::kind(&()); let tcp_kind = k8s_gateway_api::TcpRoute::kind(&()); let mut siblings = existing_routes.filter(|(_, route)| route.parents.contains(parent_ref)); siblings.any(|(id, _sibling)| { if *candidate_kind == grpc_kind { false } else if *candidate_kind == http_kind { id.gkn.kind == grpc_kind } else if *candidate_kind == tls_kind { id.gkn.kind == grpc_kind || id.gkn.kind == http_kind } else if *candidate_kind == tcp_kind { id.gkn.kind == grpc_kind || id.gkn.kind == http_kind || id.gkn.kind == tls_kind } else { false } }) }

adleong · 2024-10-15T22:53:10Z

policy-controller/k8s/status/src/routes.rs

@@ -73,8 +91,32 @@ impl BackendReference {
                namespace.to_string(),
                backend_ref.name.clone(),
            ))
+        } else if linkerd_k8s_api::httproute::backend_ref_targets_kind::<


I don't think this is valid.

left a more detailed comment on the matter here: #13181 (comment)

adleong · 2024-10-15T23:02:00Z

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

+impl EgressNetworkRef {
+    fn is_accepted(&self) -> bool {
+        self.status_conditions.iter().any(|c| {
+            c.type_ == conditions::ACCEPTED.to_string()


some clippy warnings here

adleong · 2024-10-15T23:32:30Z

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

+            && l.observed_generation == r.observed_generation
+            && l.reason == r.reason
+            && l.status == r.status
+            && l.type_ == r.type_;


some clippy warnings here

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

adleong · 2024-10-18T01:14:44Z

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

+            routes::BackendReference::EgressNetwork(egress_net) => {
+                self.egress_networks.contains_key(egress_net)
+            }


Is this where we should validate that the EgressNetwork backend must be equal to the parent?

Yes, correct. Except for the fact that we now need to compute the backend status per parent because the backend status is dependent on the parent type. Namely, you can have an EgressNetwork parent with a Service backend but not the other way around.

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

adleong

LGTM modulo stray println and clippy wanring.

adleong · 2024-10-18T17:39:26Z

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

+                    }
+                }
+                routes::BackendReference::EgressNetwork(egress_net) => {
+                    println!("backend is a net");


stray println

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

implement EgressNetwork, TCPRoute and TLSRoute status setting

c1a1e33

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev requested a review from a team as a code owner October 14, 2024 11:27

update golden files

90cce50

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev force-pushed the zd/egress-network-statuses branch from ebffad9 to 90cce50 Compare October 14, 2024 14:18

adleong reviewed Oct 15, 2024

View reviewed changes

address first review pass

bd1d86f

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

adleong reviewed Oct 18, 2024

View reviewed changes

improve validation

4603580

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

adleong approved these changes Oct 18, 2024

View reviewed changes

remove println and fix clippy

963ba46

Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>

zaharidichev merged commit 3e2f31d into main Oct 19, 2024
46 checks passed

zaharidichev deleted the zd/egress-network-statuses branch October 19, 2024 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `EgressNetwork` and routes statuses #13181

Add `EgressNetwork` and routes statuses #13181

zaharidichev commented Oct 14, 2024 •

edited

Loading

adleong Oct 15, 2024

adleong Oct 15, 2024

adleong Oct 15, 2024

adleong Oct 15, 2024

zaharidichev Oct 16, 2024

adleong Oct 15, 2024

zaharidichev Oct 16, 2024

adleong Oct 15, 2024

adleong Oct 15, 2024

zaharidichev Oct 16, 2024

adleong Oct 15, 2024

adleong Oct 15, 2024

adleong Oct 18, 2024

zaharidichev Oct 18, 2024

adleong left a comment

adleong Oct 18, 2024

Add EgressNetwork and routes statuses #13181

Add EgressNetwork and routes statuses #13181

Conversation

zaharidichev commented Oct 14, 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

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

Add `EgressNetwork` and routes statuses #13181

Add `EgressNetwork` and routes statuses #13181

zaharidichev commented Oct 14, 2024 •

edited

Loading