address first review pass

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

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

@@ -45,15 +45,15 @@ spec:
  that does not match any explicit route resources associated
  with an instance of this object. The values that are allowed
  currently are:
- - AllowAll - permits all traffic, even if it has not been
+ - Allow - permits all traffic, even if it has not been
  explicitly described via attaching an xRoute
  resources.
- - DenyAll - blocks all traffic that has not been described via
+ - Deny - blocks all traffic that has not been described via
  attaching an xRoute resource.
  type: string
  enum:
- - AllowAll
- - DenyAll
+ - Allow
+ - Deny
  networks:
  type: array
  items:

charts/linkerd-crds/values.yaml

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

policy-controller/k8s/api/src/policy/egress_network.rs

@@ -20,8 +20,8 @@ pub struct EgressNetworkSpec {
 
 #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize, JsonSchema)]
 pub enum TrafficPolicy {
- AllowAll,
- DenyAll,
+ Allow,
+ Deny,
 }
 
 #[derive(Clone, Debug, PartialEq, Deserialize, Serialize, JsonSchema)]

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

@@ -116,10 +116,9 @@ struct EgressNetworkRef {
 
 impl EgressNetworkRef {
  fn is_accepted(&self) -> bool {
- self.status_conditions.iter().any(|c| {
- c.type_ == conditions::ACCEPTED.to_string()
- && c.status == cond_statuses::STATUS_TRUE.to_string()
- })
+ self.status_conditions
+ .iter()
+ .any(|c| c.type_ == *conditions::ACCEPTED && c.status == *cond_statuses::STATUS_TRUE)
  }
 }
 
@@ -1306,14 +1305,7 @@ fn eq_time_insensitive_route_parent_statuses(
  left.iter().zip(right.iter()).all(|(l, r)| {
  l.parent_ref == r.parent_ref
  && l.controller_name == r.controller_name
- && l.conditions.len() == r.conditions.len()
- && l.conditions.iter().zip(r.conditions.iter()).all(|(l, r)| {
- l.message == r.message
- && l.observed_generation == r.observed_generation
- && l.reason == r.reason
- && l.status == r.status
- && l.type_ == r.type_
- })
+ && eq_time_insensitive_conditions(&l.conditions, &r.conditions)
  })
 }
 
@@ -1326,11 +1318,10 @@ fn eq_time_insensitive_conditions(
  }
 
  left.iter().zip(right.iter()).all(|(l, r)| {
- let result = l.message == r.message
+ l.message == r.message
  && l.observed_generation == r.observed_generation
  && l.reason == r.reason
  && l.status == r.status
- && l.type_ == r.type_;
- result
+ && l.type_ == r.type_
  })
 }

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

@@ -1,36 +1,35 @@
 use super::RouteRef;
 use crate::{resource_id::NamespaceGroupKindName, routes};
 
-use ahash::AHashSet as HashSet;
 use linkerd_policy_controller_k8s_api::{gateway as k8s_gateway_api, Resource};
 
 // This method determines whether a parent that a route attempts to
 // 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>(
- mut existing_routes: impl Iterator<Item = (&'p NamespaceGroupKindName, &'p RouteRef)>,
+ 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 more_specific_routes: HashSet<_> = if *candidate_kind == grpc_kind {
-  vec![]
- } else if *candidate_kind == http_kind {
- vec![grpc_kind]
- } else if *candidate_kind == tls_kind {
- vec![grpc_kind, http_kind]
- } else {
- vec![grpc_kind, http_kind, tls_kind]
- }
- .into_iter()
- .collect();
-
- existing_routes.any(|(id, route)| {
- more_specific_routes.contains(&id.gkn.kind) && route.parents.contains(parent_ref)
+ 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
+ }
  })
 }
 

policy-controller/k8s/status/src/tests/egress_network.rs

@@ -49,7 +49,7 @@ fn egress_network_with_no_networks_specified() {
  },
  spec: linkerd_k8s_api::EgressNetworkSpec {
  networks: None,
- traffic_policy: linkerd_k8s_api::TrafficPolicy::AllowAll,
+ traffic_policy: linkerd_k8s_api::TrafficPolicy::Allow,
  },
  status: None,
  };
@@ -111,7 +111,7 @@ fn egress_network_with_nonoverlapping_networks_specified() {
  "fd00::/8".parse().unwrap(),
  ]),
  }]),
- traffic_policy: linkerd_k8s_api::TrafficPolicy::AllowAll,
+ traffic_policy: linkerd_k8s_api::TrafficPolicy::Allow,
  },
  status: None,
  };
@@ -171,7 +171,7 @@ fn egress_network_with_overlapping_networks_specified() {
  "192.168.0.0/16".parse().unwrap(),
  ]),
  }]),
- traffic_policy: linkerd_k8s_api::TrafficPolicy::AllowAll,
+ traffic_policy: linkerd_k8s_api::TrafficPolicy::Allow,
  },
  status: None,
  };

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

@@ -81,7 +81,7 @@ fn make_egress_network(
  "fd00::/8".parse().unwrap(),
  ]),
  }]),
- traffic_policy: linkerd_k8s_api::TrafficPolicy::AllowAll,
+ traffic_policy: linkerd_k8s_api::TrafficPolicy::Allow,
  },
  status: Some(linkerd_k8s_api::EgressNetworkStatus {
  conditions: vec![condition],

policy-test/tests/admit_egress_networks.rs

@@ -13,7 +13,7 @@ async fn accepts_valid() {
  ..Default::default()
  },
  spec: EgressNetworkSpec {
- traffic_policy: TrafficPolicy::AllowAll,
+ traffic_policy: TrafficPolicy::Allow,
  networks: Some(vec![
  Network {
  cidr: "10.1.0.0/24".parse().unwrap(),
@@ -39,7 +39,7 @@ async fn rejects_empty_networks() {
  ..Default::default()
  },
  spec: EgressNetworkSpec {
- traffic_policy: TrafficPolicy::AllowAll,
+ traffic_policy: TrafficPolicy::Allow,
  networks: Some(Default::default()),
  },
  status: None,