From f732b70acb9b37f347b0d7fa48243d9e59afb8dd Mon Sep 17 00:00:00 2001 From: Adrian Moreno Date: Mon, 19 Aug 2024 11:23:16 +0200 Subject: [PATCH] events: ovs: Use Option instead of unit enum value. OVS module currently makes no assumptions on the order of the event chunks and tries to build the event even if out-of-order pieces are received. This makes little sense in practice as these chuncks are sent in the same hook. Removing that unneeded requirement and assuming the base action event (OVS_DP_ACTION) will be received before specific action argument events (e.g: OVS_DP_ACTION_OUTPUT) makes decoding simpler. This also it avoids requiring a Default version of the event which is currently implemented using an "undefined" value for enums. This mechanism is not supported by pyo3. Also, create a dummy object to avoid having mixed complex unit variants in enums. [1] Upstream discussions: /~https://github.com/PyO3/pyo3/pull/3582#discussion_r1443166018 /~https://github.com/PyO3/pyo3/issues/3749 Signed-off-by: Adrian Moreno --- retis-events/src/ovs.rs | 116 ++++++++-------- retis/src/module/ovs/bpf.rs | 253 +++++++++++++++------------------- retis/src/process/tracking.rs | 1 - 3 files changed, 168 insertions(+), 202 deletions(-) diff --git a/retis-events/src/ovs.rs b/retis-events/src/ovs.rs index 2474e11fd..fae212b73 100644 --- a/retis-events/src/ovs.rs +++ b/retis-events/src/ovs.rs @@ -7,7 +7,7 @@ use super::*; use crate::{event_section, event_type, Formatter}; ///The OVS Event -#[derive(Default, PartialEq)] +#[derive(PartialEq)] #[event_section("ovs")] pub struct OvsEvent { /// Event data @@ -23,7 +23,7 @@ impl EventFmt for OvsEvent { #[event_type] #[serde(tag = "event_type")] -#[derive(Default, PartialEq)] +#[derive(PartialEq)] pub enum OvsEventType { /// Upcall event. It indicates the begining of an upcall. An upcall can have multiple enqueue /// events. @@ -51,10 +51,6 @@ pub enum OvsEventType { /// Action execution event. It indicates the datapath has executed an action on a packet. #[serde(rename = "action_execute")] Action(ActionEvent), - - #[serde(rename = "undefined")] - #[default] - Undefined, } impl EventFmt for OvsEventType { @@ -67,7 +63,6 @@ impl EventFmt for OvsEventType { RecvUpcall(e) => e, Operation(e) => e, Action(e) => e, - Undefined => return write!(f, "?"), }; disp.event_fmt(f, format) @@ -266,7 +261,7 @@ impl EventFmt for RecvUpcallEvent { pub struct ActionEvent { /// Action to be executed. #[serde(flatten)] - pub action: OvsAction, + pub action: Option, /// Recirculation id. pub recirc_id: u32, /// Queue id used for tracking. None if not tracking or if the output event did not come from @@ -284,19 +279,18 @@ impl EventFmt for ActionEvent { write!(f, "exec")?; match &self.action { - OvsAction::Unspecified => write!(f, " (unspecified)")?, - OvsAction::Output(a) => write!(f, " oport {}", a.port)?, - OvsAction::Userspace => write!(f, " userspace")?, - OvsAction::Set => write!(f, " tunnel_set")?, - OvsAction::PushVlan => write!(f, " push_vlan")?, - OvsAction::PopVlan => write!(f, " pop_vlan")?, - OvsAction::Sample => write!(f, " sample")?, - OvsAction::Recirc(a) => write!(f, " recirc {:#x}", a.id)?, - OvsAction::Hash => write!(f, " hash")?, - OvsAction::PushMpls => write!(f, " push_mpls")?, - OvsAction::PopMpls => write!(f, " pop_mpls")?, - OvsAction::SetMasked => write!(f, " set_masked")?, - OvsAction::Ct(ct) => { + Some(OvsAction::Output(a)) => write!(f, " oport {}", a.port)?, + Some(OvsAction::Userspace(_)) => write!(f, " userspace")?, + Some(OvsAction::Set(_)) => write!(f, " tunnel_set")?, + Some(OvsAction::PushVlan(_)) => write!(f, " push_vlan")?, + Some(OvsAction::PopVlan(_)) => write!(f, " pop_vlan")?, + Some(OvsAction::Sample(_)) => write!(f, " sample")?, + Some(OvsAction::Recirc(a)) => write!(f, " recirc {:#x}", a.id)?, + Some(OvsAction::Hash(_)) => write!(f, " hash")?, + Some(OvsAction::PushMpls(_)) => write!(f, " push_mpls")?, + Some(OvsAction::PopMpls(_)) => write!(f, " pop_mpls")?, + Some(OvsAction::SetMasked(_)) => write!(f, " set_masked")?, + Some(OvsAction::Ct(ct)) => { write!(f, " ct zone {}", ct.zone_id)?; if let Some(nat) = &ct.nat { @@ -358,17 +352,18 @@ impl EventFmt for ActionEvent { write!(f, " {}", flags.join(","))?; } } - OvsAction::Trunc => write!(f, " trunc")?, - OvsAction::PushEth => write!(f, " push_eth")?, - OvsAction::PopEth => write!(f, " pop_eth")?, - OvsAction::CtClear => write!(f, " ct_clear")?, - OvsAction::PushNsh => write!(f, " push_nsh")?, - OvsAction::PopNsh => write!(f, " pop_nsh")?, - OvsAction::Meter => write!(f, " meter")?, - OvsAction::Clone => write!(f, " clone")?, - OvsAction::CheckPktLen => write!(f, " check_pkt_len")?, - OvsAction::AddMpls => write!(f, " add_mpls")?, - OvsAction::DecTtl => write!(f, " dec_ttl")?, + Some(OvsAction::Trunc(_)) => write!(f, " trunc")?, + Some(OvsAction::PushEth(_)) => write!(f, " push_eth")?, + Some(OvsAction::PopEth(_)) => write!(f, " pop_eth")?, + Some(OvsAction::CtClear(_)) => write!(f, " ct_clear")?, + Some(OvsAction::PushNsh(_)) => write!(f, " push_nsh")?, + Some(OvsAction::PopNsh(_)) => write!(f, " pop_nsh")?, + Some(OvsAction::Meter(_)) => write!(f, " meter")?, + Some(OvsAction::Clone(_)) => write!(f, " clone")?, + Some(OvsAction::CheckPktLen(_)) => write!(f, " check_pkt_len")?, + Some(OvsAction::AddMpls(_)) => write!(f, " add_mpls")?, + Some(OvsAction::DecTtl(_)) => write!(f, " dec_ttl")?, + None => write!(f, " unspec")?, } if let Some(p) = self.queue_id { @@ -379,59 +374,62 @@ impl EventFmt for ActionEvent { } } +// Adding unit values in an otherwise complex is not supported by pyo3. +// FIXME: Remove when arguments from all actions are implemented. +#[event_type] +#[derive(PartialEq)] +pub struct OvsDummyAction; + #[event_type] #[serde(tag = "action")] -#[derive(Default, PartialEq)] +#[derive(PartialEq)] pub enum OvsAction { - #[serde(rename = "unspecified")] - #[default] - Unspecified, #[serde(rename = "output")] Output(OvsActionOutput), #[serde(rename = "userspace")] - Userspace, + Userspace(OvsDummyAction), #[serde(rename = "set")] - Set, + Set(OvsDummyAction), #[serde(rename = "push_vlan")] - PushVlan, + PushVlan(OvsDummyAction), #[serde(rename = "pop_vlan")] - PopVlan, + PopVlan(OvsDummyAction), #[serde(rename = "sample")] - Sample, + Sample(OvsDummyAction), #[serde(rename = "recirc")] Recirc(OvsActionRecirc), #[serde(rename = "hash")] - Hash, + Hash(OvsDummyAction), #[serde(rename = "push_mpls")] - PushMpls, + PushMpls(OvsDummyAction), #[serde(rename = "pop_mpls")] - PopMpls, + PopMpls(OvsDummyAction), #[serde(rename = "set_masked")] - SetMasked, + SetMasked(OvsDummyAction), #[serde(rename = "ct")] Ct(OvsActionCt), #[serde(rename = "trunc")] - Trunc, + Trunc(OvsDummyAction), #[serde(rename = "push_eth")] - PushEth, + PushEth(OvsDummyAction), #[serde(rename = "pop_eth")] - PopEth, + PopEth(OvsDummyAction), #[serde(rename = "ct_clear")] - CtClear, + CtClear(OvsDummyAction), #[serde(rename = "push_nsh")] - PushNsh, + PushNsh(OvsDummyAction), #[serde(rename = "pop_nsh")] - PopNsh, + PopNsh(OvsDummyAction), #[serde(rename = "meter")] - Meter, + Meter(OvsDummyAction), #[serde(rename = "clone")] - Clone, + Clone(OvsDummyAction), #[serde(rename = "check_pkt_len")] - CheckPktLen, + CheckPktLen(OvsDummyAction), #[serde(rename = "add_mpls")] - AddMpls, + AddMpls(OvsDummyAction), #[serde(rename = "dec_ttl")] - DecTtl, + DecTtl(OvsDummyAction), } /// OVS output action data. @@ -555,7 +553,7 @@ mod tests { r#"{"action":"output","event_type":"action_execute","port":2,"queue_id":1361394472,"recirc_id":0}"#, OvsEvent { event: OvsEventType::Action(ActionEvent { - action: OvsAction::Output(OvsActionOutput { port: 2 }), + action: Some(OvsAction::Output(OvsActionOutput { port: 2 })), recirc_id: 0, queue_id: Some(1361394472), }), @@ -615,7 +613,7 @@ mod tests { r#"{"action":"ct","event_type":"action_execute","flags":485,"nat":{"dir":"dst","max_addr":"10.244.1.30","max_port":36900,"min_addr":"10.244.1.3","min_port":36895},"recirc_id":34,"zone_id":20}"#, OvsEvent { event: OvsEventType::Action(ActionEvent { - action: OvsAction::Ct(OvsActionCt { + action: Some(OvsAction::Ct(OvsActionCt { zone_id: 20, flags: 485, nat: Some(OvsActionCtNat { @@ -625,7 +623,7 @@ mod tests { min_port: Some(36895), max_port: Some(36900), }), - }), + })), recirc_id: 34, queue_id: None, }), diff --git a/retis/src/module/ovs/bpf.rs b/retis/src/module/ovs/bpf.rs index 22025fd9f..e42ff5f6c 100644 --- a/retis/src/module/ovs/bpf.rs +++ b/retis/src/module/ovs/bpf.rs @@ -4,7 +4,7 @@ use std::net::Ipv6Addr; -use anyhow::{bail, Result}; +use anyhow::{anyhow, bail, Result}; use crate::{ core::events::{parse_raw_section, BpfRawSection, EventSectionFactory, RawEventSectionFactory}, @@ -56,26 +56,11 @@ impl OvsDataType { } } -// OVS module supports several event data types but many of them end up setting the OvsEvent -// to one of its variants which mean they are mutally exclusive. -// This helper ensures the event has was not set before to any of its variants to help -// report his error condition. -pub(crate) fn ensure_undefined(event: &OvsEvent, received: OvsDataType) -> Result<()> { - match &event.event { - OvsEventType::Undefined => Ok(()), - other => bail!( - "Conflicting OVS event types. Received {:?} data type but event is already {:#?}", - received, - other - ), - } -} - -pub(super) fn unmarshall_upcall(raw_section: &BpfRawSection, event: &mut OvsEvent) -> Result<()> { - ensure_undefined(event, OvsDataType::Upcall)?; +pub(super) fn unmarshall_upcall(raw_section: &BpfRawSection) -> Result { let upcall = parse_raw_section::(raw_section)?; - event.event = OvsEventType::Upcall(*upcall); - Ok(()) + Ok(OvsEvent { + event: OvsEventType::Upcall(*upcall), + }) } /// OVS action event data. @@ -87,68 +72,50 @@ pub(crate) struct BpfActionEvent { recirc_id: u32, } -pub(super) fn unmarshall_exec(raw_section: &BpfRawSection, event: &mut OvsEvent) -> Result<()> { +pub(super) fn unmarshall_exec(raw_section: &BpfRawSection) -> Result { let raw = parse_raw_section::(raw_section)?; - // Any of the action-related bpf events (e.g BpfActionTrackEvent, BpfActionTrackEvent, etc) - // might have been received before. If so, event.event is already a valid - // OvsEventType::Action. - match &mut event.event { - OvsEventType::Action(action) => { - // One of the specific action events has already been received and it has initialized - // the action.data enum. Only the common data has to be set here. - action.recirc_id = raw.recirc_id; - } - OvsEventType::Undefined => { - // When we implement event data types for every action we will be able to create the - // specific action variant when unmarshaling its event data type. Until then, we need to - // initialize the Action here based on the action_id (which corresponds to ovs_action_attr - // defined in uapi/linux/openvswitch.h). - event.event = OvsEventType::Action(ActionEvent { - action: match raw.action { - 0 => OvsAction::Unspecified, - 1 => OvsAction::Output(OvsActionOutput::default()), - 2 => OvsAction::Userspace, - 3 => OvsAction::Set, - 4 => OvsAction::PushVlan, - 5 => OvsAction::PopVlan, - 6 => OvsAction::Sample, - 7 => OvsAction::Recirc(OvsActionRecirc::default()), - 8 => OvsAction::Hash, - 9 => OvsAction::PushMpls, - 10 => OvsAction::PopMpls, - 11 => OvsAction::SetMasked, - 12 => OvsAction::Ct(OvsActionCt::default()), - 13 => OvsAction::Trunc, - 14 => OvsAction::PushEth, - 15 => OvsAction::PopEth, - 16 => OvsAction::CtClear, - 17 => OvsAction::PushNsh, - 18 => OvsAction::PopNsh, - 19 => OvsAction::Meter, - 20 => OvsAction::Clone, - 21 => OvsAction::CheckPktLen, - 22 => OvsAction::AddMpls, - 23 => OvsAction::DecTtl, - // The private OVS_ACTION_ATTR_SET_TO_MASKED action is used - // in the same way as OVS_ACTION_ATTR_SET_MASKED. Use only - // one action to avoid confusion - 25 => OvsAction::SetMasked, - val => bail!("Unsupported action id {val}"), - }, - recirc_id: raw.recirc_id, - ..ActionEvent::default() - }); - } - other => { - bail!( - "Conflicting OVS event types. Received {:?} data type but event is already {:#?}", - OvsDataType::ActionExec, - other - ); - } - } - Ok(()) + // When we implement event data types for every action we will be able to create the + // specific action variant when unmarshaling its event data type. Until then, we need to + // initialize the Action here based on the action_id (which corresponds to ovs_action_attr + // defined in uapi/linux/openvswitch.h). + Ok(OvsEvent { + event: OvsEventType::Action(ActionEvent { + action: match raw.action { + 0 => None, + 1 => Some(OvsAction::Output(OvsActionOutput::default())), + 2 => Some(OvsAction::Userspace(OvsDummyAction)), + 3 => Some(OvsAction::Set(OvsDummyAction)), + 4 => Some(OvsAction::PushVlan(OvsDummyAction)), + 5 => Some(OvsAction::PopVlan(OvsDummyAction)), + 6 => Some(OvsAction::Sample(OvsDummyAction)), + 7 => Some(OvsAction::Recirc(OvsActionRecirc::default())), + 8 => Some(OvsAction::Hash(OvsDummyAction)), + 9 => Some(OvsAction::PushMpls(OvsDummyAction)), + 10 => Some(OvsAction::PopMpls(OvsDummyAction)), + 11 => Some(OvsAction::SetMasked(OvsDummyAction)), + 12 => Some(OvsAction::Ct(OvsActionCt::default())), + 13 => Some(OvsAction::Trunc(OvsDummyAction)), + 14 => Some(OvsAction::PushEth(OvsDummyAction)), + 15 => Some(OvsAction::PopEth(OvsDummyAction)), + 16 => Some(OvsAction::CtClear(OvsDummyAction)), + 17 => Some(OvsAction::PushNsh(OvsDummyAction)), + 18 => Some(OvsAction::PopNsh(OvsDummyAction)), + 19 => Some(OvsAction::Meter(OvsDummyAction)), + 20 => Some(OvsAction::Clone(OvsDummyAction)), + 21 => Some(OvsAction::CheckPktLen(OvsDummyAction)), + 22 => Some(OvsAction::AddMpls(OvsDummyAction)), + 23 => Some(OvsAction::DecTtl(OvsDummyAction)), + // The private OVS_ACTION_ATTR_SET_TO_MASKED action is used + // in the same way as OVS_ACTION_ATTR_SET_MASKED. Use only + // one action to avoid confusion + 25 => Some(OvsAction::SetMasked(OvsDummyAction)), + val => bail!("Unsupported action id {val}"), + }, + recirc_id: raw.recirc_id, + ..ActionEvent::default() + }), + }) } /// OVS action tracking event data. @@ -164,19 +131,8 @@ pub(super) fn unmarshall_exec_track( ) -> Result<()> { let raw = parse_raw_section::(raw_section)?; - // Any of the action-related bpf events (e.g BpfActionEvent, BpfActionTrackEvent, etc) - // might have been received before. If so, event.event is already a valid - // OvsEventType::Action. match &mut event.event { OvsEventType::Action(ref mut action) => action.queue_id = Some(raw.queue_id), - OvsEventType::Undefined => { - // We received the tracking event before the generic one. - // Initialize the Action Event. - event.event = OvsEventType::Action(ActionEvent { - queue_id: Some(raw.queue_id), - ..ActionEvent::default() - }); - } other => { bail!( "Conflicting OVS event types. Received {:?} data type but event is already {:#?}", @@ -189,19 +145,8 @@ pub(super) fn unmarshall_exec_track( } fn update_action_event(event: &mut OvsEvent, action: OvsAction) -> Result<()> { - // Any of the action-related bpf events (e.g BpfActionEvent, BpfActionTrackEvent, etc) - // might have been received before. If so, event.event is already a valid - // OvsEventType::Action. match &mut event.event { - OvsEventType::Action(ref mut event) => event.action = action, - OvsEventType::Undefined => { - // We received the concrete action data type before the generic one. - // Initialize the Action Event. - event.event = OvsEventType::Action(ActionEvent { - action, - ..ActionEvent::default() - }); - } + OvsEventType::Action(ref mut event) => event.action = Some(action), other => { bail!( "Conflicting OVS event types. Received {:?} data type but event is already {:#?}", @@ -305,45 +250,35 @@ pub(super) fn unmarshall_ct(raw_section: &BpfRawSection, event: &mut OvsEvent) - update_action_event(event, OvsAction::Ct(ct)) } -pub(super) fn unmarshall_recv(raw_section: &BpfRawSection, event: &mut OvsEvent) -> Result<()> { - ensure_undefined(event, OvsDataType::RecvUpcall)?; +pub(super) fn unmarshall_recv(raw_section: &BpfRawSection) -> Result { let recv = parse_raw_section::(raw_section)?; - event.event = OvsEventType::RecvUpcall(*recv); - - Ok(()) + Ok(OvsEvent { + event: OvsEventType::RecvUpcall(*recv), + }) } -pub(super) fn unmarshall_operation( - raw_section: &BpfRawSection, - event: &mut OvsEvent, -) -> Result<()> { - ensure_undefined(event, OvsDataType::Operation)?; +pub(super) fn unmarshall_operation(raw_section: &BpfRawSection) -> Result { let op = parse_raw_section::(raw_section)?; - event.event = OvsEventType::Operation(*op); - Ok(()) + Ok(OvsEvent { + event: OvsEventType::Operation(*op), + }) } -pub(super) fn unmarshall_upcall_enqueue( - raw_section: &BpfRawSection, - event: &mut OvsEvent, -) -> Result<()> { - ensure_undefined(event, OvsDataType::UpcallEnqueue)?; +pub(super) fn unmarshall_upcall_enqueue(raw_section: &BpfRawSection) -> Result { let enqueue = parse_raw_section::(raw_section)?; - event.event = OvsEventType::UpcallEnqueue(*enqueue); - Ok(()) + Ok(OvsEvent { + event: OvsEventType::UpcallEnqueue(*enqueue), + }) } -pub(super) fn unmarshall_upcall_return( - raw_section: &BpfRawSection, - event: &mut OvsEvent, -) -> Result<()> { - ensure_undefined(event, OvsDataType::UpcallReturn)?; +pub(super) fn unmarshall_upcall_return(raw_section: &BpfRawSection) -> Result { let uret = parse_raw_section::(raw_section)?; - event.event = OvsEventType::UpcallReturn(*uret); - Ok(()) + Ok(OvsEvent { + event: OvsEventType::UpcallReturn(*uret), + }) } #[derive(Default, crate::EventSectionFactory)] @@ -351,24 +286,58 @@ pub(crate) struct OvsEventFactory {} impl RawEventSectionFactory for OvsEventFactory { fn create(&mut self, raw_sections: Vec) -> Result> { - let mut event = OvsEvent::default(); + let mut event = None; // = OvsEvent::default(); for section in raw_sections.iter() { match OvsDataType::from_u8(section.header.data_type)? { - OvsDataType::Upcall => unmarshall_upcall(section, &mut event), - OvsDataType::UpcallEnqueue => unmarshall_upcall_enqueue(section, &mut event), - OvsDataType::UpcallReturn => unmarshall_upcall_return(section, &mut event), - OvsDataType::RecvUpcall => unmarshall_recv(section, &mut event), - OvsDataType::Operation => unmarshall_operation(section, &mut event), - OvsDataType::ActionExec => unmarshall_exec(section, &mut event), - OvsDataType::ActionExecTrack => unmarshall_exec_track(section, &mut event), - OvsDataType::OutputAction => unmarshall_output(section, &mut event), - OvsDataType::RecircAction => unmarshall_recirc(section, &mut event), - OvsDataType::ConntrackAction => unmarshall_ct(section, &mut event), - }?; + OvsDataType::Upcall => { + event = Some(unmarshall_upcall(section)?); + } + OvsDataType::UpcallEnqueue => { + event = Some(unmarshall_upcall_enqueue(section)?); + } + OvsDataType::UpcallReturn => { + event = Some(unmarshall_upcall_return(section)?); + } + OvsDataType::RecvUpcall => { + event = Some(unmarshall_recv(section)?); + } + OvsDataType::Operation => { + event = Some(unmarshall_operation(section)?); + } + OvsDataType::ActionExec => { + event = Some(unmarshall_exec(section)?); + } + OvsDataType::ActionExecTrack => unmarshall_exec_track( + section, + event + .as_mut() + .ok_or_else(|| anyhow!("received action track without action"))?, + )?, + OvsDataType::OutputAction => unmarshall_output( + section, + event + .as_mut() + .ok_or_else(|| anyhow!("received action data without action"))?, + )?, + OvsDataType::RecircAction => unmarshall_recirc( + section, + event + .as_mut() + .ok_or_else(|| anyhow!("received action data without action"))?, + )?, + OvsDataType::ConntrackAction => unmarshall_ct( + section, + event + .as_mut() + .ok_or_else(|| anyhow!("received action data without action"))?, + )?, + }; } - Ok(Box::new(event)) + Ok(Box::new( + event.ok_or_else(|| anyhow!("Incomplete OVS event"))?, + )) } } diff --git a/retis/src/process/tracking.rs b/retis/src/process/tracking.rs index 2be2e5fc4..f4dc95b07 100644 --- a/retis/src/process/tracking.rs +++ b/retis/src/process/tracking.rs @@ -145,7 +145,6 @@ impl AddTracking { self.process_skb(event)?; } }, - Undefined => bail!("Cannot track undefined ovs event"), } } else { // It's not an OVS event, try skb-only tracking.