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

[Antonio Quartulli]

On 08/10/2024 14:52, Jiri Pirko wrote:
> Tue, Oct 08, 2024 at 11:16:01AM CEST, antonio@xxxxxxxxxxx wrote:
> > On 08/10/2024 10:58, Jiri Pirko wrote:
> > > Tue, Oct 08, 2024 at 10:01:40AM CEST, antonio@xxxxxxxxxxx wrote:
> > > > Hi,
> > > >
> > > > On 07/10/24 17:32, Jiri Pirko wrote:
> > > > > Wed, Oct 02, 2024 at 11:02:17AM CEST, antonio@xxxxxxxxxxx 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > 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.