[libvirt PATCH 2/3] conf: add an attribute to turn on NAT for IPv6 virtual networks
Laine Stump
laine at redhat.com
Tue Jun 9 04:07:22 UTC 2020
On 6/8/20 10:51 AM, 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.
>
> Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>
> ---
> docs/formatnetwork.html.in | 14 ++++++++++
> docs/schemas/network.rng | 8 ++++++
> src/conf/network_conf.c | 26 +++++++++++++++++--
> src/conf/network_conf.h | 2 ++
> .../nat-network-forward-nat-ipv6.xml | 10 +++++++
> .../nat-network-forward-nat-ipv6.xml | 11 ++++++++
> tests/networkxml2xmltest.c | 1 +
> 7 files changed, 70 insertions(+), 2 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..42cfb6708a 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
Missing . at the end.
> + </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..d9448fa3bb 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -181,6 +181,14 @@
> </optional>
> <optional>
> <element name='nat'>
> + <optional>
> + <attribute name="ipv6">
> + <choice>
> + <value>yes</value>
> + <value>no</value>
> + </choice>
> + </attribute>
<ref name="virYesNo"/>
> + </optional>
> <interleave>
> <optional>
> <element name='address'>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index f1d22b25b1..cd7683e94b 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;
> VIR_XPATH_NODE_AUTORESTORE(ctxt);
>
> ctxt->node = node;
> @@ -1369,6 +1370,19 @@ virNetworkForwardNatDefParseXML(const char *networkName,
> goto cleanup;
> }
>
> + ipv6 = virXMLPropString(node, "ipv6");
> + if (ipv6) {
> + if ((def->natIPv6
> + = virTristateBoolTypeFromString(ipv6)) <= 0) {
You need to parse this into a temporary int, otherwise you'll never see
< 0, and so won't be able to detect errors.
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid ipv6 setting '%s' "
> + "in network '%s' NAT"),
> + ipv6, networkName);
> + goto cleanup;
> + }
> + VIR_FREE(ipv6);
> + }
> +
> /* addresses for SNAT */
> nNatAddrs = virXPathNodeSet("./address", ctxt, &natAddrNodes);
> if (nNatAddrs < 0) {
> @@ -2516,10 +2530,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)
You'd *think* it would be enough to just add !fwd->natIPv6 here.
But you'd unfortunately be wrong. :-( The problem is that in
virNetworkDefFormatBuf() there is a local called "shortforward" - if
shortforward is true, then the <forward> element is completed in a
single line. And sadly, rather than being set based on whether or not
virNetworkForwaddNatDefFormat() produces any output, shortforward is set
by directly checking a bunch of attributes (includeing
def->forward.port.(start|end).)
The result is that with the current code you have, when you parse and
format the example from the commit log message, you end up with just:
<forward mode='nat'/>
<nat ipv6='yes'/>
(note the <forward> element ends early). You could just add another
check for natIPv6 to virNetworkDefFormatBuf(), or you could go the whole
9 yards, do the honorable thing and make it depend on whether or not the
subordinate function produces output.
> 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;
> + }
I think somebody is going to insist you put a blank line after the } (I
personally don't care, but pedants lurk in every shadow! :-)
> + virBufferAddLit(buf, ">\n");
> virBufferAdjustIndent(buf, 2);
>
> if (addrStart) {
> 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..c3b641224c
> --- /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" dev="eth1">
> + <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..74e1c36c69
> --- /dev/null
> +++ b/tests/networkxml2xmlout/nat-network-forward-nat-ipv6.xml
> @@ -0,0 +1,11 @@
> +<network>
> + <name>default</name>
> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
> + <forward dev='eth1' mode='nat'>
> + <nat ipv6='yes'/>
> + <interface dev='eth1'/>
Aha! This is the reason you didn't notice it in your unit test - the
unit test also specifies an interface dev, which *is* checked for
"shortforward".
> + </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");
More information about the libvir-list
mailing list