[libvirt] [PATCH] network: only prevent forwarding of DNS requests for unqualified names
In commit f386825 we began adding the option "--local=/$mydomain/" to all dnsmasq commandlines (later changed to "local=/$mydomain/" when we moved the options from the commandline to a conf file) with the stated reason of preventing forwarding of DNS queries for names that weren't fully qualified domain names ("FQDN", i.e. a name that included some "."s and a domain name). The original patch on the list, and discussion about it, is here: https://www.redhat.com/archives/libvir-list/2012-August/msg01594.html When a domain name isn't specified (no <domain> element in the network definition), the corresponding addition of "local=//" will prevent forwarding of domain-less requests to the virtualization host's DNS resolver, but if a domain *is* specified, the addition of "local=/domain/" will prevent forwarding of any requests for names within that domain that aren't resolvable by libvirt's dnsmasq itself. An example of the problems this causes: let's say a network is defined with: <domain name='example.com'/> <dhcp> .. <host mac='52:54:00:11:22:33' ip='1.2.3.4' name='myguest'/> </dhcp> This results in "local=/example.com/" being added to the dnsmasq options. If a guest requests "myguest" or "myguest.example.com", that will be resolved by dnsmasq. If the guest asks for "www.example.com", dnsmasq will not know the answer, but instead of forwarding it to the host, it will return NOT FOUND to the guest. In most cases that isn't the behavior an admin is looking for. Really we should have been just including the option "--local=//" in all cases, so that (unresolvable by dnsmasq) requests for names without a domain would be treated as "local to dnsmasq" and not forwarded, but all other requests would be forwarded. That's what this patch does. --- I'm debating whether there is any value at all to maintaining the previous behavior of "don't forward unresolved requests for hosts in the network's defined domain", or if it should just be considered purely a bug. If so, I think it shouldn't be the default behavior, and should be controlled by a new attribute to the <domain> element, something like this: <domain name='example.com' forwardUnresolved='no'/> (this would default to yes). Any opinions on 1) whether or not this is needed, and 2) if so, any better name for the option? (it would be nice if it could default to 'no' or 'local-only' (which was == 0) or something else that didn't require a non-0 default or a strange double-negative name). src/network/bridge_driver.c | 12 +++++------- tests/networkxml2confdata/dhcp6-network.conf | 2 +- tests/networkxml2confdata/nat-network-dns-hosts.conf | 2 +- tests/networkxml2confdata/netboot-network.conf | 2 +- tests/networkxml2confdata/netboot-proxy-network.conf | 2 +- 5 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 80c5acb..b1382e4 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -723,14 +723,12 @@ networkDnsmasqConfContents(virNetworkObjPtr network, network->def->domain); } - if (network->def->domain || !network->def->dns.forwardPlainNames) { - /* need to specify local even if no domain specified, unless - * the config says we should forward "plain" names (i.e. not - * fully qualified, no '.' characters) + if (!network->def->dns.forwardPlainNames) { + /* need to specify local=// whether or not a domain is + * specified, unless the config says we should forward "plain" + * names (i.e. not fully qualified, no '.' characters) */ - virBufferAsprintf(&configbuf, - "local=/%s/\n", - network->def->domain ? network->def->domain : ""); + virBufferAsprintf(&configbuf, "local=//\n"); } if (pidfile) diff --git a/tests/networkxml2confdata/dhcp6-network.conf b/tests/networkxml2confdata/dhcp6-network.conf index 5fde07f..24ac7fa 100644 --- a/tests/networkxml2confdata/dhcp6-network.conf +++ b/tests/networkxml2confdata/dhcp6-network.conf @@ -8,7 +8,7 @@ strict-order domain-needed domain=mynet expand-hosts -local=/mynet/ +local=// except-interface=lo bind-dynamic interface=virbr0 diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.conf b/tests/networkxml2confdata/nat-network-dns-hosts.conf index 2577882..bb1d5f4 100644 --- a/tests/networkxml2confdata/nat-network-dns-hosts.conf +++ b/tests/networkxml2confdata/nat-network-dns-hosts.conf @@ -8,7 +8,7 @@ strict-order domain-needed domain=example.com expand-hosts -local=/example.com/ +local=// except-interface=lo bind-dynamic interface=virbr0 diff --git a/tests/networkxml2confdata/netboot-network.conf b/tests/networkxml2confdata/netboot-network.conf index b6f3c23..37fef40 100644 --- a/tests/networkxml2confdata/netboot-network.conf +++ b/tests/networkxml2confdata/netboot-network.conf @@ -8,7 +8,7 @@ strict-order domain-needed domain=example.com expand-hosts -local=/example.com/ +local=// except-interface=lo bind-interfaces listen-address=192.168.122.1 diff --git a/tests/networkxml2confdata/netboot-proxy-network.conf b/tests/networkxml2confdata/netboot-proxy-network.conf index 1e969fa..ae34aa0 100644 --- a/tests/networkxml2confdata/netboot-proxy-network.conf +++ b/tests/networkxml2confdata/netboot-proxy-network.conf @@ -8,7 +8,7 @@ strict-order domain-needed domain=example.com expand-hosts -local=/example.com/ +local=// except-interface=lo bind-interfaces listen-address=192.168.122.1 -- 1.8.3.1
On Fri, Dec 06, 2013 at 04:19:01PM +0200, Laine Stump wrote:
In commit f386825 we began adding the option "--local=/$mydomain/" to all dnsmasq commandlines (later changed to "local=/$mydomain/" when we moved the options from the commandline to a conf file) with the stated reason of preventing forwarding of DNS queries for names that weren't fully qualified domain names ("FQDN", i.e. a name that included some "."s and a domain name).
The original patch on the list, and discussion about it, is here:
https://www.redhat.com/archives/libvir-list/2012-August/msg01594.html
When a domain name isn't specified (no <domain> element in the network definition), the corresponding addition of "local=//" will prevent forwarding of domain-less requests to the virtualization host's DNS resolver, but if a domain *is* specified, the addition of "local=/domain/" will prevent forwarding of any requests for names within that domain that aren't resolvable by libvirt's dnsmasq itself.
An example of the problems this causes: let's say a network is defined with:
<domain name='example.com'/> <dhcp> .. <host mac='52:54:00:11:22:33' ip='1.2.3.4' name='myguest'/> </dhcp>
This results in "local=/example.com/" being added to the dnsmasq options.
If a guest requests "myguest" or "myguest.example.com", that will be resolved by dnsmasq. If the guest asks for "www.example.com", dnsmasq will not know the answer, but instead of forwarding it to the host, it will return NOT FOUND to the guest. In most cases that isn't the behavior an admin is looking for.
Really we should have been just including the option "--local=//" in all cases, so that (unresolvable by dnsmasq) requests for names without a domain would be treated as "local to dnsmasq" and not forwarded, but all other requests would be forwarded. That's what this patch does. ---
I'm debating whether there is any value at all to maintaining the previous behavior of "don't forward unresolved requests for hosts in the network's defined domain", or if it should just be considered purely a bug. If so, I think it shouldn't be the default behavior, and should be controlled by a new attribute to the <domain> element, something like this:
<domain name='example.com' forwardUnresolved='no'/>
(this would default to yes). Any opinions on 1) whether or not this is needed, and 2) if so, any better name for the option? (it would be nice if it could default to 'no' or 'local-only' (which was == 0) or something else that didn't require a non-0 default or a strange double-negative name).
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 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 Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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.
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"? (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)
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
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 ?
(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)
Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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).
participants (2)
-
Daniel P. Berrange -
Laine Stump