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><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 :-)
> > 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