Re: [PATCH v1 1/1] dts: Remove XML-RPC server for Scapy TG and instead use PythonShell

[Jeremy Spewock <jspewock@iol.unh.edu>]

On Fri, Jun 21, 2024 at 10:14 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
>
>
> > +    #: Padding to add to the start of a line for python syntax compliance.
> > +    _padding: ClassVar[str] = " " * 4
>
> We use padding in the context of packets so let's use something else
> here, such as _python_indentation.

I didn't think of that, good call.

>
> > +
> >       def __init__(self, tg_node: Node, config: ScapyTrafficGeneratorConfig):
> >           """Extend the constructor with Scapy TG specifics.
> >
> > -        The traffic generator first starts an XML-RPC on the remote `tg_node`.
> > -        Then it populates the server with functions which use the Scapy library
> > -        to send/receive traffic:
> > -
> > -            * :func:`scapy_send_packets_and_capture`
> > -            * :func:`scapy_send_packets`
> > -
> > -        To enable verbose logging from the xmlrpc client, use the :option:`--verbose`
> > -        command line argument or the :envvar:`DTS_VERBOSE` environment variable.
> > +        Initializes both a traffic generator and an interactive shell to handle Scapy functions.
>
> Should these be with the definite article (instead of a and an)?

Yeah, I can see how that would make sense. I'll update this.

>
> > +        The interactive shell will be started on `tg_node`.
> >
> >           Args:
> >               tg_node: The node where the traffic generator resides.
> >               config: The traffic generator's test run configuration.
> >           """
> > -        super().__init__(tg_node, config)
> > +        CapturingTrafficGenerator.__init__(self, tg_node, config)
> > +        PythonShell.__init__(self, tg_node, privileged=True)
> >
>
> This should probably mirror the superclass order from which the class is
> subclassed - PythonShell first, then CapturingTrafficGenerator.

Ack.

>
> I'm also thinking of using super() here, but it's a bit convoluted. We'd
> need to do something like this:
>
> class Common:
>      def __init__(self, *args, **kwargs):
>          super().__init__()
>
> class TrafficGenerator(Common):
>      def __init__(self, tg_node, config, **kwargs):
>          super().__init__(tg_node, **kwargs)
>
> class InteractiveShell(Common):
>      def __init__(self, node, privileged = False, timeout = 15,
> start_on_init = True, params = "", **kwargs):
>          super().__init__(node, **kwargs)
>
> This works for both mro's. The common class basically makes the two
> subclasses eligible for multiple interitance while still working with
> normal inheritance (e.g. when making an instance of regular PythonShell)
> and the super() calls in the subclasses define what they have in common.
>
> Since we only have this one use case, it may be overkill to use super()
> instead of calling both __init__()s directly. But with super(), we
> should be able to use any traffic generator with any interactive shell.
> I don't know whether we'll need that (we can use T-Rex without an
> interactive shell, but maybe using one is superior), but it could be useful.

It could be useful, but it does increase the complexity a little bit.
I could put it together and see how it looks. It definitely would be
better to use super so we don't have to unnecessarily override the
init method in the future, the only thing that would deter me slightly
is the seemingly random `**kwargs` that don't get used inside the
interactive shell class. Of course this ambiguity goes away if it's
documented, so this shouldn't really be a problem.

>
> >           assert (
> >               self._tg_node.config.os == OS.linux
> >           ), "Linux is the only supported OS for scapy traffic generation"
> >
> > -        self.session = PythonShell(
> > -            self._tg_node, timeout=5, privileged=True, name="ScapyXMLRPCServer"
> > -        )
> > +    def start_application(self) -> None:
> > +        """Extends :meth:`framework.remote_session.interactive_shell.start_application`.
> >
> > -        # import libs in remote python console
> > -        for import_statement in SCAPY_RPC_SERVER_IMPORTS:
> > -            self.session.send_command(import_statement)
> > +        Adds a command that imports everything from the scapy library immediately after starting
>
> This sound as if we were adding the command to some sort of internal
> list for processing later. We can probably just say "Import everything
> ... in the remote shell.". I think it would be valuable to explicitly
> say where the import happens.

That makes sense, I'll make the change.

>
> > +        the shell for usage in later calls to the methods of this class.
> > +        """
> > +        super().start_application()
> > +        self.send_command("from scapy.all import *")
> >
>
> It's possible we should check that the import was successful or do some
> other check that Scapy is indeed present and throw an exception if it isn't.
>

That's not a bad idea, I could try and just use something from the
scapy library and check the output.

> Actually, we probably should add a smoke test that would do that - some
> basic verification of traffic generators.

This idea I like a lot! We've had the traffic generator itself
implemented for a while now, a smoke test that ensures traffic can
flow in general would definitely be beneficial.

>