Re: [libvirt] [PATCH] network: Let domains be restricted to local DNS

On 12/01/2014 01:50 PM, Josh Stone wrote: > On 12/01/2014 05:18 AM, Martin Kletzander wrote: >> On Mon, Dec 01, 2014 at 07:49:32AM -0500, Laine Stump wrote: >>> On 11/30/2014 04:06 PM, Martin Kletzander wrote: >>>> On Thu, Nov 20, 2014 at 06:46:41PM -0800, Josh Stone wrote: >>>>> This adds a new "localOnly" attribute on the domain element of the >>>>> network xml. With this set to "yes", DNS requests under that domain >>>>> will only be resolved by libvirt's dnsmasq, never forwarded upstream. >>>>> >>>>> This was how it worked before commit f69a6b987d616, and I found that >>>>> functionality useful. For example, I have my host's NetworkManager >>>>> dnsmasq configured to forward that domain to libvirt's dnsmasq, so I can >>>>> easily resolve guest names from outside. But if libvirt's dnsmasq >>>>> doesn't know a name and forwards it to the host, I'd get an endless >>>>> forwarding loop. Now I can set localOnly="yes" to prevent the loop. >>>>> >>>> I've found 2 things in this patch I'm not sure about: >>>> >>>> 1) According to the documentation about --local= parameter, it says: >>>> "Setting this flag does not suppress reading of /etc/resolv.conf, >>>> use -R to do that." Shouldn't "no-resolv" be added as the option >>>> you want in the config file? >>> No, "-R" (aka "no-resolv") tells dnsmasq to not use the servers listed >>> in /etc/resolv.conf for *anything*. In this case, all that is wanted is >>> to resolve requests _within the configured domain_ only locally, and not >>> forward those requests upstream. Requests for any other domain _should_ >>> be forwarded to whatever server is listed in /etc/resolv.conf. Based on >>> a discussion with Josh, I'm certain that this is the behavior he wants >>> (and it makes sense to have it available). >>> >> OK, I just misunderstood this part, sorry for that. > Right, I want to keep its own domain local, but go out otherwise. > >>>> 2) If your answer to the previous question is "NO", which might very >>>> well be the case, for example if you don't want to forward that >>>> one particular domain only (and I misunderstood the commit >>>> message), I think you should just use the ",local" subparameter of >>>> the domain parameter (domain=example.com,local). >>> Interesting idea - this requires specifying the IP/prefix of the network >>> in the option along with domain (which I suppose isn't a problem), and >>> has the same effect as adding local=/domain.com/ as well as the reverse >>> resolution, which is a nice plus. However, the prefix for the network >>> must be 8, 16, or 24 for this to work (because of the way that >>> *.in-addr.arpa records are defined), so we can't do this all the time. >>> The question is then - do we write in a special case to do it this way >>> when the prefix is one of the right lengths, or just not do it at all? >>> Having it "magically happen" for some configs and not others could lead >>> to confusion, but having reverse resolution when possible would be very >>> useful (for example, some sshd setups have a long delay if the >>> reverse-resolve of the client's IP doesn't return something other than >>> "not found"; a pointless piece of security theater, but still fairly >>> common). >>> >> Looking at the documentation again, I understand it a bit more yet >> again. Let's assume people use normal domain names. Libvirt can use >> the IP address and netmask from the 'ip' element (if known) and >> construct the domain parameter for the config file: >> >> domain=virt,192.168.13.0/24,local >> >> If there's no IP address known, then it could be constructed as >> follows: >> >> domain=virt >> local=/virt/ >> >> What do you say? Yeah, that's what I was suggesting as a possibility above (but wondering if it was worth the ambiguity), but according to Josh's testing, there is at least one version of dnsmasq that implements "domain=blah,x.x.x.x/y,local" incorrectly, so I guess we can't use it, at least not by itself. >> >> >> Currently this is possible to achieve with the following in the XML: >> >> <domain name='virt,192.168.13.0/24,local'/> >> >> But that's just plain ugly ;-) > Interesting. If this works, and libvirt promises not to take it away > from me later, then it would be enough to satisfy my itch. :) I'm actually glad that it didn't work, as even the presence of this email could create a precedent that we don't want to exist. As a rule, libvirt does not like multiple attributes in a single string; if multiple attributes are needed, we try to represent them separately in the XML. In other words, don't count on the above XML working. As a matter of fact, assume that it will be eliminated.

> However, it doesn't actually seem to do the right thing. The option > does survive from XML to the dnsmasq conf, but it doesn't seem to > actually behave as local. When I manually tweak the conf and restart > libvirt's dnsmasq myself, it seems that having separate "domain=foo" and > "local=/foo/" terminates properly, but having a oneliner > "domain=foo,192.168.122.0/24,local" is still forwarding to the host. > Which then forwards back to libvirt, and I get my loop. :( > > This is dnsmasq-2.72-3.fc21.x86_64. Maybe dnsmasq only only treats it > local when coming from that subnet? But it's documented as equivalent > to local=/foo/, so maybe dnsmasq has a bug here. Did you try it? From the other mail it sounds like you discovered a bug in dnsmasq. But libvirt will anyway want to work even with buggy versions of dnsmasq, so perhaps localOnly should continue to add local=/foo/, but also add ",${ip}/$prefix,local" to the domain option *if* the prefix is 8, 16, or 24. I would say this can/should be done in a separate patch (which would update the test cases and the docs accordingly).

Back to the code of your patch - the only issue I see with it is what John caught (missing the addition to virNetworkDefFormatBuf(), and using virTriStateBool(To|From)String rather than comparing to "yes"). Other than that, it would have been nice if this config could be in the <dns> element, but <domain> is at the top level of network because it is used by both dns and dhcp, and this new attribute is most closely associated with the domain name, so that's not really possible.