Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

policy: limit TCPRoute to one per policy response #13272

Merged
merged 1 commit into from
Nov 6, 2024
Merged

policy: limit TCPRoute to one per policy response #13272

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions policy-controller/grpc/src/outbound/tcp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@ use linkerd_policy_controller_core::{
};
use std::net::SocketAddr;

// Since there is no way to do real selection on a TCPRoute, we only allow 1
const MAXIMUM_ALLOWED_TCP_ROUTES: usize = 1;

pub(crate) fn protocol(
default_backend: outbound::Backend,
routes: impl Iterator<Item = (GroupKindNamespaceName, TcpRoute)>,
parent_info: &ParentInfo,
original_dst: Option<SocketAddr>,
) -> outbound::proxy_protocol::Kind {
let mut routes = routes
.take(MAXIMUM_ALLOWED_TCP_ROUTES)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the routes sorted in any particular way at this point when we take the first one? Is there a deterministic way to know which TCP route will take affect and which ones will be ignored?

If there are multiple TCP routes attached, should we be setting a RouteConflict and/or Accepted=false status on them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have to consult more of the impl to tell you how/where this is happening, but the routes must be sorted according to preference (i.e. according to the route sorting rules dictated by the gateway API--naming, creation timestamp, etc). This change, as it is, simply encodes what the proxy would already do: use the first matching route. The proxy has no concept of conflicts or accepted=false, so all of that ordering must be handled in the policy controller.

Given that this function is building the actual protobuf types, I believe that must have already happened.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sorting happens before this is called:

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

Lines 436 to 437 in 8fb22d6

tcp_routes.sort_by(timestamp_then_name);
tcp::protocol(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adleong @olix0r

As pointed out sorting happens before this is called. In general the GW api spec does not prescribe setting conflict statuses in these cases. These are prescribed when there are routes with higher specificity (ie GRPC > HTTP). In cases when there are more than one route we need to follow disambiguation rules. In cases like HTTPRoute that would be more specific hostname, header and method matches. In the case of TCP these things are not present so we revert to sorting on route age and naming. For reference:

Proxy or Load Balancer routing configuration generated from HTTPRoutes MUST prioritize matches based on the following criteria, continuing on ties. Across all rules specified on applicable Routes, precedence must be given to the match having:

  • “Exact” path match.
  • “Prefix” path match with largest number of characters.
  • Method match.
  • Largest number of header matches.
  • Largest number of query param matches.

Note: The precedence of RegularExpression path matches are implementation-specific.

If ties still exist across multiple Routes, matching precedence MUST be determined in order of the following criteria, continuing on ties:

  • The oldest Route based on creation timestamp.
  • The Route appearing first in alphabetical order by “{namespace}/{name}”.

If ties still exist within an HTTPRoute, matching precedence MUST be granted to the FIRST matching rule (in list order) with a match meeting the above criteria.

When no rules matching a request have been successfully attached to the parent a request is coming from, a HTTP 404 status code MUST be returned.

.map(|(gknn, route)| {
convert_outbound_route(
gknn,
Expand All @@ -24,11 +28,13 @@ pub(crate) fn protocol(
})
.collect::<Vec<_>>();

if let ParentInfo::EgressNetwork { traffic_policy, .. } = parent_info {
routes.push(default_outbound_egress_route(
default_backend,
traffic_policy,
));
if routes.is_empty() {
if let ParentInfo::EgressNetwork { traffic_policy, .. } = parent_info {
routes.push(default_outbound_egress_route(
default_backend,
traffic_policy,
));
}
}

outbound::proxy_protocol::Kind::Opaque(outbound::proxy_protocol::Opaque { routes })
Expand Down
Loading