From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AF3184337F; Mon, 20 Nov 2023 15:32:13 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 30EFE42DDE; Mon, 20 Nov 2023 15:32:13 +0100 (CET) Received: from mail-ed1-f51.google.com (mail-ed1-f51.google.com [209.85.208.51]) by mails.dpdk.org (Postfix) with ESMTP id ED80842DD2 for ; Mon, 20 Nov 2023 15:32:11 +0100 (CET) Received: by mail-ed1-f51.google.com with SMTP id 4fb4d7f45d1cf-548c548c40aso1586343a12.0 for ; Mon, 20 Nov 2023 06:32:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700490731; x=1701095531; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=rAFURLtNsNyaLqo/GydngIaRG3VO38Ok/Z3TZ2HPA04=; b=MaSAMNu+k/TA8e1jWfK+W1aI0drKdd2xUq9TQ/2C86a4yjf5EtwoAD5+akjzD9eULj MK+xjK7juUXOfS/9/O9S0KwMTgJpUXaMSOjH3PwsD1YvaG+Rius9pkWSgkySniJoiBZI YNZuFCuzwHltSwBM96i7jUVhjVhR9eEBlN1ztOsvyYjtnXPUVyDqY6kiJ/BmGv942cj8 43tQpj7XUmpVE4M3JKHV7Ez8+XkVmdQH+O8TDuSl3TbVkLz/qNuNwXY8zaVfTDCI9rBZ fh68mZ/UzU9sitTfRc8YUESXBucid4k+9Pim3V6l5KdE3SF50PRfaMosLNpYl+Gg5zwL R7VA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700490731; x=1701095531; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=rAFURLtNsNyaLqo/GydngIaRG3VO38Ok/Z3TZ2HPA04=; b=Cp1e8OOkSHXLYJFbTL0fNgdjmh9DjfUHEE9ZdnQ6vGhWLH9iO6djNyDFCf35qkauqV 0UGFedS+KMtPxNNy2KNvvgoQV+ad/tsB2VghV+C7R/X6RgEyFSLFV1cLhnkQFLafqiE1 MJ89fc/2V8sn+KSL0NnFnqgIhQVWXo9zsCBpY1BpNa8CGGhu5Jo9RNCD8DiaZa889h0o LHWs3cCw9aN2gJpqhCNtRXifhkNk0mVI1dqAAAACDL3RR79t7XEhrZn/41UdKfYfGd2s WczeeQ0/iLvDWU/sDaKRK0BUGxGKYIyjiIUp44TnL/eULUDHB7L+efdaoMToHE65Jc90 pSTQ== X-Gm-Message-State: AOJu0Yy78JIIBql6be+DPI5EhjkQ4o7qb3Mr1NlDC5nL+oXyNWcGU79Q hzFsHhiEBELaURHxiSejiVTbI5lpgy9zrlTqQvqupw== X-Google-Smtp-Source: AGHT+IHSexTZRpPaQ4hPdk28c+oW2yDOebqEIFi8JDlWKPR4nsSD2N/PuUrLatmqEAkTtcr9z16jGVeqicx7254KrZc= X-Received: by 2002:a17:906:20da:b0:9e5:26b2:b38d with SMTP id c26-20020a17090620da00b009e526b2b38dmr1908738ejc.10.1700490730677; Mon, 20 Nov 2023 06:32:10 -0800 (PST) MIME-Version: 1.0 References: <20231113202833.12900-1-jspewock@iol.unh.edu> <20231113202833.12900-6-jspewock@iol.unh.edu> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Mon, 20 Nov 2023 15:31:59 +0100 Message-ID: Subject: Re: [PATCH v3 5/7] dts: add optional packet filtering to scapy sniffer To: Jeremy Spewock 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, Nov 17, 2023 at 7:06=E2=80=AFPM Jeremy Spewock wrote: > > > > On Thu, Nov 16, 2023 at 1:34=E2=80=AFPM Juraj Linke=C5=A1 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 i= nterpreting 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 wou= ld think that for the most part a string is what would be used, but it coul= d be different depending on what traffic generator you use. I know that tcp= dump 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 ad= d those traffic generators. > > If we needed this to be more generic as well, it could be possible to ass= ign 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 all= ow us to implement methods for some of the more generic filters (like the o= ne I use here, a BPF) rather than implementing them on the traffic generato= r that uses the filter. Then we could just use a TypeVar to generically cre= ate 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 fi= ltering out packets that are generally just noise like these and instead ju= st check the packets we receive when capturing and only return ones that ar= e relevant (like ones that have the same layers as the packet we sent for e= xample). However, I think because of only having one traffic generator it w= ould be fine to just create a method i nthe scapy capturing traffic generat= or that knows how to create BPF filters and I can do that for the next vers= ion 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=E2=80=AFPM wrote: >> > >> > From: Jeremy Spewock >> > >> > 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 te= st >> > cases. >> > >> > Signed-off-by: Jeremy Spewock >> > --- >> > 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 =3D 1 >> > + self, >> > + packet: Packet, >> > + duration: float =3D 1, >> > + no_lldp: bool =3D True, >> > + no_arp: bool =3D 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 =3D self._adjust_addresses(packet) >> > return self.tg_node.send_packet_and_capture( >> > - packet, self._tg_port_egress, self._tg_port_ingress, dura= tion >> > + 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.p= y 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 =3D _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 a= re 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 =3D _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 s= ending 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 t= he 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/test= bed_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=3Drecv_iface, >> > store=3DTrue, >> > started_callback=3Dlambda *args: scapy.all.sendp(scapy_packet= s, iface=3Dsend_iface), >> > + filter=3Dsniff_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 =3D _get_default_capture_name(), >> > ) -> list[Packet]: >> > binary_packets =3D [packet.build() for packet in packets] >> > - >> > + sniff_filter =3D [] >> > + if no_lldp: >> > + sniff_filter.append("ether[12:2] !=3D 0x88cc") >> > + if no_arp: >> > + sniff_filter.append("ether[12:2] !=3D 0x0806") >> > xmlrpc_packets: list[ >> > xmlrpc.client.Binary >> > ] =3D 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 =3D [Ether(packet.data) for packet in xmlrpc_pa= ckets] >> > diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/te= stbed_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 =3D 1, >> > + no_lldp: bool =3D True, >> > + no_arp: bool =3D 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 a= re captured. >> > """ >> > return self.traffic_generator.send_packet_and_capture( >> > - packet, send_port, receive_port, duration >> > + packet, send_port, receive_port, duration, no_lldp, no_ar= p >> > ) >> > >> > def close(self) -> None: >> > -- >> > 2.42.0 >> >