Fix/gobgp legacy withdraw

daemon/src/event/grpc.rs

@@ -667,12 +667,14 @@ impl From<api::PeerGroup> for PeerGroup {
     }
 }
 
+type PathUuidMap = FnvHashMap<uuid::Uuid, (Family, Vec<packet::PathNlri>)>;
+
 pub(super) struct GrpcService {
     init: Arc<tokio::sync::Notify>,
     active_conn_tx: mpsc::UnboundedSender<TcpStream>,
     pub(super) global: GlobalHandle,
     pub(super) tables: TableHandle,
-    path_uuid_map: tokio::sync::Mutex<FnvHashMap<uuid::Uuid, (Family, Vec<packet::PathNlri>)>>,
+    path_uuid_map: tokio::sync::Mutex<PathUuidMap>,
 }
 
 /// Validate and convert `api::EbgpMultihop` to the internal `Option<u8>`.
@@ -770,6 +772,70 @@ impl GrpcService {
         }
     }
 
+    fn withdraw_local_paths(
+        &self,
+        uuid_map: &mut PathUuidMap,
+        family: Family,
+        nets: &[packet::PathNlri],
+    ) {
+        let timestamp = crate::proto::unix_secs();
+        for net in nets {
+            self.tables
+                .remove_route(table::Source::local(), family, net.clone(), None, timestamp);
+        }
+        let removed: FnvHashSet<_> = nets.iter().collect();
+        // Forget every handle for a removed path, so a stale UUID cannot
+        // delete a later announcement with the same NLRI and identifier.
+        uuid_map.retain(|_, (mapped_family, mapped_nets)| {
+            if *mapped_family == family {
+                mapped_nets.retain(|net| !removed.contains(net));
+            }
+            !mapped_nets.is_empty()
+        });
+    }
+
+    fn local_paths_to_delete(
+        &self,
+        uuid_map: &PathUuidMap,
+        family: Option<Family>,
+        vrf: Option<&table::Vrf>,
+    ) -> FnvHashMap<Family, Vec<packet::PathNlri>> {
+        let mut paths: FnvHashMap<Family, Vec<packet::PathNlri>> = FnvHashMap::default();
+        let in_vrf = |net: &packet::PathNlri| {
+            vrf.is_none_or(|vrf| match &net.nlri {
+                packet::Nlri::VpnV4(n) => n.rd == vrf.rd && n.labels.labels() == [vrf.label],
+                packet::Nlri::VpnV6(n) => n.rd == vrf.rd && n.labels.labels() == [vrf.label],
+                _ => false,
+            })
+        };
+        for shard in &self.tables.shards {
+            let shard = shard.lock().unwrap();
+            for f in shard
+                .rtable
+                .families()
+                .filter(|f| family.is_none_or(|family| *f == family))
+            {
+                // iter_reach includes policy-filtered routes and their original
+                // path identifiers, including paths injected without a UUID.
+                for reach in shard.rtable.iter_reach(f).filter(|r| r.source.is_local()) {
+                    if in_vrf(&reach.net) {
+                        paths.entry(f).or_default().push(reach.net);
+                    }
+                }
+            }
+        }
+        // Also invalidate handles for local paths replaced by kernel updates.
+        for (f, nets) in uuid_map.values() {
+            if family.is_none_or(|family| *f == family) {
+                paths
+                    .entry(*f)
+                    .or_default()
+                    .extend(nets.iter().filter(|net| in_vrf(net)).cloned());
+            }
+        }
+        paths
+    }
+
     async fn is_available(&self, need_active: bool) -> Result<(), Error> {
         let global = &self.global.read().await;
         if need_active && global.asn == 0 {
@@ -795,14 +861,38 @@ impl GrpcService {
             Some(family) => convert::family_from_api(&family),
             None => Family::IPV4,
         };
-        let net = convert::net_from_api(path.nlri.ok_or(Error::EmptyArgument)?, family)
-            .map_err(|_| tonic::Status::new(tonic::Code::InvalidArgument, "prefix is invalid"))?;
+        // GoBGP gives the binary fields precedence when both forms are supplied.
+        let net = if path.nlri_binary.is_empty() {
+            convert::net_from_api(path.nlri.ok_or(Error::EmptyArgument)?, family)
+                .map_err(|_| tonic::Status::invalid_argument("prefix is invalid"))?
+        } else {
+            packet::Nlri::decode_from_bytes(family, &path.nlri_binary)
+                .map_err(|_| tonic::Status::invalid_argument("invalid binary NLRI"))?
+        };
+        let attributes = if path.pattrs_binary.is_empty() {
+            path.pattrs
+                .into_iter()
+                .map(|a| {
+                    convert::attr_from_api(a)
+                        .map_err(|_| tonic::Status::invalid_argument("invalid attribute"))
+                })
+                .collect::<Result<Vec<_>, _>>()?
+        } else {
+            path.pattrs_binary
+                .iter()
+                .map(|a| {
+                    packet::Attribute::decode_from_bytes(a)
+                        .map_err(|_| tonic::Status::invalid_argument("invalid binary attribute"))
+                })
+                .collect::<Result<Vec<_>, _>>()?
+        };
         let mut attr = Vec::new();
         let mut nexthop = None;
-        for a in path.pattrs {
-            let a = convert::attr_from_api(a).map_err(|_| {
-                tonic::Status::new(tonic::Code::InvalidArgument, "invalid attribute")
-            })?;
+        let mut seen = FnvHashSet::default();
+        for a in attributes {
+            if !seen.insert(a.code()) {
+                return Err(tonic::Status::invalid_argument("duplicate attribute"));
+            }
             match a.code() {
                 bgp::Attribute::MP_REACH => {
                     // MP_REACH binary: [AFI:2][SAFI:1][NH_LEN:1][nexthop:NH_LEN][reserved:1][NLRI...]
@@ -833,6 +923,9 @@ impl GrpcService {
                 }
                 bgp::Attribute::NEXTHOP => {
                     nexthop = a.binary().and_then(|b| bgp::Nexthop::from_bytes(b));
+                    if nexthop.is_none() {
+                        return Err(tonic::Status::invalid_argument("invalid nexthop"));
+                    }
                 }
                 // RR attributes are added on reflection and must not be set by operators.
                 // MP_UNREACH has no meaning in an add_path request.
@@ -1981,8 +2074,9 @@ impl GoBgpService for GrpcService {
         let inner = request.into_inner();
         let table_type =
             api::TableType::try_from(inner.table_type).unwrap_or(api::TableType::Global);
-        let (mut family, nets, attrs, nexthop) =
-            self.local_path(inner.path.ok_or(Error::EmptyArgument)?)?;
+        let path = inner.path.ok_or(Error::EmptyArgument)?;
+        let is_withdraw = path.is_withdraw;
+        let (mut family, nets, attrs, nexthop) = self.local_path(path)?;
         let mut insert_nets = nets.clone();
         let mut insert_attrs = attrs;
         if table_type == api::TableType::Vrf {
@@ -2008,6 +2102,12 @@ impl GoBgpService for GrpcService {
         let map_nets = insert_nets.clone();
         let timestamp = crate::proto::unix_secs();
         let source = table::Source::local();
+        // Serialize route mutations and UUID bookkeeping with DeletePath.
+        let mut uuid_map = self.path_uuid_map.lock().await;
+        if is_withdraw {
+            self.withdraw_local_paths(&mut uuid_map, family, &insert_nets);
+            return Ok(tonic::Response::new(api::AddPathResponse::default()));
+        }
         if let Some(attrs) = insert_attrs {
             for net in insert_nets {
                 self.tables.insert_route(
@@ -2022,10 +2122,7 @@ impl GoBgpService for GrpcService {
             }
         }
         let id = uuid::Uuid::new_v4();
-        self.path_uuid_map
-            .lock()
-            .await
-            .insert(id, (family, map_nets));
+        uuid_map.insert(id, (family, map_nets));
         Ok(tonic::Response::new(api::AddPathResponse {
             uuid: id.as_bytes().to_vec(),
         }))
@@ -2036,25 +2133,73 @@ impl GoBgpService for GrpcService {
     ) -> Result<tonic::Response<api::DeletePathResponse>, tonic::Status> {
         let inner = request.into_inner();
         if inner.uuid.is_empty() {
-            return Err(tonic::Status::new(
-                tonic::Code::InvalidArgument,
-                "uuid is required",
-            ));
+            let table_type = api::TableType::try_from(inner.table_type)
+                .map_err(|_| tonic::Status::invalid_argument("invalid table type"))?;
+            if !matches!(
+                table_type,
+                api::TableType::Unspecified | api::TableType::Global | api::TableType::Vrf
+            ) {
+                return Err(tonic::Status::invalid_argument(
+                    "DeletePath only supports global and VRF tables",
+                ));
+            }
+            let vrf = if table_type == api::TableType::Vrf {
+                if inner.vrf_id.is_empty() {
+                    return Err(tonic::Status::invalid_argument(
+                        "vrf_id is required for VRF table type",
+                    ));
+                }
+                Some(
+                    self.tables
+                        .list_vrfs(Some(&inner.vrf_id))
+                        .into_iter()
+                        .next()
+                        .ok_or_else(|| {
+                            tonic::Status::not_found(format!("VRF '{}' not found", inner.vrf_id))
+                        })?,
+                )
+            } else {
+                None
+            };
+            let paths = if let Some(path) = inner.path {
+                let (mut family, mut nets, attrs, _) = self.local_path(path)?;
+                if let Some(vrf) = &vrf {
+                    (family, nets, _) = vrf_export_path(family, nets, attrs, vrf)?;
+                }
+                Some((family, nets))
+            } else {
+                None
+            };
+            let mut uuid_map = self.path_uuid_map.lock().await;
+            if let Some((family, nets)) = paths {
+                self.withdraw_local_paths(&mut uuid_map, family, &nets);
+            } else {
+                let mut family = inner.family.as_ref().map(convert::family_from_api);
+                if vrf.is_some() {
+                    family = match family {
+                        Some(Family::IPV4) => Some(Family::IPV4_VPN),
+                        Some(Family::IPV6) => Some(Family::IPV6_VPN),
+                        None => None,
+                        _ => {
+                            return Err(tonic::Status::invalid_argument(
+                                "VRF DeletePath only supports IPv4/IPv6 families",
+                            ));
+                        }
+                    };
+                }
+                for (family, nets) in self.local_paths_to_delete(&uuid_map, family, vrf.as_ref()) {
+                    self.withdraw_local_paths(&mut uuid_map, family, &nets);
+                }
+            }
+            return Ok(tonic::Response::new(api::DeletePathResponse {}));
         }
         let id = uuid::Uuid::from_slice(&inner.uuid)
             .map_err(|_| tonic::Status::new(tonic::Code::InvalidArgument, "invalid uuid"))?;
-        let (family, nets) = self
-            .path_uuid_map
-            .lock()
-            .await
+        let mut uuid_map = self.path_uuid_map.lock().await;
+        let (family, nets) = uuid_map
             .remove(&id)
             .ok_or_else(|| tonic::Status::new(tonic::Code::NotFound, "uuid not found"))?;
-        let timestamp = crate::proto::unix_secs();
-        let source = table::Source::local();
-        for net in nets {
-            self.tables
-                .remove_route(source.clone(), family, net, None, timestamp);
-        }
+        self.withdraw_local_paths(&mut uuid_map, family, &nets);
         Ok(tonic::Response::new(api::DeletePathResponse {}))
     }
     type ListPathStream = Pin<