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

[Laine Stump]

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?