[v3,1/4] dts: add send_packets to test suites and rework packet addressing

From: Jeremy Spewock <jspewock@iol.unh.edu>

Currently the only method provided in the test suite class for sending
packets sends a single packet and then captures the results. There is,
in some cases, a need to send multiple packets at once while not really
needing to capture any traffic received back. The method to do this
exists in the traffic generator already, but this patch exposes the
method to test suites.

This patch also updates the _adjust_addresses method of test suites so
that addresses of packets are only modified if the developer did not
configure them beforehand. This allows for developers to have more
control over the content of their packets when sending them through the
framework.

Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
---
 dts/framework/test_suite.py            | 74 ++++++++++++++++++--------
 dts/framework/testbed_model/tg_node.py |  9 ++++
 2 files changed, 62 insertions(+), 21 deletions(-)
  

This is great, I'll be using this in-favor of the boolean solution
that I implemented! Just to bring this to your attention. I am
currently working on some Generic Routing Encapsulation suites the
require multiple IP layers at packet creation; they look something
like:

Ether() / IP() / GRE() / IP() / UDP() / Raw(load='x'*80)

I have to take a deeper look to see how multiple IP layers affect the
declaration of src and dst variables, I'll let you know what I find as
some changes might be needed on this implementation to avoid future
bugs. Once I figure it out, I'll leave a review tag for you.

