Rawnetworkinterface stream #1320

JoshuaWatt · 2024-01-19T21:19:59Z

Description

Adds support for live streaming packets from a RawNetworkInterfaceDriver. This can be useful for writing tests that parse packets in real time looking for a specific pattern, for example:

import dpkt
    
drv = target.get_driver("RawNetworkInterfaceDriver")
    
with drv.record("-", timeout=60) as p:
    pcap = dpkt.pcap.Reader(p.stdout)
    for timestamp, buf in pcap:
        eth = dpkt.ethernet.Ethernet(buf)

        if _some_criteria_:
            break
    else:
        assert False, "Packet not found!"

Checklist

Documentation for the feature
Tests for the feature

The arguments and description in doc/configuration.rst have been updated

Add a section on how to use the feature to doc/usage.rst

Add a section on how to use the feature to doc/development.rst

PR has been tested

Man pages have been regenerated

JoshuaWatt · 2024-01-19T21:23:30Z

Of note, we've had a local driver that was very similar to RawNetworkInterfaceDriver for some time that was used to stream packets; the intention of this patch is to bring it in line with the functionality in that driver so we can eliminate it. For posterity, here is our driver:

import attr
import subprocess
import dpkt
from contextlib import contextmanager

from labgrid import target_factory
from labgrid.driver import Driver


@target_factory.reg_driver
@attr.s(eq=False)
class InterfaceCaptureDriver(Driver):
    """Driver to obtain tcpdump output from device's network"""

    bindings = {
        "iface": {"RemoteNetworkInterface", "USBNetworkInterface", "NetworkInterface"},
    }

    @contextmanager
    def live_capture(self, timeout=None):
        """
        Perform live capture of traffic from device's network.

        :param int timeout: When specified, performs capture for ``timeout`` seconds. \
            Defaults to `None`.

        :yields: A `dpkt.pcap.Reader` object of the captured network traffic
        """
        extra = getattr(self.iface, "extra", {})
        command = ["sudo", "tcpdump", "-i", self.iface.ifname, "-U", "-s0", "-w", "-"]

        if timeout is not None:
            command.extend(["-G", str(timeout), "-W", "1"])

        if "proxy_required" in extra:
            command = ["ssh", extra["proxy"], "--"] + command

        with subprocess.Popen(command, stdout=subprocess.PIPE) as p:
            yield dpkt.pcap.Reader(p.stdout)

codecov · 2024-01-19T21:25:06Z

Codecov Report

Attention: Patch coverage is 14.28571% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 62.7%. Comparing base (722838b) to head (d68686a).
Report is 9 commits behind head on master.

❗ Current head d68686a differs from pull request most recent head fdeb30b. Consider uploading reports for the commit fdeb30b to get more accurate results

Files	Patch %	Lines
labgrid/driver/rawnetworkinterfacedriver.py	14.2%	12 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #1320     +/-   ##
========================================
+ Coverage    62.2%   62.7%   +0.5%     
========================================
  Files         164     163      -1     
  Lines       12191   12008    -183     
========================================
- Hits         7584    7534     -50     
+ Misses       4607    4474    -133

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Bastian-Krause

Thanks! Looks good overall.

In the commit message of d68686a ("rawnetworkinterfacedriver: Implement live streaming"):

-with drv.record("-", timeout=60) as p
+with drv.record("-", timeout=60) as p:

Bastian-Krause · 2024-03-14T07:32:01Z

labgrid/driver/rawnetworkinterfacedriver.py

-        with open(filename, "wb") as outdata:
-            self._record_handle = subprocess.Popen(cmd, stdout=outdata, stderr=subprocess.PIPE)
+        if filename == "-":
+            self._record_handle = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.DEVNULL)


Wouldn't it be good to also redirect stderr to a pipe here, so a test can also make use of that? But don't make it optional.

That's not a good idea (IMHO). If you make stderr=subprocess.PIPE, then every consumer of the subprocess must periodically read from the stderr pipe, otherwise if it fills up it will deadlock the process. Making it mandatory would make the code consuming this data have to do that, which is annoying. Better would be to make it optional, although this still has problems because if you do stdout=subprocess.PIPE, stderr=subprocess.PIPE, a consumer would need to poll() on both stderr and stdout to prevent deadlock. Anyway, given how tricky this is to get correct, I think leaving it is the correct answer for now. If an error occurs, the tcpdump error output will be visible in the logs/stderr/etc. which is sufficient to debug the problem without a huge burden on the consuming code.

My expectation was that tcpdump doesn't produce too much output on stderr, but maybe that's too naive. I'm okay with keeping it as is.

helpers/labgrid-raw-interface

Bastian-Krause · 2024-03-14T07:55:16Z

labgrid/driver/rawnetworkinterfacedriver.py

+                # A timeout is expected if the context is exited early while
+                # piping to stdout
+                pass


Passing here is only correct for filename == "-", right?

helpers/labgrid-raw-interface

Bastian-Krause · 2024-03-14T08:05:12Z

helpers/labgrid-raw-interface

@@ -85,9 +96,10 @@ if __name__ == "__main__":
    parser.add_argument('program', type=str, help='program to run, either tcpreplay or tcpdump')
    parser.add_argument('interface', type=str, help='interface name')
    parser.add_argument('count', nargs="?", type=int, default=None, help='amount of frames to capture while recording')
+    parser.add_argument('--duration', type=int, default=None, help="Amount of time to capture while recording")


Is there a reason for calling this "duration" instead of "timeout"?

duration was chosen because it's distinct from the timeout argument used in rawnetworkinterface.py. It can be confusing have the same name meaning different things in that file, so I thought "duration" was a more descriptive name

labgrid/driver/rawnetworkinterfacedriver.py

