[libvirt] Plan A or Plan B?

[Laine Stump]

On 11/19/2012 01:50 PM, Gene Czarcinski wrote:
> 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.

Well, it doesn't need to completely understand every character of the
output. The way that it's done in qemu is just to do strstr() of the
entire output looking for a particular string, and we only have flags
for / check those things that we actually need (so in my initial
implementation there is just a single flag).

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

That may have been useful if it was already in dnsmasq from "the
beginning", but at this point we have to work with what's out there.
We've actually had an ongoing discussion with qemu about exactly this
topic for several years, and they've recently added a sane say to query
the capabilities of a qemu binary; we still have to continue supporting
the "parse the help output" method though, at least for any capability
that was introduced prior to adding the capabilities query to qemu.


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

Yes, technically it can be done. I just want to make sure that it
saitisfies everyone's "don't open a new hole by default"

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

It could change during a run of libvirtd. I'm caching the mtime of the
binary, and reloading the info if it changes.