On Wed, Jul 24, 2024 at 11:07 AM <jspewock@iol.unh.edu> wrote:
>
> From: Jeremy Spewock <jspewock@iol.unh.edu>
>
> Currently the only method provided in the test suite class for sending
> packets sends a single packet and then captures the results. There is,
> in some cases, a need to send multiple packets at once while not really
> needing to capture any traffic received back. The method to do this
> exists in the traffic generator already, but this patch exposes the
> method to test suites.
>
> This patch also updates the _adjust_addresses method of test suites so
> that addresses of packets are only modified if the developer did not
> configure them beforehand. This allows for developers to have more
> control over the content of their packets when sending them through the
> framework.
>
> Signed-off-by: Jeremy Spewock <jspewock@iol.unh.edu>
> ---
>  dts/framework/test_suite.py            | 74 ++++++++++++++++++--------
>  dts/framework/testbed_model/tg_node.py |  9 ++++
>  2 files changed, 62 insertions(+), 21 deletions(-)
>
> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> index 694b2eba65..0b678ed62d 100644
> --- a/dts/framework/test_suite.py
> +++ b/dts/framework/test_suite.py
> @@ -199,7 +199,7 @@ def send_packet_and_capture(
>          Returns:
>              A list of received packets.
>          """
> -        packet = self._adjust_addresses(packet)
> +        packet = self._adjust_addresses([packet])[0]
>          return self.tg_node.send_packet_and_capture(
>              packet,
>              self._tg_port_egress,
> @@ -208,6 +208,18 @@ def send_packet_and_capture(
>              duration,
>          )
>
> +    def send_packets(
> +        self,
> +        packets: list[Packet],
> +    ) -> None:
> +        """Send packets using the traffic generator and do not capture received traffic.
> +
> +        Args:
> +            packets: Packets to send.
> +        """
> +        packets = self._adjust_addresses(packets)
> +        self.tg_node.send_packets(packets, self._tg_port_egress)
> +
>      def get_expected_packet(self, packet: Packet) -> Packet:
>          """Inject the proper L2/L3 addresses into `packet`.
>
> @@ -219,39 +231,59 @@ def get_expected_packet(self, packet: Packet) -> Packet:
>          """
>          return self._adjust_addresses(packet, expected=True)
>
> -    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
> +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
>          """L2 and L3 address additions in both directions.
>
> +        Only missing addresses are added to packets, existing addressed will not be overridden.
> +
>          Assumptions:
>              Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
>
>          Args:
> -            packet: The packet to modify.
> +            packets: The packets to modify.
>              expected: If :data:`True`, the direction is SUT -> TG,
>                  otherwise the direction is TG -> SUT.
>          """
> -        if expected:
> -            # The packet enters the TG from SUT
> -            # update l2 addresses
> -            packet.src = self._sut_port_egress.mac_address
> -            packet.dst = self._tg_port_ingress.mac_address
> +        ret_packets = []
> +        for packet in packets:
> +            default_pkt_src = type(packet)().src
> +            default_pkt_dst = type(packet)().dst
> +            default_pkt_payload_src = IP().src if hasattr(packet.payload, "src") else None
> +            default_pkt_payload_dst = IP().dst if hasattr(packet.payload, "dst") else None
> +            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
> +            # packet leaves the TG towards the SUT
>
> -            # The packet is routed from TG egress to TG ingress
> -            # update l3 addresses
> -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> -        else:
> -            # The packet leaves TG towards SUT
>              # update l2 addresses
> -            packet.src = self._tg_port_egress.mac_address
> -            packet.dst = self._sut_port_ingress.mac_address
> +            if packet.src == default_pkt_src:
> +                packet.src = (
> +                    self._sut_port_egress.mac_address
> +                    if expected
> +                    else self._tg_port_egress.mac_address
> +                )
> +            if packet.dst == default_pkt_dst:
> +                packet.dst = (
> +                    self._tg_port_ingress.mac_address
> +                    if expected
> +                    else self._sut_port_ingress.mac_address
> +                )
> +
> +            # The packet is routed from TG egress to TG ingress regardless of if it is expected or
> +            # not.
>
> -            # The packet is routed from TG egress to TG ingress
>              # update l3 addresses
> -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> -
> -        return Ether(packet.build())
> +            if (
> +                default_pkt_payload_src is not None
> +                and packet.payload.src == default_pkt_payload_src
> +            ):
> +                packet.payload.src = self._tg_ip_address_egress.ip.exploded
> +            if (
> +                default_pkt_payload_dst is not None
> +                and packet.payload.dst == default_pkt_payload_dst
> +            ):
> +                packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> +            ret_packets.append(Ether(packet.build()))
> +
> +        return ret_packets
>
>      def verify(self, condition: bool, failure_description: str) -> None:
>          """Verify `condition` and handle failures.
> diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py
> index 4ee326e99c..758b676258 100644
> --- a/dts/framework/testbed_model/tg_node.py
> +++ b/dts/framework/testbed_model/tg_node.py
> @@ -83,6 +83,15 @@ def send_packet_and_capture(
>              duration,
>          )
>
> +    def send_packets(self, packets: list[Packet], port: Port):
> +        """Send packets without capturing resulting received packets.
> +
> +        Args:
> +            packets: Packets to send.
> +            port: Port to send the packets on.
> +        """
> +        self.traffic_generator.send_packets(packets, port)
> +
>      def close(self) -> None:
>          """Free all resources used by the node.
>
> --
> 2.45.2
>
  

I'll make sure to look over the other parts of this series and leave
reviews at some point next week, but I prioritized this since I will
be using this patch at some point in my GRE suites.


> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> index 694b2eba65..0b678ed62d 100644
> --- a/dts/framework/test_suite.py
> +++ b/dts/framework/test_suite.py
> @@ -199,7 +199,7 @@ def send_packet_and_capture(
>          Returns:
>              A list of received packets.
>          """
> -        packet = self._adjust_addresses(packet)
> +        packet = self._adjust_addresses([packet])[0]
>          return self.tg_node.send_packet_and_capture(
>              packet,
>              self._tg_port_egress,
> @@ -208,6 +208,18 @@ def send_packet_and_capture(
>              duration,
>          )
>
> +    def send_packets(
> +        self,
> +        packets: list[Packet],
> +    ) -> None:
> +        """Send packets using the traffic generator and do not capture received traffic.
> +
> +        Args:
> +            packets: Packets to send.
> +        """
> +        packets = self._adjust_addresses(packets)
> +        self.tg_node.send_packets(packets, self._tg_port_egress)
> +
>      def get_expected_packet(self, packet: Packet) -> Packet:
>          """Inject the proper L2/L3 addresses into `packet`.
>
> @@ -219,39 +231,59 @@ def get_expected_packet(self, packet: Packet) -> Packet:
>          """
>          return self._adjust_addresses(packet, expected=True)
>
> -    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
> +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
>          """L2 and L3 address additions in both directions.
>
> +        Only missing addresses are added to packets, existing addressed will not be overridden.

addressed should be addresses. Only saw this because of Chrome's
built-in grammar correction.

> +
>          Assumptions:
>              Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
>
>          Args:
> -            packet: The packet to modify.
> +            packets: The packets to modify.
>              expected: If :data:`True`, the direction is SUT -> TG,
>                  otherwise the direction is TG -> SUT.
>          """
> -        if expected:
> -            # The packet enters the TG from SUT
> -            # update l2 addresses
> -            packet.src = self._sut_port_egress.mac_address
> -            packet.dst = self._tg_port_ingress.mac_address
> +        ret_packets = []
> +        for packet in packets:
> +            default_pkt_src = type(packet)().src
> +            default_pkt_dst = type(packet)().dst

This is really just a probing question for my sake, but what is the
difference between the solution you have above type(packet)().src and
Ether().src? Is there a preferred means of doing this?

> +            default_pkt_payload_src = IP().src if hasattr(packet.payload, "src") else None
> +            default_pkt_payload_dst = IP().dst if hasattr(packet.payload, "dst") else None
> +            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
> +            # packet leaves the TG towards the SUT
>
> -            # The packet is routed from TG egress to TG ingress
> -            # update l3 addresses
> -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded

This is where it gets a little tricky. There will be circumstances,
albeit probably infrequently, where a user-created packet has more
than one IP layer, such as the ones I am using in the ipgre and nvgre
test suites that I am writing. In these cases, you need to specify an
index of the IP layer you want to modify, otherwise it will modify the
outermost IP layer in the packet (the IP layer outside the GRE layer.
See my previous comment for an example packet). Should be pretty easy
to fix, you just need to check if a packet contains an GRE layer, and
if it does, modify the packet by doing something like
packet[IP][1].src = self._tg_ip_address_egress.ip.exploded.

> -        else:
> -            # The packet leaves TG towards SUT
>              # update l2 addresses
> -            packet.src = self._tg_port_egress.mac_address
> -            packet.dst = self._sut_port_ingress.mac_address

You wouldn't need to make changes to how Ether addresses get allocated
if accounting for GRE as described above, since I'm pretty sure there
aren't really circumstances where packets would have more than one
Ethernet header, at least not that I've seen (GRE packets only have
one).

> +            if packet.src == default_pkt_src:
> +                packet.src = (
> +                    self._sut_port_egress.mac_address
> +                    if expected
> +                    else self._tg_port_egress.mac_address
> +                )
> +            if packet.dst == default_pkt_dst:
> +                packet.dst = (
> +                    self._tg_port_ingress.mac_address
> +                    if expected
> +                    else self._sut_port_ingress.mac_address
> +                )
> +
> +            # The packet is routed from TG egress to TG ingress regardless of if it is expected or

Maybe change 'regardless of if it is expected' to 'regardless of
whether it is expected.' It's admittedly picky of me, but I think it
reads a little bit better.

> +            # not.
>
> -            # The packet is routed from TG egress to TG ingress
>              # update l3 addresses
> -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> -
> -        return Ether(packet.build())
> +            if (
> +                default_pkt_payload_src is not None
> +                and packet.payload.src == default_pkt_payload_src
> +            ):
> +                packet.payload.src = self._tg_ip_address_egress.ip.exploded
> +            if (
> +                default_pkt_payload_dst is not None
> +                and packet.payload.dst == default_pkt_payload_dst
> +            ):
> +                packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> +            ret_packets.append(Ether(packet.build()))
> +
> +        return ret_packets
>
>      def verify(self, condition: bool, failure_description: str) -> None:
>          """Verify `condition` and handle failures.
> diff --git a/dts/framework/testbed_model/tg_node.py b/dts/framework/testbed_model/tg_node.py
> index 4ee326e99c..758b676258 100644
> --- a/dts/framework/testbed_model/tg_node.py
> +++ b/dts/framework/testbed_model/tg_node.py
> @@ -83,6 +83,15 @@ def send_packet_and_capture(
>              duration,
>          )
>
> +    def send_packets(self, packets: list[Packet], port: Port):
> +        """Send packets without capturing resulting received packets.
> +
> +        Args:
> +            packets: Packets to send.
> +            port: Port to send the packets on.
> +        """
> +        self.traffic_generator.send_packets(packets, port)
> +
>      def close(self) -> None:
>          """Free all resources used by the node.
>
> --
> 2.45.2
>
  

Thanks for the comments, I just had one clarifying question about
them, but otherwise I will address them in the next version.

On Fri, Jul 26, 2024 at 3:00 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> I'll make sure to look over the other parts of this series and leave
> reviews at some point next week, but I prioritized this since I will
> be using this patch at some point in my GRE suites.
>
>
> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> > index 694b2eba65..0b678ed62d 100644
> > --- a/dts/framework/test_suite.py
> > +++ b/dts/framework/test_suite.py
> > @@ -199,7 +199,7 @@ def send_packet_and_capture(
> >          Returns:
> >              A list of received packets.
> >          """
> > -        packet = self._adjust_addresses(packet)
> > +        packet = self._adjust_addresses([packet])[0]
> >          return self.tg_node.send_packet_and_capture(
> >              packet,
> >              self._tg_port_egress,
> > @@ -208,6 +208,18 @@ def send_packet_and_capture(
> >              duration,
> >          )
> >
> > +    def send_packets(
> > +        self,
> > +        packets: list[Packet],
> > +    ) -> None:
> > +        """Send packets using the traffic generator and do not capture received traffic.
> > +
> > +        Args:
> > +            packets: Packets to send.
> > +        """
> > +        packets = self._adjust_addresses(packets)
> > +        self.tg_node.send_packets(packets, self._tg_port_egress)
> > +
> >      def get_expected_packet(self, packet: Packet) -> Packet:
> >          """Inject the proper L2/L3 addresses into `packet`.
> >
> > @@ -219,39 +231,59 @@ def get_expected_packet(self, packet: Packet) -> Packet:
> >          """
> >          return self._adjust_addresses(packet, expected=True)
> >
> > -    def _adjust_addresses(self, packet: Packet, expected: bool = False) -> Packet:
> > +    def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
> >          """L2 and L3 address additions in both directions.
> >
> > +        Only missing addresses are added to packets, existing addressed will not be overridden.
>
> addressed should be addresses. Only saw this because of Chrome's
> built-in grammar correction.

Good catch.

>
> > +
> >          Assumptions:
> >              Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
> >
> >          Args:
> > -            packet: The packet to modify.
> > +            packets: The packets to modify.
> >              expected: If :data:`True`, the direction is SUT -> TG,
> >                  otherwise the direction is TG -> SUT.
> >          """
> > -        if expected:
> > -            # The packet enters the TG from SUT
> > -            # update l2 addresses
> > -            packet.src = self._sut_port_egress.mac_address
> > -            packet.dst = self._tg_port_ingress.mac_address
> > +        ret_packets = []
> > +        for packet in packets:
> > +            default_pkt_src = type(packet)().src
> > +            default_pkt_dst = type(packet)().dst
>
> This is really just a probing question for my sake, but what is the
> difference between the solution you have above type(packet)().src and
> Ether().src? Is there a preferred means of doing this?

There isn't really a functional difference at all under the assumption
that every packet we send will start with an Ethernet header. This
obviously isn't an unreasonable assumption to make, so maybe I was
reaching for flexibility that isn't really needed here by making it
work with any theoretical first layer that has a source address. I
wanted to do the same thing for the payload, but that causes issues
when the following layer with an address isn't the very next layer
after Ether.

>
> > +            default_pkt_payload_src = IP().src if hasattr(packet.payload, "src") else None
> > +            default_pkt_payload_dst = IP().dst if hasattr(packet.payload, "dst") else None
> > +            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
> > +            # packet leaves the TG towards the SUT
> >
> > -            # The packet is routed from TG egress to TG ingress
> > -            # update l3 addresses
> > -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> > -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
>
> This is where it gets a little tricky. There will be circumstances,
> albeit probably infrequently, where a user-created packet has more
> than one IP layer, such as the ones I am using in the ipgre and nvgre
> test suites that I am writing. In these cases, you need to specify an
> index of the IP layer you want to modify, otherwise it will modify the
> outermost IP layer in the packet (the IP layer outside the GRE layer.
> See my previous comment for an example packet). Should be pretty easy
> to fix, you just need to check if a packet contains an GRE layer, and
> if it does, modify the packet by doing something like
> packet[IP][1].src = self._tg_ip_address_egress.ip.exploded.

I'm not as familiar with how GRE affects the packets, do you need to
have the address on the inner IP layer at all times, or are you saying
you need it on both IP layers?

>
> > -        else:
> > -            # The packet leaves TG towards SUT
> >              # update l2 addresses
> > -            packet.src = self._tg_port_egress.mac_address
> > -            packet.dst = self._sut_port_ingress.mac_address
>
> You wouldn't need to make changes to how Ether addresses get allocated
> if accounting for GRE as described above, since I'm pretty sure there
> aren't really circumstances where packets would have more than one
> Ethernet header, at least not that I've seen (GRE packets only have
> one).
>
> > +            if packet.src == default_pkt_src:
> > +                packet.src = (
> > +                    self._sut_port_egress.mac_address
> > +                    if expected
> > +                    else self._tg_port_egress.mac_address
> > +                )
> > +            if packet.dst == default_pkt_dst:
> > +                packet.dst = (
> > +                    self._tg_port_ingress.mac_address
> > +                    if expected
> > +                    else self._sut_port_ingress.mac_address
> > +                )
> > +
> > +            # The packet is routed from TG egress to TG ingress regardless of if it is expected or
>
> Maybe change 'regardless of if it is expected' to 'regardless of
> whether it is expected.' It's admittedly picky of me, but I think it
> reads a little bit better.

Ack.

>
> > +            # not.
> >
> > -            # The packet is routed from TG egress to TG ingress
> >              # update l3 addresses
> > -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> > -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
<snip>
> >
  

Hi Jeremy, sorry for the delay! See my comments below.

<snip>
> > >          Assumptions:
> > >              Two links between SUT and TG, one link is TG -> SUT, the other SUT -> TG.
> > >
> > >          Args:
> > > -            packet: The packet to modify.
> > > +            packets: The packets to modify.
> > >              expected: If :data:`True`, the direction is SUT -> TG,
> > >                  otherwise the direction is TG -> SUT.
> > >          """
> > > -        if expected:
> > > -            # The packet enters the TG from SUT
> > > -            # update l2 addresses
> > > -            packet.src = self._sut_port_egress.mac_address
> > > -            packet.dst = self._tg_port_ingress.mac_address
> > > +        ret_packets = []
> > > +        for packet in packets:
> > > +            default_pkt_src = type(packet)().src
> > > +            default_pkt_dst = type(packet)().dst
> >
> > This is really just a probing question for my sake, but what is the
> > difference between the solution you have above type(packet)().src and
> > Ether().src? Is there a preferred means of doing this?
>
> There isn't really a functional difference at all under the assumption
> that every packet we send will start with an Ethernet header. This
> obviously isn't an unreasonable assumption to make, so maybe I was
> reaching for flexibility that isn't really needed here by making it
> work with any theoretical first layer that has a source address. I
> wanted to do the same thing for the payload, but that causes issues
> when the following layer with an address isn't the very next layer
> after Ether.

Makes sense to me! It's probably best to not to make the Ether
assumption regardless of whether or not it will likely always be
present.

>
> >
> > > +            default_pkt_payload_src = IP().src if hasattr(packet.payload, "src") else None
> > > +            default_pkt_payload_dst = IP().dst if hasattr(packet.payload, "dst") else None
> > > +            # If `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
> > > +            # packet leaves the TG towards the SUT
> > >
> > > -            # The packet is routed from TG egress to TG ingress
> > > -            # update l3 addresses
> > > -            packet.payload.src = self._tg_ip_address_egress.ip.exploded
> > > -            packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
> >
> > This is where it gets a little tricky. There will be circumstances,
> > albeit probably infrequently, where a user-created packet has more
> > than one IP layer, such as the ones I am using in the ipgre and nvgre
> > test suites that I am writing. In these cases, you need to specify an
> > index of the IP layer you want to modify, otherwise it will modify the
> > outermost IP layer in the packet (the IP layer outside the GRE layer.
> > See my previous comment for an example packet). Should be pretty easy
> > to fix, you just need to check if a packet contains an GRE layer, and
> > if it does, modify the packet by doing something like
> > packet[IP][1].src = self._tg_ip_address_egress.ip.exploded.
>
> I'm not as familiar with how GRE affects the packets, do you need to
> have the address on the inner IP layer at all times, or are you saying
> you need it on both IP layers?

Basically, GRE is a header that encapsulates a traditional packet.
Practically speaking, this means that a scapy packet with GRE will
look something like 'Ether() / IP() / GRE() / IP() / UDP() / Raw()'.
If you try to modify layer 3 addresses in the way the framework does
it now (packet.payload.src), and more than one IP layer is present in
a given packet, it will modify the the front-most IP layer (in this
case, the IP layer before the GRE layer is the packet I listed
before). If there are multiple IP layers, you can choose which layer
you want to modify by doing something like 'packet[IP][1] = address'
to modify the inner IP layer.

It is my understanding that GRE packets need to have an inner IP layer
as well as an outer IP layer. Here is a quick readup on what GRE is
(scroll to the bottom of the article and look at the diagram of a
regular datagram vs a GRE datagram as the rest of the article isn't
super important).

https://ipwithease.com/generic-routing-encapsulation-gre/
<snip>

-Nicholas