Message-ID: <056588a7-de1b-4416-8553-750c8d20dc97@openvpn.net>
Date: Tue, 8 Oct 2024 11:16:01 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Jiri Pirko <jiri@...nulli.us>, ryazanov.s.a@...il.com
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Donald Hunter <donald.hunter@...il.com>,
 Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 sd@...asysnail.net
Subject: Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support

On 08/10/2024 10:58, Jiri Pirko wrote:
> Tue, Oct 08, 2024 at 10:01:40AM CEST, antonio@...nvpn.net wrote:
>> Hi,
>>
>> On 07/10/24 17:32, Jiri Pirko wrote:
>>> Wed, Oct 02, 2024 at 11:02:17AM CEST, antonio@...nvpn.net wrote:
>>>
>>> [...]
>>>
>>>
>>>> +operations:
>>>> +  list:
>>>> +    -
>>>> +      name: dev-new
>>>> +      attribute-set: ovpn
>>>> +      flags: [ admin-perm ]
>>>> +      doc: Create a new interface of type ovpn
>>>> +      do:
>>>> +        request:
>>>> +          attributes:
>>>> +            - ifname
>>>> +            - mode
>>>> +        reply:
>>>> +          attributes:
>>>> +            - ifname
>>>> +            - ifindex
>>>> +    -
>>>> +      name: dev-del
>>>
>>> Why you expose new and del here in ovn specific generic netlink iface?
>>> Why can't you use the exising RTNL api which is used for creation and
>>> destruction of other types of devices?
>>
>> That was my original approach in v1, but it was argued that an ovpn interface
>> needs a userspace program to be configured and used in a meaningful way,
>> therefore it was decided to concentrate all iface mgmt APIs along with the
>> others in the netlink family and to not expose any RTNL ops.
> 
> Can you please point me to the message id?

<CAHNKnsQnHAdxC-XhC9RP-cFp0d-E4YGb+7ie3WymXVL9N-QS6A@...l.gmail.com> 
from Sergey and subsequent replies.
RTNL vs NL topic starts right after the definition of 'ovpn_link_ops'

Recently Kuniyuki commented on this topic as well in:
<20240919055259.17622-1-kuniyu@...zon.com>
and that is why I added a default dellink implemetation.

> 
> 
>>
>> However, recently we decided to add a dellink implementation for better
>> integration with network namespaces and to allow the user to wipe a dangling
>> interface.
> 
> Hmm, one more argument to have symmetric add/del impletentation in RTNL
> 
> 
>>
>> In the future we are planning to also add the possibility to create a
>> "persistent interface", that is an interface created before launching any
>> userspace program and that survives when the latter is stopped.
>> I can guess this functionality may be better suited for RTNL, but I am not
>> sure yet.
> 
> That would be quite confusing to have RTNL and genetlink iface to
> add/del device. From what you described above, makes more sent to have
> it just in RTNL

All in all I tend to agree.

> 
>>
>> @Jiri: do you have any particular opinion why we should use RTNL ops and not
>> netlink for creating/destroying interfaces? I feel this is mostly a matter of
>> taste, but maybe there are technical reasons we should consider.
> 
> Well. technically, you can probabaly do both. But it is quite common
> that you can add/delete these kind of devices over RTNL. Lots of
> examples. People are used to it, aligns with existing flows.

The only counterargument I see is the one brought by Sergey: "the ovpn 
interface is not usable after creation, if no openvpn process is running".

However, allowing to create "persistent interfaces" will define a 
use-case for having an ovpn device without any userspace process.

@Sergey what is your opinion here? I am not sure persistent interfaces 
were discussed at the time you brought your point about RTNL vs NL.


Regards,


> 
>>
>> Thanks a lot for your contribution.
>>
>> Regards,
>>
>>
>>>
>>>
>>> ip link add [link DEV | parentdev NAME] [ name ] NAME
>>> 		    [ txqueuelen PACKETS ]
>>> 		    [ address LLADDR ]
>>> 		    [ broadcast LLADDR ]
>>> 		    [ mtu MTU ] [index IDX ]
>>> 		    [ numtxqueues QUEUE_COUNT ]
>>> 		    [ numrxqueues QUEUE_COUNT ]
>>> 		    [ netns { PID | NETNSNAME | NETNSFILE } ]
>>> 		    type TYPE [ ARGS ]
>>>
>>> ip link delete { DEVICE | dev DEVICE | group DEVGROUP } type TYPE [ ARGS ]
>>>
>>> Lots of examples of existing types creation is for example here:
>>> https://developers.redhat.com/blog/2018/10/22/introduction-to-linux-interfaces-for-virtual-networking
>>>
>>>
>>>
>>>> +      attribute-set: ovpn
>>>> +      flags: [ admin-perm ]
>>>> +      doc: Delete existing interface of type ovpn
>>>> +      do:
>>>> +        pre: ovpn-nl-pre-doit
>>>> +        post: ovpn-nl-post-doit
>>>> +        request:
>>>> +          attributes:
>>>> +            - ifindex
>>>
>>> [...]
>>
>> -- 
>> Antonio Quartulli
>> OpenVPN Inc.

-- 
Antonio Quartulli
OpenVPN Inc.

Powered by blists - more mailing lists