Re: [PATCH net-next v8 03/24] ovpn: add basic netlink support

[Antonio Quartulli <antonio@openvpn.net>]

On 08/10/2024 14:52, Jiri Pirko wrote:
> Tue, Oct 08, 2024 at 11:16:01AM CEST, antonio@openvpn.net wrote:
>> On 08/10/2024 10:58, Jiri Pirko wrote:
>>> Tue, Oct 08, 2024 at 10:01:40AM CEST, antonio@openvpn.net wrote:
>>>> Hi,
>>>>
>>>> On 07/10/24 17:32, Jiri Pirko wrote:
>>>>> Wed, Oct 02, 2024 at 11:02:17AM CEST, antonio@openvpn.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@mail.gmail.com> from
>> Sergey and subsequent replies.
>> RTNL vs NL topic starts right after the definition of 'ovpn_link_ops'
> 
> Yeah, does not make sense to me. All devices should implement common
> rtnl ops, the extra-config, if needed, could be on a separate channel.
> I don't find Sergey's argumentation valid.

Thanks a lot for taking the time to read our conversation.

Ok, considering all points we have discussed so far (including future 
developments already on the roadmap), I think it makes sense to go back 
to RTNL and drop the new/del-dev ops from the netlink family.

Will do that in v9.

Regards,

> 
> 
>>
>> Recently Kuniyuki commented on this topic as well in:
>> <20240919055259.17622-1-kuniyu@amazon.com>
>> and that is why I added a default dellink implemetation.
> 
> Having dellink without newlink implemented is just wrong.
> 
> 
>>
>>>
>>>
>>>>
>>>> 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.

-- 
Antonio Quartulli
OpenVPN Inc.