network: don't add "no-resolv" if we still need DNS servers from resolv.conf

[libvirt] [PATCH] network: don't add "no-resolv" if we still need DNS servers from resolv.conf

Posted by Laine Stump 7 years, 1 month ago

It was pointed out here:

  https://bugzilla.redhat.com/show_bug.cgi?id=1331796#c4

that we shouldn't be adding a "no-resolv" to the dnsmasq.conf file for
a network if there isn't any <forwarder> element that specifies an IP
address but no qualifying domain. If there is such an element, it will
handle all DNS requests that weren't otherwise handled by one of the
forwarder entries with a matching domain attribute. If not, then DNS
requests that don't match the domain of any <forwarder> would not be
resolved if we added no-resolv.

So, only add "no-resolv" when there is at least one <forwarder>
element that specifies an IP address but no qualifying domain.
---
 src/network/bridge_driver.c                                | 14 +++++++++++++-
 .../nat-network-dns-forwarder-no-resolv.conf               | 12 ++++++++++++
 .../nat-network-dns-forwarder-no-resolv.xml                | 11 +++++++++++
 tests/networkxml2confdata/nat-network-dns-forwarders.conf  |  2 +-
 tests/networkxml2conftest.c                                |  1 +
 .../nat-network-dns-forwarder-no-resolv.xml                | 11 +++++++++++
 .../nat-network-dns-forwarder-no-resolv.xml                | 11 +++++++++++
 tests/networkxml2xmltest.c                                 |  1 +
 8 files changed, 61 insertions(+), 2 deletions(-)
 create mode 100644 tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.conf
 create mode 100644 tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.xml
 create mode 100644 tests/networkxml2xmlin/nat-network-dns-forwarder-no-resolv.xml
 create mode 100644 tests/networkxml2xmlout/nat-network-dns-forwarder-no-resolv.xml

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c5ec282..32c5ab7 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1085,7 +1085,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
         virBufferAddLit(&configbuf, "port=0\n");
 
     if (wantDNS && network->def->dns.forwarders) {
-        virBufferAddLit(&configbuf, "no-resolv\n");
+        /* addNoResolv should be set to true if there are any entries
+         * that specify an IP address for requests, but no domain
+         * qualifier (implying that all requests otherwise "unclaimed"
+         * should be sent to that address). if it is still false when
+         * we've looked at all entries, it means we still need the
+         * host's resolv.conf for some cases.
+         */
+        bool addNoResolv = false;
+
         for (i = 0; i < network->def->dns.nfwds; i++) {
             virNetworkDNSForwarderPtr fwd = &network->def->dns.forwarders[i];
 
@@ -1099,11 +1107,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
                     goto cleanup;
                 virBufferAsprintf(&configbuf, "%s\n", addr);
                 VIR_FREE(addr);
+                if (!fwd->domain)
+                    addNoResolv = true;
             } else {
                 /* "don't forward requests for this domain" */
                 virBufferAddLit(&configbuf, "#\n");
             }
         }
