On Fri, Nov 17, 2023 at 7:06 PM Jeremy Spewock <jspew...@iol.unh.edu> wrote:
>
>
>
> On Thu, Nov 16, 2023 at 1:34 PM Juraj Linkeš <juraj.lin...@pantheon.tech>
> wrote:
>>
>> As I'm thinking about the filtering, it's not a trivial manner. For
>> now, I'd like to pass a class instead of multiple parameters, as that
>> will be easier to extend if we need to add to the filter. The class
>> could be a dataclass holding the various booleans. Then the capturing
>> traffic generators would need to implement a method that would
>> translate the dataclass into a TG-specific filters. I'm not sure we
>> want to introduce an abstract method for this, as the return value may
>> differ for different TGs - this needs more thought.
>
>
> I agree that making it a dataclass and having a method within Scapy for
> interpreting it for now would work. You're right there there are some other
> considerations to be had like if the return types could be different. I would
> think that for the most part a string is what would be used, but it could be
> different depending on what traffic generator you use. I know that tcpdump
> supports BPFs as they are shown here, so if we went down the route of using
> that for collecting packets with other traffic generators we would be fine on
> that front, but that might need more discussion as well when we add those
> traffic generators.
>
> If we needed this to be more generic as well, it could be possible to assign
> each traffic generator a filter type and then we could make subclasses of the
> dataclass for each filter type. This way, in the filter subclasses, they
> could specify what each of the parameters meant and how to create the filter.
> This would always return the right data type for the filter and allow us to
> implement methods for some of the more generic filters (like the one I use
> here, a BPF) rather than implementing them on the traffic generator that uses
> the filter. Then we could just use a TypeVar to generically create the
> filters based on what the capturing traffic generator class says it would
> need.
>
> I think another way we could go about this problem however is just not
> filtering out packets that are generally just noise like these and instead
> just check the packets we receive when capturing and only return ones that
> are relevant (like ones that have the same layers as the packet we sent for
> example). However, I think because of only having one traffic generator it
> would be fine to just create a method i nthe scapy capturing traffic
> generator that knows how to create BPF filters and I can do that for the next
> version if that is preferred.
>
Let's just create a method in the scapy tg (and the dataclass,
probably in traffic_generator.py?) in this patch and keep discussing
the issue for the next release(s).
>>
>>
>> On Mon, Nov 13, 2023 at 9:28 PM <jspew...@iol.unh.edu> wrote:
>> >
>> > From: Jeremy Spewock <jspew...@iol.unh.edu>
>> >
>> > Added the options to filter out LLDP and ARP packets when
>> > sniffing for packets with scapy. This was done using BPF filters to
>> > ensure that the noise these packets provide does not interfere with test
>> > cases.
>> >
>> > Signed-off-by: Jeremy Spewock <jspew...@iol.unh.edu>
>> > ---
>> > dts/framework/test_suite.py | 13 +++++++++++--
>> > .../testbed_model/capturing_traffic_generator.py | 12 +++++++++++-
>> > dts/framework/testbed_model/scapy.py | 11 ++++++++++-
>> > dts/framework/testbed_model/tg_node.py | 4 +++-
>> > 4 files changed, 35 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
>> > index 3b890c0451..3222f172f3 100644
>> > --- a/dts/framework/test_suite.py
>> > +++ b/dts/framework/test_suite.py
>> > @@ -152,7 +152,11 @@ def _configure_ipv4_forwarding(self, enable: bool) ->
>> > None:
>> > self.sut_node.configure_ipv4_forwarding(enable)
>> >
>> > def send_packet_and_capture(
>> > - self, packet: Packet, duration: float = 1
>> > + self,
>> > + packet: Packet,
>> > + duration: float = 1,
>> > + no_lldp: bool = True,
>> > + no_arp: bool = True,
>>
>> The parameters of this method are not documented in the docstring, but
>> we should add these new parameters to the docstring. The same goes for
>> the other methods (and other parameters of other methods) if they're
>> not overriding an abstract method.
>>
>> The default should be False if these are meant to be optional, but I
>> like these defaulting to True, so I'd change the wording from optional
>> to configurable.
>
>
> That is a good catch, I didn't think about the wording that way but that
> makes a lot of sense. I will make this change.
>
>>
>>
>> > ) -> list[Packet]:
>> > """
>> > Send a packet through the appropriate interface and
>> > @@ -162,7 +166,12 @@ def send_packet_and_capture(
>> > """
>> > packet = self._adjust_addresses(packet)
>> > return self.tg_node.send_packet_and_capture(
>> > - packet, self._tg_port_egress, self._tg_port_ingress, duration
>> > + packet,
>> > + self._tg_port_egress,
>> > + self._tg_port_ingress,
>> > + duration,
>> > + no_lldp,
>> > + no_arp,
>> > )
>> >
>> > def get_expected_packet(self, packet: Packet) -> Packet:
>> > diff --git a/dts/framework/testbed_model/capturing_traffic_generator.py
>> > b/dts/framework/testbed_model/capturing_traffic_generator.py
>> > index ab98987f8e..0a0d0f7d0d 100644
>> > --- a/dts/framework/testbed_model/capturing_traffic_generator.py
>> > +++ b/dts/framework/testbed_model/capturing_traffic_generator.py
>> > @@ -52,6 +52,8 @@ def send_packet_and_capture(
>> > send_port: Port,
>> > receive_port: Port,
>> > duration: float,
>> > + no_lldp: bool,
>> > + no_arp: bool,
>> > capture_name: str = _get_default_capture_name(),
>> > ) -> list[Packet]:
>> > """Send a packet, return received traffic.
>> > @@ -71,7 +73,7 @@ def send_packet_and_capture(
>> > A list of received packets. May be empty if no packets are
>> > captured.
>> > """
>> > return self.send_packets_and_capture(
>> > - [packet], send_port, receive_port, duration, capture_name
>> > + [packet], send_port, receive_port, duration, no_lldp, no_arp,
>> > capture_name
>> > )
>> >
>> > def send_packets_and_capture(
>> > @@ -80,6 +82,8 @@ def send_packets_and_capture(
>> > send_port: Port,
>> > receive_port: Port,
>> > duration: float,
>> > + no_lldp: bool,
>> > + no_arp: bool,
>> > capture_name: str = _get_default_capture_name(),
>> > ) -> list[Packet]:
>> > """Send packets, return received traffic.
>> > @@ -93,6 +97,8 @@ def send_packets_and_capture(
>> > send_port: The egress port on the TG node.
>> > receive_port: The ingress port in the TG node.
>> > duration: Capture traffic for this amount of time after
>> > sending the packets.
>> > + no_lldp: Option to disable capturing LLDP packets
>> > + no_arp: Option to disable capturing ARP packets
>> > capture_name: The name of the .pcap file where to store the
>> > capture.
>> >
>> > Returns:
>> > @@ -108,6 +114,8 @@ def send_packets_and_capture(
>> > send_port,
>> > receive_port,
>> > duration,
>> > + no_lldp,
>> > + no_arp,
>> > )
>> >
>> > self._logger.debug(
>> > @@ -123,6 +131,8 @@ def _send_packets_and_capture(
>> > send_port: Port,
>> > receive_port: Port,
>> > duration: float,
>> > + no_lldp: bool,
>> > + no_arp: bool,
>> > ) -> list[Packet]:
>> > """
>> > The extended classes must implement this method which
>> > diff --git a/dts/framework/testbed_model/scapy.py
>> > b/dts/framework/testbed_model/scapy.py
>> > index af0d4dbb25..58f01af21a 100644
>> > --- a/dts/framework/testbed_model/scapy.py
>> > +++ b/dts/framework/testbed_model/scapy.py
>> > @@ -69,6 +69,7 @@ def scapy_send_packets_and_capture(
>> > send_iface: str,
>> > recv_iface: str,
>> > duration: float,
>> > + sniff_filter: str,
>> > ) -> list[bytes]:
>> > """RPC function to send and capture packets.
>> >
>> > @@ -90,6 +91,7 @@ def scapy_send_packets_and_capture(
>> > iface=recv_iface,
>> > store=True,
>> > started_callback=lambda *args: scapy.all.sendp(scapy_packets,
>> > iface=send_iface),
>> > + filter=sniff_filter,
>> > )
>> > sniffer.start()
>> > time.sleep(duration)
>> > @@ -264,10 +266,16 @@ def _send_packets_and_capture(
>> > send_port: Port,
>> > receive_port: Port,
>> > duration: float,
>> > + no_lldp: bool,
>> > + no_arp: bool,
>> > capture_name: str = _get_default_capture_name(),
>> > ) -> list[Packet]:
>> > binary_packets = [packet.build() for packet in packets]
>> > -
>> > + sniff_filter = []
>> > + if no_lldp:
>> > + sniff_filter.append("ether[12:2] != 0x88cc")
>> > + if no_arp:
>> > + sniff_filter.append("ether[12:2] != 0x0806")
>> > xmlrpc_packets: list[
>> > xmlrpc.client.Binary
>> > ] = self.rpc_server_proxy.scapy_send_packets_and_capture(
>> > @@ -275,6 +283,7 @@ def _send_packets_and_capture(
>> > send_port.logical_name,
>> > receive_port.logical_name,
>> > duration,
>> > + " && ".join(sniff_filter),
>> > ) # type: ignore[assignment]
>> >
>> > scapy_packets = [Ether(packet.data) for packet in xmlrpc_packets]
>> > diff --git a/dts/framework/testbed_model/tg_node.py
>> > b/dts/framework/testbed_model/tg_node.py
>> > index 27025cfa31..98e55b7831 100644
>> > --- a/dts/framework/testbed_model/tg_node.py
>> > +++ b/dts/framework/testbed_model/tg_node.py
>> > @@ -56,6 +56,8 @@ def send_packet_and_capture(
>> > send_port: Port,
>> > receive_port: Port,
>> > duration: float = 1,
>> > + no_lldp: bool = True,
>> > + no_arp: bool = True,
>> > ) -> list[Packet]:
>> > """Send a packet, return received traffic.
>> >
>> > @@ -73,7 +75,7 @@ def send_packet_and_capture(
>> > A list of received packets. May be empty if no packets are
>> > captured.
>> > """
>> > return self.traffic_generator.send_packet_and_capture(
>> > - packet, send_port, receive_port, duration
>> > + packet, send_port, receive_port, duration, no_lldp, no_arp
>> > )
>> >
>> > def close(self) -> None:
>> > --
>> > 2.42.0
>> >
