Re: [PATCH] network: add an option to make dns public

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/11/2015 01:37 PM, Laine Stump wrote:
> On 06/10/2015 03:56 PM, John Ferlan wrote:
>>
>> On 06/01/2015 07:54 AM, Cédric Bosdonnat wrote:
>>> In some use cases we don't want the virtual network's DNS to only
>>> listen to the vnet interface. Adding a publiclyAccessible attribute
> 
> :-) Really, that name was only intended as a placeholder! I was hoping
> you (or someone else) would be able to find something shorter/simpler.
> Lacking that, I guess this is a reasonable name though.
> 

haha - I was thinking that publiclyAccessible was long (and challenging
for my fingers to type), but figured forwardPlainNames was a similarly
long attribute name, so I just left it as is. Certainly far better than
some TLA or FLA (three/four letter acronym)

>>> to the dns element in the configuration allows the DNS to listen to
>>> all interfaces.
>>>
>>> It simply disables the bind-dynamic option of dnsmasq for the network.
>>> ---
>>>  docs/formatnetwork.html.in                           | 11 +++++++++++
>>>  docs/schemas/network.rng                             | 15 ++++++++++-----
>>>  src/conf/network_conf.c                              |  6 ++++++
>>>  src/conf/network_conf.h                              |  1 +
>>>  src/network/bridge_driver.c                          |  4 +++-
>>>  tests/networkxml2confdata/nat-network-dns-hosts.conf |  1 -
>>>  tests/networkxml2confdata/nat-network-dns-hosts.xml  |  2 +-
>>>  7 files changed, 32 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
>>> index 6abed8f..8e43658 100644
>>> --- a/docs/formatnetwork.html.in
>>> +++ b/docs/formatnetwork.html.in
>>> @@ -851,6 +851,17 @@
>>>            DNS server.
>>>          </p>
>>>  
>>> +        <p>
>>> +          The dns element
>>> +          can have an optional <code>publiclyAccessible</code>
>>> +          attribute <span class="since">Since 1.2.17</span>.
>>> +          If <code>publiclyAccessible</code> is "yes", then the DNS server
>>> +          will handle requests for all interfaces.
>>> +          If <code>publiclyAccessible</code> is not set or "no", the DNS
>>> +          server will only handle requests for the interface of the virtual
>>> +          network.
>>> +        </p>
>>> +
>>>          Currently supported sub-elements of <code>&lt;dns&gt;</code> are:
>>>          <dl>
>>>            <dt><code>forwarder</code></dt>
>>> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
>>> index 4edb6eb..f989625 100644
>>> --- a/docs/schemas/network.rng
>>> +++ b/docs/schemas/network.rng
>>> @@ -244,12 +244,17 @@
>>>               and other features in the <dns> element -->
>>>          <optional>
>>>            <element name="dns">
>>> -            <optional>
>>> -              <attribute name="forwardPlainNames">
>>> -                <ref name="virYesNo"/>
>>> -              </attribute>
>>> -            </optional>
>>>              <interleave>
>>> +              <optional>
>>> +                <attribute name="forwardPlainNames">
>>> +                  <ref name="virYesNo"/>
>>> +                </attribute>
>>> +              </optional>
>>> +              <optional>
>>> +                <attribute name="publiclyAccessible">
>>> +                  <ref name="virYesNo"/>
>>> +                </attribute>
>>> +              </optional>
>> Moving the attributes inside the <interleave> had me looking through
>> other .rng's... I'm no expert, but had thought they really only mattered
>> for <element>'s
> 
> I'm not an expert either, but you are correct :-)
> 
> 
>>>                <zeroOrMore>
>>>                  <element name="forwarder">
>>>                    <attribute name="addr"><ref name="ipAddr"/></attribute>
>>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>>> index f4a9df0..99bac6d 100644
>>> --- a/src/conf/network_conf.c
>>> +++ b/src/conf/network_conf.c
>>> @@ -1309,9 +1309,14 @@ virNetworkDNSDefParseXML(const char *networkName,
>>>      size_t i;
>>>      int ret = -1;
>>>      xmlNodePtr save = ctxt->node;
>>> +    char *publiclyAccessible = NULL;
>>>  
>>>      ctxt->node = node;
>>>  
>>> +    publiclyAccessible = virXPathString("string(./@publiclyAccessible)", ctxt);
>>> +    if (publiclyAccessible)
>>> +        def->publiclyAccessible = virTristateBoolTypeFromString(publiclyAccessible);
>>> +
>>>      forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt);
>>>      if (forwardPlainNames) {
>>>          def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames);
>>> @@ -1410,6 +1415,7 @@ virNetworkDNSDefParseXML(const char *networkName,
>>>  
>>>      ret = 0;
>>>   cleanup:
>>> +    VIR_FREE(publiclyAccessible);
>>>      VIR_FREE(forwardPlainNames);
>>>      VIR_FREE(fwdNodes);
>>>      VIR_FREE(hostNodes);
>>> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
>>> index f69d999..f555b6b 100644
>>> --- a/src/conf/network_conf.h
>>> +++ b/src/conf/network_conf.h
>>> @@ -136,6 +136,7 @@ struct _virNetworkDNSDef {
>>>      virNetworkDNSSrvDefPtr srvs;
>>>      size_t nfwds;
>>>      char **forwarders;
>>> +    int publiclyAccessible; /* enum virTristateBool */
>>>  };
>>>  
>>>  typedef struct _virNetworkIpDef virNetworkIpDef;
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index d195085..c39b1a5 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -996,8 +996,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
>>>           * other than one of the virtual guests connected directly to
>>>           * this network). This was added in response to CVE 2012-3411.
>>>           */
>>> +        if (network->def->dns.publiclyAccessible != VIR_TRISTATE_BOOL_YES)
>>> +            virBufferAddLit(&configbuf,
>>> +                              "bind-dynamic\n");
>>>          virBufferAsprintf(&configbuf,
>>> -                          "bind-dynamic\n"
>>>                            "interface=%s\n",
>>>                            network->def->bridge);
>>>      } else {
>>> diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.conf b/tests/networkxml2confdata/nat-network-dns-hosts.conf
>>> index 021316f..759a9e9 100644
>>> --- a/tests/networkxml2confdata/nat-network-dns-hosts.conf
>>> +++ b/tests/networkxml2confdata/nat-network-dns-hosts.conf
>>> @@ -10,6 +10,5 @@ expand-hosts
>>>  domain-needed
>>>  local=//
>>>  except-interface=lo
>>> -bind-dynamic
>>>  interface=virbr0
>>>  addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts
>>> diff --git a/tests/networkxml2confdata/nat-network-dns-hosts.xml b/tests/networkxml2confdata/nat-network-dns-hosts.xml
>>> index 9add456..969dfa5 100644
>>> --- a/tests/networkxml2confdata/nat-network-dns-hosts.xml
>>> +++ b/tests/networkxml2confdata/nat-network-dns-hosts.xml
>>> @@ -4,7 +4,7 @@
>>>    <forward dev='eth0' mode='nat'/>
>>>    <bridge name='virbr0' stp='on' delay='0'/>
>>>    <domain name="example.com"/>
>>> -  <dns forwardPlainNames='no'>
>>> +  <dns forwardPlainNames='no' publiclyAccessible='yes'>
>>>      <host ip='192.168.122.1'>
>>>        <hostname>host</hostname>
>>>        <hostname>gateway</hostname>
>>>
>> Rather than change an existing test, a new test or two should be
>> created... One that specifically states 'yes' and possibly one that has
>> 'no' keeping the existing one with nothing provided to be sure that
>> works as well.
>>
>> I don't mind doing that for you, but also I figure by bumping this
>> perhaps Laine will take a look too since he usually responds to most of
>> the network related patches anyway... It seems fine to me though.
> 
> This is better than the previously proposed "bindDynamic" option (or
> whatever it was called; I just remember it was something dnsmasq-specific).

Hmmm... I do recall reading something about bindDynamic, but didn't make
the correlation between this and that. In the future when patches are
submitted like this perhaps providing a link to the upstream discussion
about a previous name would be beneficial...

John
> 
> However, the patch doesn't put "bind-interfaces" in place of
> "bind-dynamic", and unless I'm remembering incorrectly, removing
> bind-dynamic without adding "bind-interfaces" will cause multiple
> dnsmasq instances to conflict with each other - when neither option is
> used, dnsmasq will create a single socket listening on the wildcard
> address, then discard packets not intended for the IP address/interface;
> if any application listens on a given port using the wildcard address,
> no other application will be able to listen on that port on *any*
> interface/address. Here's an email that describes the differences:
> 
>  
> http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006525.html
> 
> Beyond that, I don't think that the name/description of this new option
> ("publiclyAccessible") really fits the original problem that I believe
> prompted the creation of the patch. As I understand it, the original
> problem was that LXC guests connected to a libvirt network were unable
> to get their IP address via dhcp (and probably weren't able to use
> libvirt's DNS server) because the packets were identified as arriving on
> the host side of the guest's veth interface rather than on the libvirt
> network's bridge device (the previous patch for this said something
> about vlans - was it only a problem in the case of vlans?).
> 
> I don't have a problem pushing this patch (after modifying it to use
> bind-interfaces when bind-dynamic isn't used, adding some new unit tests
> as John described, and copious amounts of regression testing, e.g.
> multiple simultaneous networks with a mixture of
> publiclyAccessible='yes|no', running a systemwide instance of dnsmasq
> with bind-dynamic//bind-interfaces at the same time, routed and nated
> networks, qemu and lxc guests with/without dhcp, etc). But I think that
> we should still try to find a solution to the original problem that
> doesn't involve opening up the DNS and DHCP servers to receiving packets
> that arrived on *any* interface. (Cedric - perhaps you could provide a
> config for something that doesn't work so that I can be sure I'm
> understanding exactly what is the problem).
> 

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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]