Re: [libvirt] [PATCH 2/3] Add a "forward family" option

On 10/15/2012 10:54 AM, Michal Privoznik wrote: > On 15.10.2012 12:27, Benjamin Cama wrote: >> It allows to specify forwarding only for IPv4 or IPv6. Not specifying it >> enables forwarding for both protocols. Okay, I can understand the usefulness of this option (although I recall that when I added IPv6 support, we discussed whether or not to have a separate forward mode for ipv4 and ipv6 on the same network, and decided against it, because it created unnecessary complexity).

See below though - I'm thinking it might make more sense to add an attribute to each <ip> element rather than to the <forward> element. Perhaps something like: <ip family='ipv6' forward='none' address='fdd6:4978:2ac:5::1' prefix='64'> <ip family='ipv4' forward='nat' address='192.168.123.1' netmask='255.255.255.0'> etc. This setting would override whatever was set in the <forward> element for that particular IP address.

>> --- >> src/conf/network_conf.c | 58 >> ++++++++++++++++++++++++++++++++++++++++++++++- >> src/conf/network_conf.h | 1 + >> 2 files changed, 58 insertions(+), 1 deletions(-) > When you introduce new feature to XML schema, you must update > docs/schemas/*.rng and docs/format*.html.in > >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index 891d48c..8c2ae9b 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -1204,6 +1204,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) >> xmlNodePtr virtPortNode = NULL; >> xmlNodePtr forwardNode = NULL; >> int nIps, nPortGroups, nForwardIfs, nForwardPfs, nForwardAddrs; >> + char *forwardFamily = NULL; >> char *forwardDev = NULL; >> char *forwardManaged = NULL; >> char *type = NULL; >> @@ -1358,6 +1359,23 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) >> def->forwardType = VIR_NETWORK_FORWARD_NAT; >> } >> >> + forwardFamily = virXPathString("string(./@family)", ctxt); >> + if (forwardFamily) { >> + if (STREQ(forwardFamily, "ipv4")) { >> + def->forwardFamily = AF_INET; >> + } else if (STREQ(forwardFamily, "ipv6")) { >> + def->forwardFamily = AF_INET6; >> + } else { >> + virNetworkReportError(VIR_ERR_XML_ERROR, >> + _("Unknown forward family '%s'"), >> forwardFamily); > %s/virNetworkReportError/virReportError/ > >> + VIR_FREE(forwardFamily); >> + goto error; >> + } >> + VIR_FREE(forwardFamily); >> + } else { >> + def->forwardFamily = AF_UNSPEC; >> + } >> + > Usually, we create an enum for this and use vir.*TypeFromString() and > vir.*TypeToString(); > >> forwardDev = virXPathString("string(./@dev)", ctxt); >> forwardManaged = virXPathString("string(./@managed)", ctxt); >> if(forwardManaged != NULL) { >> @@ -1515,8 +1533,16 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) >> VIR_FREE(forwardIfNodes); >> VIR_FREE(forwardAddrNodes); >> switch (def->forwardType) { >> - case VIR_NETWORK_FORWARD_ROUTE: >> case VIR_NETWORK_FORWARD_NAT: >> + if (def->forwardFamily == AF_INET6) { >> + virNetworkReportError(VIR_ERR_XML_ERROR, >> + _("%s forwarding is not allowed in >> IPv6 (network '%s')"), >> + >> virNetworkForwardTypeToString(def->forwardType), >> + def->name); >> + goto error; >> + } Okay. It took me a minute to parse this, but what you're saying is "if the forward type is 'nat', this implies doing NAT forwarding of IPv4 packets, so it doesn't make sense to say that only IPv6 forwarding is allowed".

Note that in the future that may not remain the case, depending on what, if anything, libvirt does with IPv6 NAT (this is as good a place as any to start reading: http://lwn.net/Articles/452293/)

Which actually brings up another topic - what do we do with combined ipv4/6 networks that have forward mode='nat' if/when we do support some type of IPv6 NAT? Since there will already be a lot of deployments in the field that expect the current behavior, we really can't just suddenly change the meaning of <forward mode='nat'> "NAT ipv4 + route ipv6" to "NAT ipv4 + NAT ipv6" - that would break too many existing installations. We may want to think about the implications of the above before we commit to any particular attribute scheme for limiting the forwarding to one type or another.

As a matter of fact, I just thought of something else - what about a network with multiple IP addresses where we only want to allow forwarding of traffic with addresses on one of those IP networks? Perhaps this should be an attribute of the <ip> element rather than of the <forward> element. (this thought stuck in my head so much that I went back and mentioned it at the top of this message).