Re: [libvirt] [PATCH] network: restrict usage of port management APIs

On Wed, Aug 08, 2018 at 11:41:23PM -0400, Laine Stump wrote: > On 08/08/2018 11:46 AM, Daniel P. Berrangé wrote: >> The port allocation APIs are currently called unconditionally for all >> types of NIC, but (mostly) only do anything for NICs with type=network. >> >> The exception is the port allocate API which does some validation even >> for NICs with type!=network. Relying on this validation is flawed, >> however, since the network driver may not even be installed, so virt >> drivers must not delegation validation to it for NICs with >> type!=network. > Although I never thought through all the minute details to the end (and > didn't need to because ,,,AllocateActualDevice() wasn't in a public > API), I had always figured that these calls into the network driver > would be where, eventually, we would do all of the plumbing for a > network device, like creating the tap/macvtap device during startup, and > disconnecting/deleting devices during domain shutdown. (it also kind of > makes sense for nwfilters to be added/removed by the network driver > during these APIs, since the filterref is in the <interface> definition). > > That's the reason for the unconditional calls. > > (one of the "minute details" that I hadn't thought about was the fact > that we probably can't do *all* of the plumbing at one time - at the > very least we need to have one API call for creating the devices and > hooking them up, and another call that happens right before the guest > CPUs are started (to bring everything online). Still, if we limit the > netAllocate API to only being called for type='network' then we > definitely will need an additional API that will likely be called > unconditionally just after netAllocation is called just for type='network'.) Yep, this is a bigger problem than I first considered. We have four hypervisors using tap devices (qemu, lxc, libxl & bhyve) and all of them have different requirements for the way the tap devices are created. This is going to make it hard to delegate everything to the network driver, as the work is hypervisor specific.

I'm trying to tackle this is distinct achieveable phases. Even the current AllocateActualDevice() method is doing too much at the same time. For example, it deals with allocating the resources inside the network driver (ie reserving a VF or hostdev), as well as populating the virDomainNetActualDef struct, and invoking hook scripts. This makes it too tangled up with the hyervisor drivers - for example needing a full domain XML is very undesirable.

> BTW, once it is the responsibility of the network driver to create tap > devices and connect them to bridges, the network driver will no longer > be optional (unless we want to duplicate the code in the hypervisor > drivers), but that is the price we have to pay in order to give > unprivileged libvirt the ability to use any type of network device. Even if the network driver can create tap devices, I'd still want to deal with type=network vs type=bridge separately for access control purposes. It is desirable to be able to have ACLs on the opening operation and the natural place to attach the ACL is against the virNetworkPtr object. We have no object to represent standalone bridge devices outside of a virNetworkPtr, so nothing to attach an ACL to for creating tap devices.