Skip to content

Commit

Permalink
fix(policy): ignore route .status.parents ordering (#13328)
Browse files Browse the repository at this point in the history
The `eq_time_insensitive_route_parent_statuses` considers ordering when comparing statuses, even though ordering
doesn't matter. This can result in infite update loops.

Add logic to `eq_time_insensitive_route_parent_statuses` to pre-sort statuses before comparison, thus making the result independent of ordering.

A unit test was added to validate the fix.

Fixes #13327
Signed-off-by: Derek Brown <6845676+DerekTBrown@users.noreply.github.com>
  • Loading branch information
DerekTBrown authored Nov 15, 2024
1 parent 8265134 commit f8a1343
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 2 deletions.
23 changes: 21 additions & 2 deletions policy-controller/k8s/status/src/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1521,14 +1521,33 @@ pub(crate) fn invalid_backend_kind(message: &str) -> k8s_core_api::Condition {
}
}

fn eq_time_insensitive_route_parent_statuses(
pub(crate) fn eq_time_insensitive_route_parent_statuses(
left: &[k8s_gateway_api::RouteParentStatus],
right: &[k8s_gateway_api::RouteParentStatus],
) -> bool {
if left.len() != right.len() {
return false;
}
left.iter().zip(right.iter()).all(|(l, r)| {

// Create sorted versions of the input slices
let mut left_sorted: Vec<_> = left.to_vec();
let mut right_sorted: Vec<_> = right.to_vec();

left_sorted.sort_by(|a, b| {
a.controller_name
.cmp(&b.controller_name)
.then_with(|| a.parent_ref.name.cmp(&b.parent_ref.name))
.then_with(|| a.parent_ref.namespace.cmp(&b.parent_ref.namespace))
});
right_sorted.sort_by(|a, b| {
a.controller_name
.cmp(&b.controller_name)
.then_with(|| a.parent_ref.name.cmp(&b.parent_ref.name))
.then_with(|| a.parent_ref.namespace.cmp(&b.parent_ref.namespace))
});

// Compare each element in sorted order
left_sorted.iter().zip(right_sorted.iter()).all(|(l, r)| {
l.parent_ref == r.parent_ref
&& l.controller_name == r.controller_name
&& eq_time_insensitive_conditions(&l.conditions, &r.conditions)
Expand Down
1 change: 1 addition & 0 deletions policy-controller/k8s/status/src/tests/routes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use chrono::{DateTime, Utc};
use linkerd_policy_controller_core::POLICY_CONTROLLER_NAME;

mod grpc;
mod helpers;
mod http;
mod tcp;
mod tls;
Expand Down
17 changes: 17 additions & 0 deletions policy-controller/k8s/status/src/tests/routes/helpers.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
use super::make_parent_status;
use crate::index::eq_time_insensitive_route_parent_statuses;

#[test]
fn test_eq_time_insensitive_route_parent_statuses_order_sensitive() {
// Create RouteParentStatus instances using make_parent_status helper
let status1 = make_parent_status("ns", "parent1", "Ready", "True", "AllGood");
let status2 = make_parent_status("ns", "parent2", "Ready", "True", "AllGood");

// Create two lists with the same elements in different orders
let list1 = vec![status1.clone(), status2.clone()];
let list2 = vec![status2, status1];

// Assert that eq_time_insensitive_route_parent_statuses returns true
// indicating that it considers the two lists equal
assert!(eq_time_insensitive_route_parent_statuses(&list1, &list2));
}

0 comments on commit f8a1343

Please sign in to comment.