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@xxxxxxxxxx>
---
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");
