[libvirt PATCH v2 2/3] conf: add an attribute to turn on NAT for IPv6 virtual networks

Laine Stump laine at redhat.com
Wed Jun 10 04:14:51 UTC 2020


On 6/9/20 12:17 PM, Daniel P. Berrangé wrote:
> Historically IPv6 did not support NAT, so when IPv6 was added to
> libvirt's virtual networks, when requesting <forward mode="nat"/>
> libvirt will NOT apply NAT to IPv6 traffic, only IPv4 traffic.
>
> This is an annoying historical design decision as it means we
> cannot enable IPv6 automatically. We thus need to introduce a
> new attribute
>
>     <forward mode="nat">
>       <nat ipv6="yes"/>
>     </forward>
>
> The new attribute is a tri-state, so it leaves open the possibility of
> us intentionally changing the default behaviour in future to honour
> NAT for IPv6.


Any network defined with an older libvirt, and even newly defined 
networks that don't have ipv6 set explicitly in the input XML, won't 
have an explicit value saved in the config. So if we change the default, 
those existing networks will have different behavior. I don't see how we 
could change the default without those existing networks getting 
automatically changed to ipv6='yes'.


>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
>   docs/formatnetwork.html.in                    | 14 +++++++++
>   docs/schemas/network.rng                      |  5 ++++
>   src/conf/network_conf.c                       | 30 +++++++++++++++++--
>   src/conf/network_conf.h                       |  2 ++
>   .../nat-network-forward-nat-ipv6.xml          | 10 +++++++
>   .../nat-network-forward-nat-ipv6.xml          | 10 +++++++
>   tests/networkxml2xmltest.c                    |  1 +
>   7 files changed, 69 insertions(+), 3 deletions(-)
>   create mode 100644 tests/networkxml2xmlin/nat-network-forward-nat-ipv6.xml
>   create mode 100644 tests/networkxml2xmlout/nat-network-forward-nat-ipv6.xml
>
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index 0383e2d891..fb740111b1 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -276,6 +276,20 @@
>       </nat>
>     </forward>
>   ...</pre>
> +
> +            <p>
> +              <span class="since">Since 6.5.0</span> it is possible to
> +              enable NAT with IPv6 networking. As noted above, IPv6
> +              has historically done plain forwarding and thus to avoid
> +              breaking historical compatibility, IPv6 NAT must be
> +              explicitly requested.
> +            </p>
> +            <pre>
> +...
> +  <forward mode='nat'>
> +    <nat ipv6='yes'/>
> +  </forward>
> +...</pre>
>             </dd>
>   
>             <dt><code>route</code></dt>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 88b6f4dfdd..3a5eb3ced4 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -181,6 +181,11 @@
>                 </optional>
>                 <optional>
>                   <element name='nat'>
> +                  <optional>
> +                    <attribute name="ipv6">
> +                      <ref name="virYesNo"/>
> +                    </attribute>
> +                  </optional>
>                     <interleave>
>                       <optional>
>                         <element name='address'>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index f1d22b25b1..1b89e2985d 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1358,6 +1358,7 @@ virNetworkForwardNatDefParseXML(const char *networkName,
>       int nNatAddrs, nNatPorts;
>       char *addrStart = NULL;
>       char *addrEnd = NULL;
> +    char *ipv6 = NULL;


Eww. I just noticed this file isn't doing autofree memory allocation on  
many of its pointers...


>       VIR_XPATH_NODE_AUTORESTORE(ctxt);
>   
>       ctxt->node = node;
> @@ -1369,6 +1370,20 @@ virNetworkForwardNatDefParseXML(const char *networkName,
>           goto cleanup;
>       }
>   
> +    ipv6 = virXMLPropString(node, "ipv6");
> +    if (ipv6) {
> +        int natIPv6;
> +        if ((natIPv6 = virTristateBoolTypeFromString(ipv6)) <= 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("Invalid ipv6 setting '%s' "
> +                             "in network '%s' NAT"),
> +                           ipv6, networkName);
> +            goto cleanup;


...which leads to a memory leak here, since the code at the cleanup 
label doesn't free ipv6.


For some reason I felt like spending an hour doing something mindless 
just now, so I made a patch to convert all the char* and xmlNodePtr* in 
network_conf.c to g_autofree (along with a few other g_autoptr things, 
and the standard cleanup/error label removals). I'll post that as a 
patch based on your patches; you can either just assume that my patch 
will go in after yours and fix the memory leak, or you can just change 
ipv6 to a g_autofree yourself and I'll adjust my patch accordingly.


> +        }
> +        def->natIPv6 = natIPv6;
> +        VIR_FREE(ipv6);
> +    }
> +
>       /* addresses for SNAT */
>       nNatAddrs = virXPathNodeSet("./address", ctxt, &natAddrNodes);
>       if (nNatAddrs < 0) {
> @@ -2516,10 +2531,18 @@ virNetworkForwardNatDefFormat(virBufferPtr buf,
>               goto cleanup;
>       }
>   
> -    if (!addrEnd && !addrStart && !fwd->port.start && !fwd->port.end)
> +    if (!addrEnd && !addrStart && !fwd->port.start && !fwd->port.end && !fwd->natIPv6)
>           return 0;
>   
> -    virBufferAddLit(buf, "<nat>\n");
> +    virBufferAddLit(buf, "<nat");
> +    if (fwd->natIPv6)
> +        virBufferAsprintf(buf, " ipv6='%s'", virTristateBoolTypeToString(fwd->natIPv6));
> +
> +    if (!addrEnd && !addrStart && !fwd->port.start && !fwd->port.end) {
> +        virBufferAddLit(buf, "/>\n");
> +        return 0;
> +    }
> +    virBufferAddLit(buf, ">\n");
>       virBufferAdjustIndent(buf, 2);
>   
>       if (addrStart) {
> @@ -2627,7 +2650,8 @@ virNetworkDefFormatBuf(virBufferPtr buf,
>                            || def->forward.port.start
>                            || def->forward.port.end
>                            || (def->forward.driverName
> -                             != VIR_NETWORK_FORWARD_DRIVER_NAME_DEFAULT));
> +                             != VIR_NETWORK_FORWARD_DRIVER_NAME_DEFAULT)
> +                         || def->forward.natIPv6);
>           virBufferAsprintf(buf, "%s>\n", shortforward ? "/" : "");
>           virBufferAdjustIndent(buf, 2);
>   
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index f2dc388ef0..e3a61c62ea 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -244,6 +244,8 @@ struct _virNetworkForwardDef {
>       /* ranges for NAT */
>       virSocketAddrRange addr;
>       virPortRange port;
> +
> +    virTristateBool natIPv6;
>   };
>   
>   typedef struct _virPortGroupDef virPortGroupDef;
> diff --git a/tests/networkxml2xmlin/nat-network-forward-nat-ipv6.xml b/tests/networkxml2xmlin/nat-network-forward-nat-ipv6.xml
> new file mode 100644
> index 0000000000..c360941e1e
> --- /dev/null
> +++ b/tests/networkxml2xmlin/nat-network-forward-nat-ipv6.xml
> @@ -0,0 +1,10 @@
> +<network>
> +  <name>default</name>
> +  <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
> +  <bridge name="virbr0"/>
> +  <forward mode="nat">
> +    <nat ipv6="yes"/>
> +  </forward>
> +  <ip family="ipv6" address="2001:db8:ac10:fe01::1" prefix="64">
> +  </ip>
> +</network>
> diff --git a/tests/networkxml2xmlout/nat-network-forward-nat-ipv6.xml b/tests/networkxml2xmlout/nat-network-forward-nat-ipv6.xml
> new file mode 100644
> index 0000000000..cfec391ee2
> --- /dev/null
> +++ b/tests/networkxml2xmlout/nat-network-forward-nat-ipv6.xml
> @@ -0,0 +1,10 @@
> +<network>
> +  <name>default</name>
> +  <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
> +  <forward mode='nat'>
> +    <nat ipv6='yes'/>
> +  </forward>
> +  <bridge name='virbr0' stp='on' delay='0'/>
> +  <ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'>
> +  </ip>
> +</network>
> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c
> index 700744785a..17817418b7 100644
> --- a/tests/networkxml2xmltest.c
> +++ b/tests/networkxml2xmltest.c
> @@ -140,6 +140,7 @@ mymain(void)
>       DO_TEST("nat-network-dns-forward-plain");
>       DO_TEST("nat-network-dns-forwarders");
>       DO_TEST("nat-network-dns-forwarder-no-resolv");
> +    DO_TEST("nat-network-forward-nat-ipv6");
>       DO_TEST("nat-network-forward-nat-address");
>       DO_TEST("nat-network-forward-nat-no-address");
>       DO_TEST("nat-network-mtu");


I don't know that I agree with the comment about ease of changing the 
default in the future (maybe I misunderstood), and if you want the patch 
to stand on its own you should fix the one memory leak (my patch will be 
posted soon, if not already), but it all looks good and works properly 
now (for defining the network at least - I haven't actually tried 
getting an IPv6 address on a guest yet, but I'll let you know if there 
are any problems :-)


Reviewed-by: Laine Stump <laine at redhat.com>




More information about the libvir-list mailing list