[libvirt] [PATCH] network: only prevent forwarding of DNS requests for unqualified names

[Laine Stump]

On 12/19/2013 06:50 PM, Daniel P. Berrange wrote:
> On Mon, Dec 16, 2013 at 04:41:31PM +0200, Laine Stump wrote:
>> On 12/10/2013 04:45 PM, Daniel P. Berrange wrote:
>>> So considering your example XML config above we're debating the
>>> behaviour of the following 5 possible DNS requests
>>>
>>>  - myguest
>>>  - myguest.example.com
>>>  - notmyguest
>>>  - notmyguest.example.com
>>>  - google.com
>>>
>>> Originally
>>>
>>>  - myguest -> dnsmasq
>>>  - myguest.example.com -> dnsmasq
>>>  - notmyguest -> forwarded
>>>  - notmyguest.example.com -> forwarded
>>>  - google.com -> forwarded
>>>
>>> With the current GIT
>>>
>>>  - myguest -> dnsmasq
>>>  - myguest.example.com -> dnsmasq
>>>  - notmyguest -> error
>>>  - notmyguest.example.com -> error
>>>  - google.com -> forwarded
>>>
>>> With your proposal
>>>
>>>  - myguest -> dnsmasq
>>>  - myguest.example.com -> dnsmasq
>>>  - notmyguest -> error
>>>  - notmyguest.example.com -> fowarded
>>>  - google.com -> forwarded
>> Nicely organized and diagrammed!
>>
>> This is really a case of accepting a patch without performing adequate
>> due diligence on the ramifications of the change in behavior. The reason
>> behind adding the patch seemed valid, but it had a wider effect than
>> intended.
> Indeed.
>
>>> IMHO I tend to think that the original behaviour should not have been
>>> changed and is the right default. If we desired either of the other
>>> behaviours we should have a config option for them to force returning
>>> of errors instead of forwarding. A simple boolean wouldn't suffice
>>> since there are 3 possible valid behaviours here - so we'd need an
>>> enum attribute
>> The first time I encountered this particular blowback of the original
>> patch (it had also included --filterwin2k, which itself caused problems
>> and was reverted), my response was to add the
>>
>>   <dns forwardPlainNames='yes'/>
>>
>> attribute - that restores (tries to anyway) the original behavior, but
>> not by default. (My thinking was that the intent of the original patch
>> was desirable, since it had been ACKed and pushed (and besides,
>> forwarding of non-qualified names really is frowned on, at least by root
>> dns servers). Too bad we didn't have this discussion back then :-/
>> That's the problem of having such a high volume of mail on the list -
>> there's never enough time for everyone to read it all, so some
>> discussions just never happen.
>>
>> So is your suggestion that we add to this patch another patch which
>> changes the default for "forwardPlainNames" to "no"?
> Oh, I didn't realize we had that attribute already. Don't we need it
> to be a tri-state instead of just yes/no ?
>
> Going back to be list at the to. IIUC, the forwardPlainNames=no
> toggles between
>
>>> Originally
>>>  - notmyguest -> forwarded
>>>  - notmyguest.example.com -> forwarded
>>>
>>> With the current GIT
>>>
>>>  - notmyguest -> error
>>>  - notmyguest.example.com -> error

Actually, with my patch it will toggle between

 Originally
 - notmyguest -> forwarded
 - notmyguest.example.com -> forwarded

With the current GIT

 - notmyguest -> error
 - notmyguest.example.com -> forward

With the default being the latter.

>
> But doesn't give us a way to handle this new use case:
>
>>> With your proposal
>>>
>>>  - notmyguest -> error
>>>  - notmyguest.example.com -> fowarded
>
> Or am I misunderstanding ?

Yes. That case is handled. The case that isn't possible (either
originally or after my patch) is this one:

 - notmyguest -> error
 - notmyguest.example.com -> error


i.e. now the forwardPlainNames really will only control whether or not
unresolved names w/o a domain will be forwarded.

I think this other case is better handled by some sort of extension as
hinted at in this bit of my earlier mail:

>> (I can also see how this could/should functionally fit together with a
>> list of domains/dns servers, which we *also* "kind of" support via the
>> <forwarder> element (which unfortunately only does things halfway - it
>> allows specifying the IP address of a dns server, but not what domain it
>> should be used for; strange, seeing that the dnsmasq option it is
>> encapsulating does exactly that). I'm thinking there should be another
>> patch which extends each <forwarder> to have an optional "domain"
>> attribute to limit which domains a particular dns server is used for)

with possibly the ability to define a NULL forwarder in some way - any
unresolved requests for that particular domain will return an error
rather than being forwarded.

I really don't want to make all of that more convoluted than it already
is though, so don't want to rush to a decision and make yet another
patch we'll regret later. However, there is at least one organization
that is currently suffering due to the change in behavior, so I'd like
to make a simple change to restore that as soon as I can.

What would you say to the patch that started this thread, with the
addition that "forwardPlainNames" defaults to "yes"? This way, the
original behavior is once again default (basically unresolved requests
are always forwarded to the upstream DNS), but it's possible for someone
to return error on on unresolved plain names (which was the intent of
the patch that started this problem).