[v3,1/1] dts: rework packet addressing
Checks
Commit Message
From: Jeremy Spewock <jspewock@iol.unh.edu>
This patch 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 | 75 ++++++++++++++++++++++++++-----------
1 file changed, 54 insertions(+), 21 deletions(-)
Comments
> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> + def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
> + # The packet is routed from TG egress to TG ingress regardless of whether it is
> + # expected or not.
> + if ip_src_is_unset:
> + l3_to_use.src = self._tg_ip_address_egress.ip.exploded
> +
> + if ip_dst_is_unset:
> + l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
So this is where l3_to_use also appears. This could also be in the same
if branch, right? As you mentioned, ip_src_is_unset is only going to be
true in that branch.
Now that I look at it, we're mixing the update of l2 addresses (starting
with pkt_src_is_unset = "src" not in packet.fields) with l3 addresses
(starting with num_ip_layers right below that). We could first do l2
addresses, then l3 addresses. And I don't think we even need the
*_is_unset variables, they're only used once.
> + ret_packets.append(Ether(packet.build()))
>
> - return Ether(packet.build())
> + return ret_packets
>
> def verify(self, condition: bool, failure_description: str) -> None:
> """Verify `condition` and handle failures.
On Thu, Sep 26, 2024 at 8:31 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
>
> > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
>
> > + def _adjust_addresses(self, packets: list[Packet], expected: bool = False) -> list[Packet]:
> > + # The packet is routed from TG egress to TG ingress regardless of whether it is
> > + # expected or not.
> > + if ip_src_is_unset:
> > + l3_to_use.src = self._tg_ip_address_egress.ip.exploded
> > +
> > + if ip_dst_is_unset:
> > + l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
>
> So this is where l3_to_use also appears. This could also be in the same
> if branch, right? As you mentioned, ip_src_is_unset is only going to be
> true in that branch.
>
> Now that I look at it, we're mixing the update of l2 addresses (starting
> with pkt_src_is_unset = "src" not in packet.fields) with l3 addresses
> (starting with num_ip_layers right below that). We could first do l2
> addresses, then l3 addresses. And I don't think we even need the
> *_is_unset variables, they're only used once.
That is true, I can change it to work in this way instead.
>
> > + ret_packets.append(Ether(packet.build()))
> >
> > - return Ether(packet.build())
> > + return ret_packets
> >
> > def verify(self, condition: bool, failure_description: str) -> None:
> > """Verify `condition` and handle failures.
>
@@ -225,7 +225,7 @@ def send_packets_and_capture(
Returns:
A list of received packets.
"""
- packets = [self._adjust_addresses(packet) for packet in packets]
+ packets = self._adjust_addresses(packets)
return self.tg_node.send_packets_and_capture(
packets,
self._tg_port_egress,
@@ -243,41 +243,74 @@ def get_expected_packet(self, packet: Packet) -> Packet:
Returns:
`packet` with injected L2/L3 addresses.
"""
- return self._adjust_addresses(packet, expected=True)
+ return self._adjust_addresses([packet], expected=True)[0]
- 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.
+ Packets in `packets` will be directly modified in this method. The returned list of packets
+ however will be copies of the modified packets in order to keep the two lists distinct.
+
+ Only missing addresses are added to packets, existing addresses will not be overridden. If
+ any packet in `packets` has multiple IP layers (using GRE, for example) only the inner-most
+ IP layer will have its addresses adjusted.
+
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.
+
+ Returns:
+ A list containing copies of all packets in `packets` after modification.
"""
- 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:
+ # The fields parameter of a packet does not include fields of the payload, so this can
+ # only be the Ether src/dst.
+ pkt_src_is_unset = "src" not in packet.fields
+ pkt_dst_is_unset = "dst" not in packet.fields
+ num_ip_layers = packet.layers().count(IP)
+
+ if num_ip_layers > 0:
+ # Update the last IP layer if there are multiple (the framework should be modifying
+ # the packet address instead of the tunnel address if there is one).
+ l3_to_use = packet.getlayer(IP, num_ip_layers)
+ ip_src_is_unset = "src" not in l3_to_use.fields
+ ip_dst_is_unset = "dst" not in l3_to_use.fields
+ else:
+ ip_src_is_unset = None
+ ip_dst_is_unset = None
- # 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 `expected` is :data:`True`, the packet enters the TG from SUT, otherwise the
+ # packet leaves the TG towards the SUT
+ if pkt_src_is_unset:
+ packet.src = (
+ self._sut_port_egress.mac_address
+ if expected
+ else self._tg_port_egress.mac_address
+ )
+ if pkt_dst_is_unset:
+ 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
# update l3 addresses
- packet.payload.src = self._tg_ip_address_egress.ip.exploded
- packet.payload.dst = self._tg_ip_address_ingress.ip.exploded
+ # The packet is routed from TG egress to TG ingress regardless of whether it is
+ # expected or not.
+ if ip_src_is_unset:
+ l3_to_use.src = self._tg_ip_address_egress.ip.exploded
+
+ if ip_dst_is_unset:
+ l3_to_use.dst = self._tg_ip_address_ingress.ip.exploded
+ ret_packets.append(Ether(packet.build()))
- return Ether(packet.build())
+ return ret_packets
def verify(self, condition: bool, failure_description: str) -> None:
"""Verify `condition` and handle failures.