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

[Laine Stump]

On 08/09/2018 03:58 AM, Daniel P. Berrangé wrote:
> 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.

The differences between qemu and lxc were on my mind but, without
looking, I had naively assumed that "all the others" (including libxl
and bhve) were just doing something similar to qemu :-/

>
> 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.

I've always been hopeful that we could figure out some way to
consolidate everything needed for setup/teardown into just a few well
defined actions (to avoid an explosion of APIs that, as a side effect,
are difficult to explain). But as always, "Easy to do" is easy to say
(and again I find myself using ":-/")


Anyway, as long as I know you're considering this, I'm sure you've got a
better picture in your brain than I do, so

Reviewed-by: Laine Stump <laine at laine.org>

for the patch.

>
>> 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.

So are you saying that the network driver simply wouldn't be able to
create the tap devices for type=bridge (or type=direct)? I'm not exactly
following you (although I don't doubt what you say).