Re: [PATCH net-next v10 23/23] testing/selftests: add test tool and scripts for ovpn module

[Shuah Khan <skhan@linuxfoundation.org>]

On 10/28/24 04:13, Antonio Quartulli wrote:
> On 27/10/2024 01:40, Shuah Khan wrote:
>> On 10/25/24 03:14, Antonio Quartulli wrote:
>>> The ovpn-cli tool can be compiled and used as selftest for the ovpn
>>> kernel module.
>>>
>>> It implements the netlink API and can thus be integrated in any
>>> script for more automated testing.
>>>
>>> Along with the tool, 4 scripts are added that perform basic
>>> functionality tests by means of network namespaces.
>>>
>>> Cc: shuah@kernel.org
>>> Cc: linux-kselftest@vger.kernel.org
>>> Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
>>> ---
>>>   MAINTAINERS                                        |    1 +
>>>   tools/testing/selftests/Makefile                   |    1 +
>>>   tools/testing/selftests/net/ovpn/.gitignore        |    2 +
>>>   tools/testing/selftests/net/ovpn/Makefile          |   17 +
>>>   tools/testing/selftests/net/ovpn/config            |   10 +
>>>   tools/testing/selftests/net/ovpn/data64.key        |    5 +
>>>   tools/testing/selftests/net/ovpn/ovpn-cli.c        | 2370 ++++++++++ ++++++++++
>>>   tools/testing/selftests/net/ovpn/tcp_peers.txt     |    5 +
>>>   .../testing/selftests/net/ovpn/test-chachapoly.sh  |    9 +
>>>   tools/testing/selftests/net/ovpn/test-float.sh     |    9 +
>>>   tools/testing/selftests/net/ovpn/test-tcp.sh       |    9 +
>>>   tools/testing/selftests/net/ovpn/test.sh           |  183 ++
>>>   tools/testing/selftests/net/ovpn/udp_peers.txt     |    5 +
>>>   13 files changed, 2626 insertions(+)
>>>
>>
>> What does the test output look like? Add that to the change log.
> 
> Hi Shuan,
> 
> is there any expected output for kselftest scripts?
> Right now it just prints a bunch of messages about what is being tested, plus the output from `ping` and `iperf`.
> 
> My assumption is that the output would be useful in case of failures, to understand where and what went wrong.
> 
> I can document that, but I am not sure it is truly helpful (?).
> What do you think?
> 
> Is there any specific output format I should obey to?
> 
> 
> [...]
> 
> 
>>> +
>>> +static void usage(const char *cmd)
>>> +{
>>> +    fprintf(stderr,
>>> +        "Usage %s <command> <iface> [arguments..]\n",
>>> +        cmd);
>>> +    fprintf(stderr, "where <command> can be one of the following\n\n");
>>> +
>>> +    fprintf(stderr, "* new_iface <iface> [mode]: create new ovpn interface\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tmode:\n");
>>> +    fprintf(stderr, "\t\t- P2P for peer-to-peer mode (i.e. client)\n");
>>> +    fprintf(stderr, "\t\t- MP for multi-peer mode (i.e. server)\n");
>>> +
>>> +    fprintf(stderr, "* del_iface <iface>: delete ovpn interface\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* listen <iface> <lport> <peers_file> [ipv6]: listen for incoming peer TCP connections\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tlport: TCP port to listen to\n");
>>> +    fprintf(stderr,
>>> +        "\tpeers_file: file containing one peer per line: Line format:\n");
>>> +    fprintf(stderr, "\t\t<peer_id> <vpnaddr>\n");
>>> +    fprintf(stderr,
>>> +        "\tipv6: whether the socket should listen to the IPv6 wildcard address\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* connect <iface> <peer_id> <raddr> <rport> [key_file]: start connecting peer of TCP-based VPN session\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tpeer_id: peer ID of the connecting peer\n");
>>> +    fprintf(stderr, "\traddr: peer IP address to connect to\n");
>>> +    fprintf(stderr, "\trport: peer TCP port to connect to\n");
>>> +    fprintf(stderr,
>>> +        "\tkey_file: file containing the symmetric key for encryption\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* new_peer <iface> <peer_id> <lport> <raddr> <rport> [vpnaddr]: add new peer\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tlport: local UDP port to bind to\n");
>>> +    fprintf(stderr,
>>> +        "\tpeer_id: peer ID to be used in data packets to/from this peer\n");
>>> +    fprintf(stderr, "\traddr: peer IP address\n");
>>> +    fprintf(stderr, "\trport: peer UDP port\n");
>>> +    fprintf(stderr, "\tvpnaddr: peer VPN IP\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* new_multi_peer <iface> <lport> <peers_file>: add multiple peers as listed in the file\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tlport: local UDP port to bind to\n");
>>> +    fprintf(stderr,
>>> +        "\tpeers_file: text file containing one peer per line. Line format:\n");
>>> +    fprintf(stderr, "\t\t<peer_id> <raddr> <rport> <vpnaddr>\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* set_peer <iface> <peer_id> <keepalive_interval> <keepalive_timeout>: set peer attributes\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tpeer_id: peer ID of the peer to modify\n");
>>> +    fprintf(stderr,
>>> +        "\tkeepalive_interval: interval for sending ping messages\n");
>>> +    fprintf(stderr,
>>> +        "\tkeepalive_timeout: time after which a peer is timed out\n");
>>> +
>>> +    fprintf(stderr, "* del_peer <iface> <peer_id>: delete peer\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tpeer_id: peer ID of the peer to delete\n");
>>> +
>>> +    fprintf(stderr, "* get_peer <iface> [peer_id]: retrieve peer(s) status\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr,
>>> +        "\tpeer_id: peer ID of the peer to query. All peers are returned if omitted\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* new_key <iface> <peer_id> <slot> <key_id> <cipher> <key_dir> <key_file>: set data channel key\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr,
>>> +        "\tpeer_id: peer ID of the peer to configure the key for\n");
>>> +    fprintf(stderr, "\tslot: either 1 (primary) or 2 (secondary)\n");
>>> +    fprintf(stderr, "\tkey_id: an ID from 0 to 7\n");
>>> +    fprintf(stderr,
>>> +        "\tcipher: cipher to use, supported: aes (AES-GCM), chachapoly (CHACHA20POLY1305)\n");
>>> +    fprintf(stderr,
>>> +        "\tkey_dir: key direction, must 0 on one host and 1 on the other\n");
>>> +    fprintf(stderr, "\tkey_file: file containing the pre-shared key\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* del_key <iface> <peer_id> [slot]: erase existing data channel key\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tpeer_id: peer ID of the peer to modify\n");
>>> +    fprintf(stderr, "\tslot: slot to erase. PRIMARY if omitted\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* get_key <iface> <peer_id> <slot>: retrieve non sensible key data\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tpeer_id: peer ID of the peer to query\n");
>>> +    fprintf(stderr, "\tslot: either 1 (primary) or 2 (secondary)\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* swap_keys <iface> <peer_id>: swap content of primary and secondary key slots\n");
>>> +    fprintf(stderr, "\tiface: ovpn interface name\n");
>>> +    fprintf(stderr, "\tpeer_id: peer ID of the peer to modify\n");
>>> +
>>> +    fprintf(stderr,
>>> +        "* listen_mcast: listen to ovpn netlink multicast messages\n");
>>> +}
>>
>> If this test is run from "make kselftest" as default run does this usage
>> output show up in the report?
> 
> No.
> This usage is only printed when invoking ovpn-cli with wrong arguments and this can't be the case in the kselftest.

The usage() is great and much needed. My concern is if this usage would show up
when we run "make kselftest" - some tests do this by adding wrapper shell script
to run the test with different options to cover all the cases.

> 
> 
> Other than documenting the output, do you think there is any other critical part to be adjusted in this patch?

No - I don't have any other comments on the test itself. I just want to make
sure this usage inadvertently doesn't end up in the "make kselftest" run.

With that

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah