Message-ID: <2f178d43-8a40-4f1a-b8cf-85d26ad0a063@openvpn.net>
Date: Tue, 29 Oct 2024 10:42:09 +0100
From: Antonio Quartulli <antonio@...nvpn.net>
To: Shuah Khan <skhan@...uxfoundation.org>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 Donald Hunter <donald.hunter@...il.com>, Andrew Lunn <andrew@...n.ch>,
 Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
 sd@...asysnail.net, Shuah Khan <shuah@...nel.org>, ryazanov.s.a@...il.com,
 Eric Dumazet <edumazet@...gle.com>, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v10 23/23] testing/selftests: add test tool and
 scripts for ovpn module



On 29/10/2024 03:29, Shuah Khan wrote:
> 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@...nel.org
>>>> Cc: linux-kselftest@...r.kernel.org
>>>> Signed-off-by: Antonio Quartulli <antonio@...nvpn.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@...uxfoundation.org>

Thanks a lot for your feedback.
I promise no usage() output will pollute the reports :-)

Ok then, I will extend the commit message with a description of the 
output and retain your Reviewed-by in v11.

I'll try to send it out today.

Thanks.
Regards,

> 
> thanks,
> -- Shuah

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists