Skip to content

Commit

Permalink
bgp: ensure attribute sets are re-eavaluated whenever necessary
Browse files Browse the repository at this point in the history 
If an attribute set isn't referenced by any route, it should be
removed. This commit contains a few fixes to ensure that this
always happens.

Additionally, `AdjRib` fields are now made private, preventing them
from being modified without using the correct APIs.

Signed-off-by: Renato Westphal <[email protected]>
  • Loading branch information
@rwestphal
rwestphal committed Apr 25, 2024
1 parent 02992ee commit 34789d7
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 38 deletions.
35 changes: 16 additions & 19 deletions holo-bgp/src/events.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,11 +488,11 @@ where
);

// Update nexthop tracking.
if let Some(old_route) = adj_rib.in_post.take() {
if let Some(old_route) = adj_rib.in_post() {
rib::nexthop_untrack(
&mut table.nht,
&prefix,
&old_route,
old_route,
&instance.tx.ibus,
);
}
Expand All @@ -506,7 +506,8 @@ where
adj_rib.update_in_post(Box::new(route), &mut rib.attr_sets);
}
PolicyResult::Reject => {
if let Some(route) = adj_rib.in_post.take() {
if let Some(route) = adj_rib.remove_in_post(&mut rib.attr_sets)
{
rib::nexthop_untrack(
&mut table.nht,
&prefix,
Expand Down Expand Up @@ -563,21 +564,17 @@ where
rpinfo.route_type,
);

let mut update = false;
if let Some(adj_rib_route) = &mut adj_rib.out_post {
if adj_rib_route.attrs != route.attrs {
*adj_rib_route = Box::new(route);
update = true;
}
// Check if the Adj-RIB-Out was updated.
let update = if let Some(adj_rib_route) = adj_rib.out_post() {
adj_rib_route.attrs != route.attrs
} else {
true
};

if update {
adj_rib
.update_out_post(Box::new(route), &mut rib.attr_sets);
update = true;
}

// If the Adj-RIB-Out was updated, enqueue the route for
// transmission.
if update {
// Update route's attributes before transmission.
let mut attrs = rpinfo.attrs;
attrs_tx_update::<A>(
Expand All @@ -593,7 +590,7 @@ where
}
}
PolicyResult::Reject => {
if adj_rib.out_post.take().is_some() {
if adj_rib.remove_out_post(&mut rib.attr_sets).is_some() {
// Update neighbor's Tx queue.
let update_queue = A::update_queue(&mut nbr.update_queues);
update_queue.unreach.insert(prefix);
Expand Down Expand Up @@ -802,10 +799,10 @@ where
let dest = entry.get();
if dest.local.is_none()
&& dest.adj_rib.values().all(|adj_rib| {
adj_rib.in_pre.is_none()
&& adj_rib.in_post.is_none()
&& adj_rib.out_pre.is_none()
&& adj_rib.out_post.is_none()
adj_rib.in_pre().is_none()
&& adj_rib.in_post().is_none()
&& adj_rib.out_pre().is_none()
&& adj_rib.out_post().is_none()
})
{
entry.remove();
Expand Down
10 changes: 7 additions & 3 deletions holo-bgp/src/neighbor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -929,17 +929,21 @@ impl Neighbor {
let table = A::table(&mut rib.tables);
for (prefix, dest) in table.prefixes.iter_mut() {
// Clear the Adj-RIB-In and Adj-RIB-Out.
if let Some(adj_rib) = dest.adj_rib.remove(&self.remote_addr).take()
{
if let Some(mut adj_rib) = dest.adj_rib.remove(&self.remote_addr) {
// Update nexthop tracking.
if let Some(adj_in_route) = &adj_rib.in_post {
if let Some(adj_in_route) = adj_rib.in_post() {
rib::nexthop_untrack(
&mut table.nht,
prefix,
adj_in_route,
ibus_tx,
);
}

adj_rib.remove_in_pre(&mut rib.attr_sets);
adj_rib.remove_in_post(&mut rib.attr_sets);
adj_rib.remove_out_pre(&mut rib.attr_sets);
adj_rib.remove_out_post(&mut rib.attr_sets);
}

// Enqueue prefix for the BGP Decision Process.
Expand Down
16 changes: 8 additions & 8 deletions holo-bgp/src/northbound/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,7 @@ fn load_callbacks() -> Callbacks<Instance> {
|(prefix, dest)| {
dest.adj_rib
.get(&nbr.remote_addr)
.and_then(|adj_rib| adj_rib.in_pre.as_ref())
.and_then(|adj_rib| adj_rib.in_pre())
.map(|route| {
ListEntry::RibV4AdjInPreRoute(prefix, route)
})
Expand Down Expand Up @@ -1041,7 +1041,7 @@ fn load_callbacks() -> Callbacks<Instance> {
|(prefix, dest)| {
dest.adj_rib
.get(&nbr.remote_addr)
.and_then(|adj_rib| adj_rib.in_post.as_ref())
.and_then(|adj_rib| adj_rib.in_post())
.map(|route| {
ListEntry::RibV4AdjInPostRoute(prefix, route)
})
Expand Down Expand Up @@ -1147,7 +1147,7 @@ fn load_callbacks() -> Callbacks<Instance> {
|(prefix, dest)| {
dest.adj_rib
.get(&nbr.remote_addr)
.and_then(|adj_rib| adj_rib.out_pre.as_ref())
.and_then(|adj_rib| adj_rib.out_pre())
.map(|route| {
ListEntry::RibV4AdjOutPreRoute(prefix, route)
})
Expand Down Expand Up @@ -1248,7 +1248,7 @@ fn load_callbacks() -> Callbacks<Instance> {
|(prefix, dest)| {
dest.adj_rib
.get(&nbr.remote_addr)
.and_then(|adj_rib| adj_rib.out_post.as_ref())
.and_then(|adj_rib| adj_rib.out_post())
.map(|route| {
ListEntry::RibV4AdjOutPostRoute(prefix, route)
})
Expand Down Expand Up @@ -1458,7 +1458,7 @@ fn load_callbacks() -> Callbacks<Instance> {
|(prefix, dest)| {
dest.adj_rib
.get(&nbr.remote_addr)
.and_then(|adj_rib| adj_rib.in_pre.as_ref())
.and_then(|adj_rib| adj_rib.in_pre())
.map(|route| {
ListEntry::RibV6AdjInPreRoute(prefix, route)
})
Expand Down Expand Up @@ -1559,7 +1559,7 @@ fn load_callbacks() -> Callbacks<Instance> {
|(prefix, dest)| {
dest.adj_rib
.get(&nbr.remote_addr)
.and_then(|adj_rib| adj_rib.in_post.as_ref())
.and_then(|adj_rib| adj_rib.in_post())
.map(|route| {
ListEntry::RibV6AdjInPostRoute(prefix, route)
})
Expand Down Expand Up @@ -1665,7 +1665,7 @@ fn load_callbacks() -> Callbacks<Instance> {
|(prefix, dest)| {
dest.adj_rib
.get(&nbr.remote_addr)
.and_then(|adj_rib| adj_rib.out_pre.as_ref())
.and_then(|adj_rib| adj_rib.out_pre())
.map(|route| {
ListEntry::RibV6AdjOutPreRoute(prefix, route)
})
Expand Down Expand Up @@ -1766,7 +1766,7 @@ fn load_callbacks() -> Callbacks<Instance> {
|(prefix, dest)| {
dest.adj_rib
.get(&nbr.remote_addr)
.and_then(|adj_rib| adj_rib.out_post.as_ref())
.and_then(|adj_rib| adj_rib.out_post())
.map(|route| {
ListEntry::RibV6AdjOutPostRoute(prefix, route)
})
Expand Down
32 changes: 24 additions & 8 deletions holo-bgp/src/rib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ pub struct Destination {

#[derive(Debug, Default)]
pub struct AdjRib {
pub in_pre: Option<Box<Route>>,
pub in_post: Option<Box<Route>>,
pub out_pre: Option<Box<Route>>,
pub out_post: Option<Box<Route>>,
in_pre: Option<Box<Route>>,
in_post: Option<Box<Route>>,
out_pre: Option<Box<Route>>,
out_post: Option<Box<Route>>,
}

#[derive(Clone, Debug, Eq, PartialEq)]
Expand Down Expand Up @@ -214,6 +214,22 @@ impl AdjRib {
*table = Some(route)
}

pub(crate) fn in_pre(&self) -> Option<&Box<Route>> {
self.in_pre.as_ref()
}

pub(crate) fn in_post(&self) -> Option<&Box<Route>> {
self.in_post.as_ref()
}

pub(crate) fn out_pre(&self) -> Option<&Box<Route>> {
self.out_pre.as_ref()
}

pub(crate) fn out_post(&self) -> Option<&Box<Route>> {
self.out_post.as_ref()
}

pub(crate) fn remove_in_pre(
&mut self,
attr_sets: &mut AttrSetsCxt,
Expand Down Expand Up @@ -791,14 +807,14 @@ pub(crate) fn loc_rib_update<A>(
Debug::BestPathNotFound(prefix.into()).log();

// Remove route from the Loc-RIB.
if let Some(local_route) = dest.local.take()
&& !local_route.origin.is_local()
{
if let Some(local_route) = dest.local.take() {
// Check attribute sets that might need to be removed.
attr_sets.remove_route_attr_sets(&local_route.attrs);

// Uninstall route from the global RIB.
southbound::tx::route_uninstall(ibus_tx, prefix);
if !local_route.origin.is_local() {
southbound::tx::route_uninstall(ibus_tx, prefix);
}
}
}
}
Expand Down

0 comments on commit 34789d7

Please sign in to comment.