diff mbox series

[ovs-dev,RFC] sendpkt: Allow definitions via Scapy as well as hex bytes.

Message ID 20250207144552.1702528-1-aconole@redhat.com
State RFC
Headers show
Series [ovs-dev,RFC] sendpkt: Allow definitions via Scapy as well as hex bytes. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/cirrus-robot success cirrus build: passed

Commit Message

Aaron Conole Feb. 7, 2025, 2:45 p.m. UTC
When writing tests it can be very difficult to understand what
the test is doing without properly documenting the test.  Added
to that difficulty is when a stream of bytes gets posted with
the patch as a packet definition.  We then need to write out
detailed descriptions of the packets, and tend to get packet
strings that break line length parsers (leading to errors applying
patches.

With this change, we can write out clear descriptions of packets
that make use of Scapy to generate the actual bytes strings.
This should serve as accurate documentation, while also trimming
the amount of bytes a developer needs to write to represent a
packet.

Signed-off-by: Aaron Conole <aconole@redhat.com>
---
NOTE: I'm submitting it here as RFC, because I'm a bit nervous
      with the eval() call I've baked into the parser routine.
      Probably it's okay because users just trust the test suite
      anyway.

 tests/sendpkt.py | 64 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

Comments

Ilya Maximets Feb. 12, 2025, 1:27 p.m. UTC | #1
On 2/7/25 15:45, Aaron Conole wrote:
> When writing tests it can be very difficult to understand what
> the test is doing without properly documenting the test.  Added
> to that difficulty is when a stream of bytes gets posted with
> the patch as a packet definition.  We then need to write out
> detailed descriptions of the packets, and tend to get packet
> strings that break line length parsers (leading to errors applying
> patches.
> 
> With this change, we can write out clear descriptions of packets
> that make use of Scapy to generate the actual bytes strings.
> This should serve as accurate documentation, while also trimming
> the amount of bytes a developer needs to write to represent a
> packet.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> ---
> NOTE: I'm submitting it here as RFC, because I'm a bit nervous
>       with the eval() call I've baked into the parser routine.
>       Probably it's okay because users just trust the test suite
>       anyway.
> 

Hi, Aaron.  Thanks for the patch, but I'm not sure if we should do that.
A few things about scapy:

1. The distribution packages have a very large amount of dependencies,
   so it's not a very desirable thing to install, unless installed
   from pip.  And installing from pip is another can of worms.

2. Importing scapy is generally slow, unless we cherry-pick small
   modules to import, but then there might not be a point in using
   it at all.

We have an 'ovs-ofctl compose-packet' that can generate almost any type of
a packet that OVS can parse.  A few things that it can't generate include:

1. Intentionally crafted malformed packets that we use for regression
   testing.  Scapy can't really help more than our existing tools with
   that.

2. Fragmented packets.  Not sure if scapy can help here, but it feels
   like we should be extending the compose-packet to support fragments
   instead.

3. IGMP.  The only potential use case for scapy as OpenFlow doesn't
   support IGMP fields.  But might make sense to just add a special
   support for this to the compose-packet.

One other benefit of using compose-packet is also that we're testing
our own packet parse/compose code that doesn't get a lot of testing
otherwise.

Also, OVN has a scapy service utility already implemented, so we may
port that instead, if absolutely necessary.  (OVN needs scapy, because
it has to generate a lot of different packet types for various protocols
that do not make sense for OVS.)

Best regards, Ilya Maximets.
Aaron Conole Feb. 18, 2025, 1:06 p.m. UTC | #2
Ilya Maximets <i.maximets@ovn.org> writes:

> On 2/7/25 15:45, Aaron Conole wrote:
>> When writing tests it can be very difficult to understand what
>> the test is doing without properly documenting the test.  Added
>> to that difficulty is when a stream of bytes gets posted with
>> the patch as a packet definition.  We then need to write out
>> detailed descriptions of the packets, and tend to get packet
>> strings that break line length parsers (leading to errors applying
>> patches.
>> 
>> With this change, we can write out clear descriptions of packets
>> that make use of Scapy to generate the actual bytes strings.
>> This should serve as accurate documentation, while also trimming
>> the amount of bytes a developer needs to write to represent a
>> packet.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> ---
>> NOTE: I'm submitting it here as RFC, because I'm a bit nervous
>>       with the eval() call I've baked into the parser routine.
>>       Probably it's okay because users just trust the test suite
>>       anyway.
>> 
>
> Hi, Aaron.  Thanks for the patch, but I'm not sure if we should do that.
> A few things about scapy:
>
> 1. The distribution packages have a very large amount of dependencies,
>    so it's not a very desirable thing to install, unless installed
>    from pip.  And installing from pip is another can of worms.

This is a good point.

> 2. Importing scapy is generally slow, unless we cherry-pick small
>    modules to import, but then there might not be a point in using
>    it at all.
>
> We have an 'ovs-ofctl compose-packet' that can generate almost any type of
> a packet that OVS can parse.  A few things that it can't generate include:
>
> 1. Intentionally crafted malformed packets that we use for regression
>    testing.  Scapy can't really help more than our existing tools with
>    that.
>
> 2. Fragmented packets.  Not sure if scapy can help here, but it feels
>    like we should be extending the compose-packet to support fragments
>    instead.

That makes sense.

> 3. IGMP.  The only potential use case for scapy as OpenFlow doesn't
>    support IGMP fields.  But might make sense to just add a special
>    support for this to the compose-packet.
>
> One other benefit of using compose-packet is also that we're testing
> our own packet parse/compose code that doesn't get a lot of testing
> otherwise.
>
> Also, OVN has a scapy service utility already implemented, so we may
> port that instead, if absolutely necessary.  (OVN needs scapy, because
> it has to generate a lot of different packet types for various protocols
> that do not make sense for OVS.)

Okay - it's probably worth looking into adding the support to compose
packet.  BTW, this was triggered because a recent patch from Paolo ended
up being flagged malformed (patchwork issue, IIUC).  And then when
trying to read the packets (and having done the read/write work before),
I realized there are certainly better ways that we could be expressing
this.  Maybe scapy isn't the ideal one - but probably we could do
something with compose-packet (or other facility, too).

> Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/tests/sendpkt.py b/tests/sendpkt.py
index 7cbea51654..32af99b308 100755
--- a/tests/sendpkt.py
+++ b/tests/sendpkt.py
@@ -25,10 +25,66 @@ 
 #
 
 
+import re
 import socket
 import sys
 from optparse import OptionParser
 
+try:
+    from scapy.all import Ether, IP, IPv6, TCP, UDP, ARP, ICMP, ICMPv6ND_RA
+    from scapy.all import ICMPv6NDOptSrcLLAddr, ICMPv6NDOptMTU
+    from scapy.all import ICMPv6NDOptPrefixInfo
+    from scapy.all import ICMPv6EchoRequest, ICMPv6EchoReply
+    SCAPY_PROTO = {
+        "Ether": Ether,
+        "IP": IP,
+        "IPv6": IPv6,
+        "TCP": TCP,
+        "UDP": UDP,
+        "ARP": ARP,
+        "ICMP": ICMP,
+        "ICMPv6ND_RA": ICMPv6ND_RA,
+        "ICMPv6NDOptSrcLLAddr": ICMPv6NDOptSrcLLAddr,
+        "ICMPv6NDOptMTU": ICMPv6NDOptMTU,
+        "ICMPv6NDOptPrefixInfo": ICMPv6NDOptPrefixInfo,
+        "ICMPv6EchoRequest": ICMPv6EchoRequest,
+        "ICMPv6EchoReply": ICMPv6EchoReply,
+    }
+
+    def scapy_parse(packet_def):
+        pkt_layers = packet_def.split("/")
+
+        pkt = None
+
+        for layer in pkt_layers:
+            # Word(...) match
+            lm = re.match(r'(\w+)\((.*?)\)', layer)
+            if not lm:
+                raise ValueError(
+                    f"Invalid definition {packet_def} at layer {layer}")
+
+            proto, proto_args_str = lm.groups()
+            if proto not in SCAPY_PROTO:
+                raise ValueError("Unable to construct a packet with {proto}.")
+
+            proto_args = {}
+            if proto_args_str:
+                kvp = re.findall(r'(\w)=(?:\'([^\']*)\'|([\w.]+))',
+                                 proto_args_str)
+                for key, str_type, n_type in kvp:
+                    proto_args[key] = str_type if str_type else eval(n_type)
+
+            layer_obj = SCAPY_PROTO[proto](**proto_args)
+            if pkt is None:
+                pkt = layer_obj
+            else:
+                pkt /= layer_obj
+
+        return pkt
+
+except:
+    def scapy_parse(packet_def):
+        raise RuntimeError("No scapy module while trying to parse scapy def.")
 
 usage = "usage: %prog [OPTIONS] OUT-INTERFACE HEX-BYTES \n \
          bytes in HEX-BYTES must be separated by space(s)"
@@ -50,8 +106,12 @@  if options.packet_type != "eth":
 
 # Strip '0x' prefixes from hex input, combine into a single string and
 # convert to bytes.
-hex_str = "".join([a[2:] if a.startswith("0x") else a for a in args[1:]])
-pkt = bytes.fromhex(hex_str)
+try:
+    hex_str = "".join([a[2:] if a.startswith("0x") else a for a in args[1:]])
+    pkt = bytes.fromhex(hex_str)
+except ValueError:
+    parsed_pkt_obj = scapy_parse(args[1])
+    pkt = bytes(parsed_pkt_obj)
 
 try:
     sockfd = socket.socket(socket.AF_PACKET, socket.SOCK_RAW)