From: Jeremy Spewock <jspewock@iol.unh.edu>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
Cc: Honnappa.Nagarahalli@arm.com, thomas@monjalon.net,
wathsala.vithanage@arm.com, probb@iol.unh.edu,
paul.szczepanek@arm.com, yoan.picchi@foss.arm.com,
ferruh.yigit@amd.com, andrew.rybchenko@oktetlabs.ru,
dev@dpdk.org
Subject: Re: [PATCH v3 5/7] dts: add optional packet filtering to scapy sniffer
Date: Fri, 17 Nov 2023 13:05:53 -0500 [thread overview]
Message-ID: <CAAA20USAgRdpqb36MQx=sLiq-SJXAUkoPCPt20nsxn1K6acMwQ@mail.gmail.com> (raw)
In-Reply-To: <CAOb5WZYUcOGszAxBN5KoaxBVYqwQMfDkB0xsOCAZGqCgqo6U_w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 10629 bytes --]
On Thu, Nov 16, 2023 at 1:34 PM Juraj Linkeš <juraj.linkes@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.
>
> On Mon, Nov 13, 2023 at 9:28 PM <jspewock@iol.unh.edu> wrote:
> >
> > From: Jeremy Spewock <jspewock@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 <jspewock@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
> >
>
[-- Attachment #2: Type: text/html, Size: 13608 bytes --]
next prev parent reply other threads:[~2023-11-17 18:06 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-13 20:28 [PATCH v3 0/7] dts: Port scatter suite over jspewock
2023-11-13 20:28 ` [PATCH v3 1/7] dts: Add scatter test suite jspewock
2023-11-15 7:04 ` Patrick Robb
2023-11-16 19:20 ` Juraj Linkeš
2023-11-21 19:26 ` Jeremy Spewock
2023-11-22 8:47 ` Juraj Linkeš
2023-11-13 20:28 ` [PATCH v3 2/7] dts: add waiting for port up in testpmd jspewock
2023-11-16 19:05 ` Juraj Linkeš
2023-11-17 18:09 ` Jeremy Spewock
2023-11-13 20:28 ` [PATCH v3 3/7] dts: add scatter to the yaml schema jspewock
2023-11-13 20:28 ` [PATCH v3 4/7] dts: allow passing parameters into interactive apps jspewock
2023-11-16 18:52 ` Juraj Linkeš
2023-11-17 18:08 ` Jeremy Spewock
2023-11-20 14:35 ` Juraj Linkeš
2023-11-21 21:55 ` Jeremy Spewock
2023-11-13 20:28 ` [PATCH v3 5/7] dts: add optional packet filtering to scapy sniffer jspewock
2023-11-16 18:34 ` Juraj Linkeš
2023-11-17 18:05 ` Jeremy Spewock [this message]
2023-11-20 14:31 ` Juraj Linkeš
2023-11-13 20:28 ` [PATCH v3 6/7] dts: add pci addresses to EAL parameters jspewock
2023-11-16 18:10 ` Juraj Linkeš
2023-11-17 17:13 ` Jeremy Spewock
2023-11-13 20:28 ` [PATCH v3 7/7] dts: allow configuring MTU of ports jspewock
2023-11-16 18:05 ` Juraj Linkeš
2023-11-17 17:06 ` Jeremy Spewock
2023-11-16 19:23 ` [PATCH v3 0/7] dts: Port scatter suite over Juraj Linkeš
2023-12-14 22:10 ` [PATCH v4 " jspewock
2023-12-14 22:10 ` [PATCH v4 1/7] dts: add required methods to testpmd_shell jspewock
2023-12-14 22:10 ` [PATCH v4 2/7] dts: allow passing parameters into interactive apps jspewock
2023-12-14 22:10 ` [PATCH v4 3/7] dts: add optional packet filtering to scapy sniffer jspewock
2023-12-14 22:10 ` [PATCH v4 4/7] dts: add pci addresses to EAL parameters jspewock
2023-12-14 22:10 ` [PATCH v4 5/7] dts: allow configuring MTU of ports jspewock
2023-12-14 22:10 ` [PATCH v4 6/7] dts: add scatter to the yaml schema jspewock
2023-12-14 22:10 ` [PATCH v4 7/7] dts: add scatter test suite jspewock
2023-12-18 17:22 ` [PATCH v4 0/7] dts: Port scatter suite over jspewock
2023-12-18 17:22 ` [PATCH v4 1/7] dts: add required methods to testpmd_shell jspewock
2023-12-18 17:22 ` [PATCH v4 2/7] dts: allow passing parameters into interactive apps jspewock
2023-12-18 17:22 ` [PATCH v4 3/7] dts: add optional packet filtering to scapy sniffer jspewock
2023-12-18 17:22 ` [PATCH v4 4/7] dts: add pci addresses to EAL parameters jspewock
2023-12-18 17:22 ` [PATCH v4 5/7] dts: allow configuring MTU of ports jspewock
2023-12-18 17:22 ` [PATCH v4 6/7] dts: add scatter to the yaml schema jspewock
2023-12-18 17:22 ` [PATCH v4 7/7] dts: add scatter test suite jspewock
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAA20USAgRdpqb36MQx=sLiq-SJXAUkoPCPt20nsxn1K6acMwQ@mail.gmail.com' \
--to=jspewock@iol.unh.edu \
--cc=Honnappa.Nagarahalli@arm.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=juraj.linkes@pantheon.tech \
--cc=paul.szczepanek@arm.com \
--cc=probb@iol.unh.edu \
--cc=thomas@monjalon.net \
--cc=wathsala.vithanage@arm.com \
--cc=yoan.picchi@foss.arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).