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

Re: [libvirt] [PATCHv3] handle DNS over IPv6

(Sorry for the delay in responding. We're in a bit of a crunch ;-)

On 01/12/2011 04:24 PM, Paweł Krześniak wrote:
2011/1/7 Daniel P. Berrange<berrange redhat com>:
Practically no apps
will be do this and so they typically can't make use of
the link-local address.
I agree.
v2 of patch attached.

As Eric indicated, it would be more convenient if your mailer attached the patch as an ASCII file rather than base64-encoded (I'm guessing possibly it did that automatically because of the accented characters in your name - perhaps there's a config parameter that would cause it to use some other encoding when it encounters things outside basic 7bit ASCII?)

commit ce44728e232a39ff42bf87f5eb102bc76b71241d
Author: Pawel Krzesniak<pawel krzesniak gmail com>
Date:   Tue Jan 11 23:36:19 2011 +0100

     bridge_driver: handle DNS over IPv6
     * dnsmasq listens on all defined IPv[46] addresses for network
     * Add ip6tables rules to allow DNS traffic to host
  src/network/bridge_driver.c |   59 ++++++++++++++++++++++++++++++++++--------
  1 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 4c64a74..d5524db 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -423,7 +423,7 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
      return 0;

  static int
  networkBuildDnsmasqArgv(virNetworkObjPtr network,
                          virNetworkIpDefPtr ipdef,

I'd been thinking that, if we were going to add this capability, we may as well eliminate the virNetworkIpDefPtr arg from the function call - we're already iterating over all the IPs anyway, so we may as well look for the IP that has the ranges and static hosts as we go through the list.

(As a matter of fact, doing this would make it easier to (in a future patch, not here and now! :-) add in the ranges and static hosts from *all* IPs on an interface, not just the first found to have them. I'm not a dnsmasq expert, but it looks like this would require using the "network-id" detailed in the dnsmasq man page, along with multiple --dhcp-option arguments (and probably some other things).

@@ -431,7 +431,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
                          virCommandPtr cmd) {
      int r, ret = -1;
      int nbleases = 0;
-    char *bridgeaddr;
+    char *bridgeaddr = NULL;
+    char *ipaddr = NULL;
+    int ii, jj;
+    virNetworkIpDefPtr tmpipdef;
+    int families[FAMILIES_COUNT] = { AF_INET, AF_INET6 };

I actually think it's a bit of overkill to do this. At this point, we only have AF_INET and AF_INET6 addresses, and we want to do the same thing with all of them, so we don't even need to look at family.

      if (!(bridgeaddr = virSocketFormatAddr(&ipdef->address)))
          goto cleanup;
@@ -468,20 +472,29 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
      /* *no* conf file */
      virCommandAddArgList(cmd, "--conf-file=", "", NULL);

-    /*
-     * XXX does not actually work, due to some kind of
-     * race condition setting up ipv6 addresses on the
-     * interface. A sleep(10) makes it work, but that's
-     * clearly not practical
-     *
-     * virCommandAddArg(cmd, "--interface");
-     * virCommandAddArg(cmd, ipdef->bridge);
-     */
-                         "--listen-address", bridgeaddr,
                           "--except-interface", "lo",

+    /*
+     * --interface does not actually work with dnsmasq<  2.47,
+     *  due to DAD for ipv6 addresses on the interface.
+     *
+     * virCommandAddArgList(cmd, "--interface", ipdef->bridge, NULL);
+     *
+     * So listen on all defined IPv[46] addresses
+     */

As mentioned above - just make the following a single loop with family "AF_UNSPEC" so it gets all the ip addresses. By definition, every item on the list will have either an AF_INET or an AF_INET6 address.

+    for (ii = 0; ii<  FAMILIES_COUNT; ii++) {
+        for (jj = 0;
+             (tmpipdef = virNetworkDefGetIpByIndex(network->def, families[ii], jj));
+             jj++) {
+            if (!(ipaddr = virSocketFormatAddr(&tmpipdef->address)))
+                goto cleanup;
+            virCommandAddArgList(cmd, "--listen-address", ipaddr, NULL);
+            VIR_FREE(ipaddr);
+        }
+    }

As part of the "future patch" mentioned above to support ranges/static hosts for all IP addresses, this next bit (about nranges) could be moved inside the above loop. For now, if you remove the virDomainNetIpDefPtr arg from the call to this function, you would want to check for the presences of hosts or ranges at each iteration through the loop.

      for (r = 0 ; r<  ipdef->nranges ; r++) {
          char *saddr = virSocketFormatAddr(&ipdef->ranges[r].start);
          if (!saddr)
@@ -1027,9 +1040,31 @@ networkAddGeneralIp6tablesRules(struct network_driver *driver,
          goto err3;

+    /* allow DNS over IPv6 */
+    if (iptablesAddTcpInput(driver->iptables, AF_INET6,
+                            network->def->bridge, 53)<  0) {
+        networkReportError(VIR_ERR_SYSTEM_ERROR,
+                           _("failed to add ip6tables rule to allow DNS requests from '%s'"),
+                           network->def->bridge);
+        goto err4;
+    }
+    if (iptablesAddUdpInput(driver->iptables, AF_INET6,
+                            network->def->bridge, 53)<  0) {
+        networkReportError(VIR_ERR_SYSTEM_ERROR,
+                           _("failed to add ip6tables rule to allow DNS requests from '%s'"),
+                           network->def->bridge);
+        goto err5;
+    }
      return 0;

      /* unwind in reverse order from the point of failure */
+    iptablesRemoveTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53);
+    iptablesRemoveForwardAllowCross(driver->iptables, AF_INET6, network->def->bridge);
      iptablesRemoveForwardRejectIn(driver->iptables, AF_INET6, network->def->bridge);

I'm fine with this 2nd part of the patch. (I don't think there's a need for a single function to add both tcp and udp rules (as Eric suggested); I'd rather each call correspond to a single rule.)

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