[libvirt] [PATCHv3] handle DNS over IPv6

Laine Stump laine at laine.org
Tue Jan 18 17:08:28 UTC 2011


(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 at 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 at 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;
>   }
>
> -
> +#define FAMILIES_COUNT 2
>   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);
> -     */
>       virCommandAddArgList(cmd,
> -                         "--listen-address", bridgeaddr,
>                            "--except-interface", "lo",
>                            NULL);
>
> +    /*
> +     * --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 */
> +err5:
> +    iptablesRemoveTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53);
> +err4:
> +    iptablesRemoveForwardAllowCross(driver->iptables, AF_INET6, network->def->bridge);
>   err3:
>       iptablesRemoveForwardRejectIn(driver->iptables, AF_INET6, network->def->bridge);
>   err2:

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




More information about the libvir-list mailing list