[libvirt] Plan A or Plan B?

[Laine Stump]

On 11/19/2012 08:31 AM, Gene Czarcinski wrote:
> Plan A:
>
> This consists of the set of patches I have submitted for conf-file,
> DHCPv6, etc.  The response has been a resounding SILENCE.  Thus, I am
> assuming that there is some reluctance to adopting these changes in
> their current form.  This especially includes the conf-file changes. 
> While I believe that changing dnsmasq parameters from the command-line
> to a configuration file makes a great deal of sense and might have
> been the approach if this was being done today, there is resistance to
> change.

At least on my part, you're misinterpreting the lack of response as
reluctance to change, when it really is just due to a lack of time to
draft a response.

I have no problems with switching to using a conf file (although, as
I've said a couple of times, pointing to a conf *directory* which could
contain admin-supplied additions to the configuration is unlikely to be
accepted - we do need an alternative to that, though; I think the method
most likely to succeed would be to add a private dnsmasq:commandline
namespace, as we've already done for qemu).

 
http://berrange.com/posts/2011/12/19/using-command-line-arg-monitor-command-passthrough-with-libvirt-and-kvm/

Here is an open BZ that mentions the idea of adding similar
functionality for dnsmasq (see comment 8, in particular):

  https://bugzilla.redhat.com/show_bug.cgi?id=824573

This is "the right way" to do this, since it makes sure that all
configuration information (including for knobs that we don't support) is
available in one place.

One of the things that's actually taken up some of my time in the last
few days is that I'm looking at an open CVE about dnsmasq and its use in
libvirt:

   https://bugzilla.redhat.com/show_bug.cgi?id=833033
   https://bugzilla.redhat.com/show_bug.cgi?id=874702

Fixing this also requires changing the dnsmasq commandline dependent
upon the version of dnsmasq running on the host. In this case, though,
we can't rely on the version number, but need to check for presence of a
particular option in the output of "dnsmasq --help" (this is because
there will be backports of the new dnsmasq option to older versions of
dnsmasq on various distros that can't/won't upgrade to a new release,
but still require the CVE fix, and yet we can't completely refuse to run
if someone hasn't upgraded their dnsmasq, because the problem doesn't
occur in the majority of configurations).

Since there will be conflicts with your changes, and the libvirt fixes
also will need to be backported to older versions of libvirt, the CVE
fix will need to be pushed before your patches (in order to make the
backport patch as similar to the upstream patch as possible)

In the meantime, this bug has shown that we really need a
general-purpose "capabilities" mechanism for dnsmasq similar to what we
have for qemu, so I'm making a trimmed down version of that.

This is my top priority right now, so hang on for a couple days and I'll
see how much conflict it creates with your patches (one thing is that
the mechanism for getting the dnsmasq version will be different).

So before you go re-arranging all your patches, give me a couple days to
work out the patch for the CVE and see how it sits with your current
patches.

>
> OK, so here is Plan B:
>
> I am removing the conf-file and related changes.  They will be
> repackaged and resubmitted at some later time.  The new multi-file
> patch will focus on "IPV6 Enhanced Support" which will consist of the
> following:
>
> 1.  In a manner similar to what is done for IPV6, add ip6tables rules
> to permit virtual systems to communicate via a defined virtual
> interface which has no gateway addresses defined.  This does mean that
> virtual systems will not be able to communicate with the host via this
> interface ... only with each other.  Also, the following must be:
>       net.ipv6.conf.virbr19.disable_ipv6 = 1
> so that the kernel does not start anything.

This discussion was left open at the end - Dan, do you see any problem
with adding the rules permitting IPv6 traffic between the guests as long
as the host has disable_ipv6 set? Or will we still need to add an
"ipv6='yes'" attribute to the toplevel <network> element?

>
> This implements IPv6 functionality currently available for IPv4.
>
> Documentation will be added to explain the functionality for both IPv4
> and IPv6.
>
> [BTW, the only place I have found to add documentaiotn is in the
> "docs/formatnetwork.html.in" file.  If there are other files I should
> be updating, please enlighten me.]

If you can find a good place in the wiki to expand on what's in
formatnetwork, that would be good, but it's a separate process (since
the wiki isn't stored in git).

>
> 2. Add code to get the dnsmasq version and save that information. The
> added code described below will require a dnsmasq version greater than
> or equal to 2.64.  Documentation will be updated to state that dnsmasq
> >= 2.64 is required for DHCPv6.
>
> 3.  Implement support for DHCPv6.  Most of this is already done with
> the existing patch.  However, this refits the code to work with
> command-line parameters AND adds a check for dnsmasq >= 2.64.
> Naturally, tests and documentation will be updated.
>
> 4.  If dnsmasq >= 2.64, do not use radvd but instead use dnsmasq to
> support the RA service for both state-full and state-less IPV6.
>
> OPTIONS:
> ========
> a) If dnsmasq < 2.64, just ignore dhcp-range or dhcp-host definitions

No. It must log an UNSUPPORTED error and fail. This should be done at
the time the network is started/created though, not when it is defined,
since the dnsmasq version may change in the interim.

>
> OR
>
> b) issue error message and stop dnsmasq startup if dhcp-range or
> dhcp-host is specified.

Right.


> ========
> c) Currently, tests for valid dhcp specifications is only done when a
> network is started (all significant changes are to
> "network/bridge_driver.c".  This situation could continue.  Thus,
> virsh could be used to specify dhcp-range and dhcp-host but it would
> not work if the network was started.

Yes. Although I'm creating a small "dnsmasqCapabilities" API where you
will be able to get this information (btw, I don't think the version
should be stored in the network object, but instead be retrieved
directly from the API (we may want to cache the info somewhere globally
in situations such as a mass-restart of all networks).

>
> OR
>
> d) Move the dnsmasq version checks back into when the network is
> defined.  This would be a bit "trickier" to implement properly since
> the same code is used by multiple network types and not just that
> supported by "network/bridge_driver.c".  If this is the approach, I
> will need some guidance as to modifying "conf/network.c".

No. If something requires support in libvirt itself that is lacking
(e.g. different code needs to be compiled in, or required code isn't yet
written), you can fail during network define, but if it requires support
in an external binary that may or may not have the support, you need to
wait until you're actually going to start it, since that support may be
added or removed at any time.

>
> OK folks.  I need some input here.  I realize that all of you are very
> busy working your own interests but I need a little time from someone
> with "commit" authority to say "go ahead" or "get lost".

Again, I would say wait for a few days. Hopefully before we take off for
the holiday on Thursday I'll have a patch that adds dnsmasqCapabilities
functions and uses that to conditionally enable --bind-dynamic (as
required for the CVE mentioned above) and we can see how disruptive that
is to your current patches.