jluebbe · 2024-03-14T11:35:40Z

helpers/labgrid-raw-interface

+            # The duration is implemented by specifying the number of seconds
+            # before rotating the dump file, but limiting the number of files
+            # to 1
+            args.append("-G")
+            args.append(str(duration))
+            args.append("-W")
+            args.append("1")


I suspect this might fail in the future when used in combination with writing to stdout. Handling that in the helper would mean using a subprocess instead os.execvp, so that can wait util that actually is a problem.

labgrid/driver/rawnetworkinterfacedriver.py

jluebbe · 2024-03-14T11:50:11Z

labgrid/driver/rawnetworkinterfacedriver.py

-            yield self.start_record(filename, count=count)
+            yield self.start_record(filename, count=count, timeout=timeout)
        finally:
-            self.stop_record(timeout=timeout)
+            # If live streaming packets, there is no reason to wait for tcpdump


This changes the behavior to starting the timeout when beginning the capture instead of at the end of the context. I'm not sure if anyone cares, though.

I think that change in behavior is okay.

labgrid/driver/rawnetworkinterfacedriver.py

Bastian-Krause · 2024-04-15T10:29:26Z

@JoshuaWatt I'm interested in using the changes in this PR in our network tests. Could you have a look at the review comments? If you don't have the time to integrate the changes, I'd be willing to take this over.

JoshuaWatt · 2024-04-22T18:47:25Z

@JoshuaWatt I'm interested in using the changes in this PR in our network tests. Could you have a look at the review comments? If you don't have the time to integrate the changes, I'd be willing to take this over.

I'll take a look this week. I was AFK for EOSS

Reworks the way that timeouts are handled so that instead of terminating the process in stop_record, tcpdump will exit after a set time. This is will allow for live streaming of packets in a future patch Signed-off-by: Joshua Watt <Joshua.Watt@garmin.com>

Adds support for live streaming of captured packets such that they can be processed in real time by tests instead of needing to capture for a set period of time time and do post-processing. As an example: ```python import dpkt drv = target.get_driver("RawNetworkInterfaceDriver") with drv.record(None, timeout=60) as p: pcap = dpkt.pcap.Reader(p.stdout) for timestamp, buf in pcap: eth = dpkt.ethernet.Ethernet(buf) .... ``` Signed-off-by: Joshua Watt <Joshua.Watt@garmin.com>

Bastian-Krause

Thanks for addressing the feedback. The changes look good to me. I'll try to test this within the next weeks and add my approval then.

JoshuaWatt force-pushed the rawnetworkinterface-stream branch from 9aebca2 to d68686a Compare January 19, 2024 21:20

Emantor requested a review from Bastian-Krause February 26, 2024 09:26

Bastian-Krause requested changes Mar 14, 2024

View reviewed changes

Bastian-Krause assigned JoshuaWatt Mar 14, 2024

Bastian-Krause added the enhancement label Mar 14, 2024

jluebbe reviewed Mar 14, 2024

View reviewed changes

JoshuaWatt added 2 commits April 22, 2024 15:30

JoshuaWatt force-pushed the rawnetworkinterface-stream branch from d68686a to fdeb30b Compare April 24, 2024 21:19

JoshuaWatt requested a review from Bastian-Krause April 24, 2024 22:31

Bastian-Krause reviewed Apr 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rawnetworkinterface stream #1320

Rawnetworkinterface stream #1320

JoshuaWatt commented Jan 19, 2024 •

edited

JoshuaWatt commented Jan 19, 2024

codecov bot commented Jan 19, 2024 •

edited

Bastian-Krause left a comment

Bastian-Krause Mar 14, 2024 •

edited by jluebbe

JoshuaWatt Apr 22, 2024 •

edited

Bastian-Krause Apr 25, 2024

Bastian-Krause Mar 14, 2024

Bastian-Krause Mar 14, 2024

JoshuaWatt Apr 22, 2024

jluebbe Mar 14, 2024

jluebbe Mar 14, 2024

Bastian-Krause Apr 25, 2024

Bastian-Krause commented Apr 15, 2024

JoshuaWatt commented Apr 22, 2024

Bastian-Krause left a comment

Rawnetworkinterface stream #1320

Are you sure you want to change the base?

Rawnetworkinterface stream #1320

Conversation

JoshuaWatt commented Jan 19, 2024 • edited

JoshuaWatt commented Jan 19, 2024

codecov bot commented Jan 19, 2024 • edited

Codecov Report

Bastian-Krause left a comment

Choose a reason for hiding this comment

Bastian-Krause Mar 14, 2024 • edited by jluebbe

Choose a reason for hiding this comment

JoshuaWatt Apr 22, 2024 • edited

Choose a reason for hiding this comment

Bastian-Krause Apr 25, 2024

Choose a reason for hiding this comment

Bastian-Krause Mar 14, 2024

Choose a reason for hiding this comment

Bastian-Krause Mar 14, 2024

Choose a reason for hiding this comment

JoshuaWatt Apr 22, 2024

Choose a reason for hiding this comment

jluebbe Mar 14, 2024

Choose a reason for hiding this comment

jluebbe Mar 14, 2024

Choose a reason for hiding this comment

Bastian-Krause Apr 25, 2024

Choose a reason for hiding this comment

Bastian-Krause commented Apr 15, 2024

JoshuaWatt commented Apr 22, 2024

Bastian-Krause left a comment

Choose a reason for hiding this comment

JoshuaWatt commented Jan 19, 2024 •

edited

codecov bot commented Jan 19, 2024 •

edited

Bastian-Krause Mar 14, 2024 •

edited by jluebbe

JoshuaWatt Apr 22, 2024 •

edited