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

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

 



On Tue, 2015-06-16 at 11:21 -0400, Laine Stump wrote:
> On 06/16/2015 09:08 AM, Cedric Bosdonnat wrote:
> > Hi Laine,
> >
> > On Thu, 2015-06-11 at 13:37 -0400, 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.
> > Given the other names around that didn't shock me, but it's surely not a
> > good habit to introduce such lengthy names ;)
> 
> Well, "forwardPlainNames" was another invention of mine, and one which
> I'm not proud of, but I gave fair published notice that I would accept
> other suggestions, and still couldn't come up with something better. The
> trick is in getting the balance between short/cryptic and
> long/unambiguously descriptive right. The worst outcome is to have
> something short and cryptic that could easily be misunderstood to mean
> something else.
> 
> Do you think "public" is specific enough? Or might that possibly be
> confused with some other intent?

public could work, I don't think there are much more cases that would
fit this case for the DNS.

> >>>> 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 :-)
> > I'm fixing that one.
> >
> >>>>                <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.
> > Ok, will do.
> >
> >>> 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).
> >>
> >> 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?).
> > The problem comes with people setting VLANs on top of the libvirt
> > network. Then the DHCP / DNS requests are arriving on veth0.x instead of
> > veth0... and our dnsmasq doesn't take these requests.
> 
> It sounds almost like a misuse of the libvirt network, and if that's the
> case then I don't really care about a separate patch that fixes just
> that problem - if you're already hacking up the network outside the
> scope of libvirt (in a way that's not supported by libvirt), then it's
> reasonable you should need to hack it up a bit more to make it work.
> 
> (after reading your description at the end, yes it is an abuse of
> libvirt's networks. See down at the bottom for my full opinion)
> 
> 
> >> 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).
> > Can all those tests be automated in our test suites? 
> 
> No, I'm talking about tests that should be run at least once manually by
> you prior to considering this "done". Any ACK given on the patch by me
> would be under the assumption that you had run such tests locally. If
> you want to add them to one of the automated test suites, that would be
> even better (are people still running the libvirt-tck?)

Yes, we are running libvirt-tck after each change in our libvirt
packages. I'll check if those tests can be added there easily: it would
really annoy me to have to setup those tests manually every time I want
them.

> > Apart from the
> > configuration tests mentioned by John, those you mention really look
> > like integration tests to be done somewhere else.
> 
> In order to detect future regressions at a later time, yes. To be sure
> that your new feature works properly and doesn't cause regressions in
> other features when it is used, they should at least be setup and run
> manually when the feature is added.

Sure.

> >
> >>  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).
> > To reproduce the problem:
> >
> >   * On the host, create a virbr0.300 vlan interface, assign it an IP
> >   * Create a guest using the default network (or the one creating
> > virbr0)
> >   * In the guest, create an eth0.300 vlan device.
> >
> > In this setup all DHCP and DNS requests won't be handled by virbr0's
> > dnsmasq, as it doesn't listen to the virbr0.* interfaces.
> >
> > After some more tests, I found out that we can have this in the dnsmasq
> > config:
> >
> >     interface=virbr0
> >     interface=virbr0.*
> >
> > Even with bind-dynamic, this would make the dnsmasq listen on the vlans
> > of the bridge. The problems I see with such an approach to fix the vlan
> > problem are:
> >   * some people / tools don't use the dev.vlanid naming convention, so
> > it wouldn't work with names like vlan0
> >   * I guess we can imaging people setting interfaces named like
> > virbr0.hacked to match the wildcard, even though it's not a VLAN.
> >
> > Any thoughts on that?
> 
> Actually after seeing this description, I think it's something that
> libvirt doesn't need to try and explicitly fix. People who do this are
> already hacking around with libvirt's network in a way which wasn't
> intended, so I don't think it's necessary/desirable to make it work by
> default, or even to provide a separate specific option to make it work -
> if someone wants to do this, they deserve what they get, and requiring
> them to use bind-interfaces instead of bind-dynamic (i.e. to turn on
> "publiclyAccessible") is perfectly acceptable.

Ok, then I won't change the interfaces list there.

> I would suggest maybe coming up with a patch to properly support this
> config in libvirt's network config, but it's unclear to me what is the
> advantage of creating a vlan on a network that isn't connected to
> anywhere else at L2 anyway - it just creates a need to configure a vlan
> interface in the guest (which you may or may not trust the guest to do),
> and ends up only serving to isolate one group of guests connected to
> that network from another group of guests connected to that network (as
> long as the guests themselves choose to honor the isolation! If the
> guests want though, they can easily get around this). It seems to me
> that a better idea would be to just create a separate libvirt network
> (i.e. a separate bridge device) for each group of guests and forget
> about burdening everyone with the extra task of setting up a vlan interface.
> 

I never gave the exact use case of this hack: it's all for testing
SUSECloud. The guys are setting up libvirt network and having vlans for
the different networks of the cloud. The whole problem is about testing
in the end ;)

--
Cedric

--
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]