+        if (addNoResolv)
+            virBufferAddLit(&configbuf, "no-resolv\n");
     }
 
     if (network->def->domain) {
diff --git a/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.conf b/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.conf
new file mode 100644
index 0000000..52d000a
--- /dev/null
+++ b/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.conf
@@ -0,0 +1,12 @@
+##WARNING:  THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE
+##OVERWRITTEN AND LOST.  Changes to this configuration should be made using:
+##    virsh net-edit default
+## or other application using the libvirt API.
+##
+## dnsmasq conf file created by libvirt
+strict-order
+server=/example.com/192.168.1.1
+except-interface=lo
+bind-dynamic
+interface=virbr0
+addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
diff --git a/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.xml b/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.xml
new file mode 100644
index 0000000..9661ce5
--- /dev/null
+++ b/tests/networkxml2confdata/nat-network-dns-forwarder-no-resolv.xml
@@ -0,0 +1,11 @@
+<network>
+  <name>default</name>
+  <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid>
+  <forward mode='nat'/>
+  <bridge name='virbr0' stp='on' delay='0'/>
+  <dns>
+    <forwarder domain='example.com' addr='192.168.1.1'/>
+  </dns>
+  <ip address='192.168.122.1' netmask='255.255.255.0'>
+  </ip>
+</network>
diff --git a/tests/networkxml2confdata/nat-network-dns-forwarders.conf b/tests/networkxml2confdata/nat-network-dns-forwarders.conf
index 0bd76bf..1b0c94c 100644
--- a/tests/networkxml2confdata/nat-network-dns-forwarders.conf
+++ b/tests/networkxml2confdata/nat-network-dns-forwarders.conf
@@ -5,11 +5,11 @@
 ##
 ## dnsmasq conf file created by libvirt
 strict-order
-no-resolv
 server=8.8.8.8
 server=8.8.4.4
 server=/example.com/192.168.1.1
 server=/www.example.com/#
+no-resolv
 except-interface=lo
 bind-dynamic
 interface=virbr0
diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c
index 9b61077..e2522fc 100644
--- a/tests/networkxml2conftest.c
+++ b/tests/networkxml2conftest.c
@@ -137,6 +137,7 @@ mymain(void)
     DO_TEST("nat-network-dns-hosts", full);
     DO_TEST("nat-network-dns-forward-plain", full);
     DO_TEST("nat-network-dns-forwarders", full);
+    DO_TEST("nat-network-dns-forwarder-no-resolv", full);
     DO_TEST("nat-network-dns-local-domain", full);
     DO_TEST("dhcp6-network", dhcpv6);
     DO_TEST("dhcp6-nat-network", dhcpv6);
diff --git a/tests/networkxml2xmlin/nat-network-dns-forwarder-no-resolv.xml b/tests/networkxml2xmlin/nat-network-dns-forwarder-no-resolv.xml
new file mode 100644
index 0000000..9661ce5
--- /dev/null
+++ b/tests/networkxml2xmlin/nat-network-dns-forwarder-no-resolv.xml
@@ -0,0 +1,11 @@
+<network>
+  <name>default</name>
+  <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid>
+  <forward mode='nat'/>
+  <bridge name='virbr0' stp='on' delay='0'/>
+  <dns>
+    <forwarder domain='example.com' addr='192.168.1.1'/>
+  </dns>
+  <ip address='192.168.122.1' netmask='255.255.255.0'>
+  </ip>
+</network>
diff --git a/tests/networkxml2xmlout/nat-network-dns-forwarder-no-resolv.xml b/tests/networkxml2xmlout/nat-network-dns-forwarder-no-resolv.xml
new file mode 100644
index 0000000..9661ce5
--- /dev/null
+++ b/tests/networkxml2xmlout/nat-network-dns-forwarder-no-resolv.xml
@@ -0,0 +1,11 @@
+<network>
+  <name>default</name>
+  <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid>
+  <forward mode='nat'/>
+  <bridge name='virbr0' stp='on' delay='0'/>
+  <dns>
+    <forwarder domain='example.com' addr='192.168.1.1'/>
+  </dns>
+  <ip address='192.168.122.1' netmask='255.255.255.0'>
+  </ip>
+</network>
diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c
index cfaf718..effd85a 100644
--- a/tests/networkxml2xmltest.c
+++ b/tests/networkxml2xmltest.c
@@ -141,6 +141,7 @@ mymain(void)
     DO_TEST("nat-network-dns-hosts");
     DO_TEST("nat-network-dns-forward-plain");
     DO_TEST("nat-network-dns-forwarders");
+    DO_TEST("nat-network-dns-forwarder-no-resolv");
     DO_TEST("nat-network-forward-nat-address");
     DO_TEST("nat-network-forward-nat-no-address");
     DO_TEST("8021Qbh-net");
-- 
2.9.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] network: don't add "no-resolv" if we still need DNS servers from resolv.conf

Posted by Jiri Denemark 7 years, 1 month ago

On Fri, Mar 17, 2017 at 12:33:14 -0400, Laine Stump wrote:
> It was pointed out here:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1331796#c4
> 
> that we shouldn't be adding a "no-resolv" to the dnsmasq.conf file for
> a network if there isn't any <forwarder> element that specifies an IP
> address but no qualifying domain. If there is such an element, it will
> handle all DNS requests that weren't otherwise handled by one of the
> forwarder entries with a matching domain attribute. If not, then DNS
> requests that don't match the domain of any <forwarder> would not be
> resolved if we added no-resolv.
> 
> So, only add "no-resolv" when there is at least one <forwarder>
> element that specifies an IP address but no qualifying domain.
...
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index c5ec282..32c5ab7 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1085,7 +1085,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>          virBufferAddLit(&configbuf, "port=0\n");
>  
>      if (wantDNS && network->def->dns.forwarders) {
> -        virBufferAddLit(&configbuf, "no-resolv\n");
> +        /* addNoResolv should be set to true if there are any entries
> +         * that specify an IP address for requests, but no domain
> +         * qualifier (implying that all requests otherwise "unclaimed"
> +         * should be sent to that address). if it is still false when
> +         * we've looked at all entries, it means we still need the
> +         * host's resolv.conf for some cases.
> +         */
> +        bool addNoResolv = false;
> +
>          for (i = 0; i < network->def->dns.nfwds; i++) {
>              virNetworkDNSForwarderPtr fwd = &network->def->dns.forwarders[i];
>  
> @@ -1099,11 +1107,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>                      goto cleanup;
>                  virBufferAsprintf(&configbuf, "%s\n", addr);
>                  VIR_FREE(addr);
> +                if (!fwd->domain)
> +                    addNoResolv = true;
>              } else {
>                  /* "don't forward requests for this domain" */
>                  virBufferAddLit(&configbuf, "#\n");
>              }
>          }
> +        if (addNoResolv)
> +            virBufferAddLit(&configbuf, "no-resolv\n");
>      }
>  
>      if (network->def->domain) {

So what if the network is isolated and supposed to only resolve names
according to its database. Such network does not have any <forwarder/>
element and yet no-resolve should be added in the configuration.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] network: don't add "no-resolv" if we still need DNS servers from resolv.conf

