[libvirt] [PATCH] network: add an option to make dns public
John Ferlan
jferlan at redhat.com
Fri Jun 12 11:32:38 UTC 2015
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><dns></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).
>
More information about the libvir-list
mailing list