[libvirt] Plan A or Plan B?

[Gene Czarcinski]

SInce I wrote this, I have "Plan B" about a half to two-thirds complete 
as far as the coding goes.  I have this "feeling" that postponing the 
conf-file changes might be better.

On 11/19/2012 01:06 PM, Laine Stump wrote:
> 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.
Understood and I expected that this might be a big part of it.
>
> 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).
Since you first brought this up, I have dropped the conf-dir= because 
what you said made sense.
>
>   
> 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
Yes, this does look like a much better approach.  I am not sure how to 
do it yet but this makes a lot more sense that the conf-dir or a 
secondary conf-file.
>
> 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).
I took a look at what comes out of --help and there is going to be 
nothing pretty about extracting info from it.

I wonder if Simon Kelley would be receptive to adding a "special" 
dnsmasq interface which would provide more information and in a 
structured manner to make it easy to be processed by other software.
>
> 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.
Done.  But, if you want to "reverse the order," it appears that it will 
not be that difficult.
>
>> 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?
I have looked over the code as well as done some testing (the code is 
all in network/bridge_driver.c).  Unless there really is an IPv6 address 
specified, disable_ipv6=1.
>
>> 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.
I have been assuming this but wanted to mention the alternative.
>
>
>> ========
>> 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).
The network object was "handy." ;)  This version stuff should be done 
once when libvirtd starts up.
>
>> 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.
I prefer to do the check when the network is started because there just 
might be something else using the code in conf/network.c. However, I did 
need to add some code the conf/network.c just to make things work.  For 
example, with dhcp-host definitions, the only "reasonable" identifier 
IPv6 is the system name since the mac-address is not defined in IPv6 ... 
and it does work.
>
>> 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.
>
>
Thanks, Gene