Posted by Laine Stump 7 years, 1 month ago

On 03/17/2017 12:58 PM, Jiri Denemark wrote:
> On Fri, Mar 17, 2017 at 12:33:14 -0400, Laine Stump wrote:
>> It was pointed out here:
>>
>>   https://bugzilla.redhat.com/show_bug.cgi?id=1331796#c4
>>
>> that we shouldn't be adding a "no-resolv" to the dnsmasq.conf file for
>> a network if there isn't any <forwarder> element that specifies an IP
>> address but no qualifying domain. If there is such an element, it will
>> handle all DNS requests that weren't otherwise handled by one of the
>> forwarder entries with a matching domain attribute. If not, then DNS
>> requests that don't match the domain of any <forwarder> would not be
>> resolved if we added no-resolv.
>>
>> So, only add "no-resolv" when there is at least one <forwarder>
>> element that specifies an IP address but no qualifying domain.
> ...
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index c5ec282..32c5ab7 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -1085,7 +1085,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>>          virBufferAddLit(&configbuf, "port=0\n");
>>  
>>      if (wantDNS && network->def->dns.forwarders) {
>> -        virBufferAddLit(&configbuf, "no-resolv\n");
>> +        /* addNoResolv should be set to true if there are any entries
>> +         * that specify an IP address for requests, but no domain
>> +         * qualifier (implying that all requests otherwise "unclaimed"
>> +         * should be sent to that address). if it is still false when
>> +         * we've looked at all entries, it means we still need the
>> +         * host's resolv.conf for some cases.
>> +         */
>> +        bool addNoResolv = false;
>> +
>>          for (i = 0; i < network->def->dns.nfwds; i++) {
>>              virNetworkDNSForwarderPtr fwd = &network->def->dns.forwarders[i];
>>  
>> @@ -1099,11 +1107,15 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>>                      goto cleanup;
>>                  virBufferAsprintf(&configbuf, "%s\n", addr);
>>                  VIR_FREE(addr);
>> +                if (!fwd->domain)
>> +                    addNoResolv = true;
>>              } else {
>>                  /* "don't forward requests for this domain" */
>>                  virBufferAddLit(&configbuf, "#\n");
>>              }
>>          }
>> +        if (addNoResolv)
>> +            virBufferAddLit(&configbuf, "no-resolv\n");
>>      }
>>  
>>      if (network->def->domain) {
> 
> So what if the network is isolated and supposed to only resolve names
> according to its database. Such network does not have any <forwarder/>
> element and yet no-resolve should be added in the configuration.

You forced me to remember that I had fixed exactly that hole a *long
time* ago (far before <forwarder> was added). I looked it up and found
commit 513122ae:

  https://bugzilla.redhat.com/show_bug.cgi?id=723862

which adds no-resolv if the network is isolated. I was momentarily
afraid that the no-resolv added in that patch had been "messed with" at
some later time, causing a regression in my fix, but found that it's
still there (look around line 1216).

So in the case of an isolated network, we still add no-resolv, no matter
whether we've added it due to <forwarders> or not.

But before we give up on this train of thought, is there maybe *some
other* situation I haven't considered?

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] network: don't add "no-resolv" if we still need DNS servers from resolv.conf

Posted by Jiri Denemark 7 years ago

On Fri, Mar 17, 2017 at 13:13:11 -0400, Laine Stump wrote:
> On 03/17/2017 12:58 PM, Jiri Denemark wrote:
> > On Fri, Mar 17, 2017 at 12:33:14 -0400, Laine Stump wrote:
> >> It was pointed out here:
> >>
> >>   https://bugzilla.redhat.com/show_bug.cgi?id=1331796#c4
> >>
> >> that we shouldn't be adding a "no-resolv" to the dnsmasq.conf file for
> >> a network if there isn't any <forwarder> element that specifies an IP
> >> address but no qualifying domain. If there is such an element, it will
> >> handle all DNS requests that weren't otherwise handled by one of the
> >> forwarder entries with a matching domain attribute. If not, then DNS
> >> requests that don't match the domain of any <forwarder> would not be
> >> resolved if we added no-resolv.
> >>
> >> So, only add "no-resolv" when there is at least one <forwarder>
> >> element that specifies an IP address but no qualifying domain.
> > ...
> > So what if the network is isolated and supposed to only resolve names
> > according to its database. Such network does not have any <forwarder/>
> > element and yet no-resolve should be added in the configuration.
> 
> You forced me to remember that I had fixed exactly that hole a *long
> time* ago (far before <forwarder> was added). I looked it up and found
> commit 513122ae:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=723862
> 
> which adds no-resolv if the network is isolated. I was momentarily
> afraid that the no-resolv added in that patch had been "messed with" at
> some later time, causing a regression in my fix, but found that it's
> still there (look around line 1216).
> 
> So in the case of an isolated network, we still add no-resolv, no matter
> whether we've added it due to <forwarders> or not.

And I see we even have a test case for isolated network and no-resolv is
still there.

ACK

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list