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

[Benjamin Cama]

Hi,

First, thanks Michal for your review.

Le lundi 15 octobre 2012 à 11:31 -0700, Laine Stump a écrit :
> 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).

I agree that this is a peculiar use case (having some “private”
addressing on one family, but not the other). My justification mainly
stands for workstations, not server, willing to experiment with IPv6, as
they (most of the time) won't have a prefix delegated to them to play
with (and forward to/from). Here, the IPv4 addressing (in my case) is
used for Internet access (as I cannot forward the ULA and don't have any
IPv6 prefix delegated) and also as a “backup stack” as I “play” with the
IPv6 stack. Maybe this is unnecessary, as it indeed adds 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.

May be a good idea. But this adds some processing for the firewalling
part, then.

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

Yes, that's what I meant.

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

Yes, it may, even if I don't like it.

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

Yes, I didn't think of these implications before. Still, I think that
IPv6 NAT should not be some kind of default if IPv4 NAT is selected. I
think (and hope) that routed IPv6 will be more prevalent than NATed
IPv6, in presence of IPv4 NAT.

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

Maybe, yes. But this will definitely tie the firewalling code to the
forwarding state. But this is maybe the only sane way to go. Still, it
would involve quite a change in the XML format.

I don't know if I should continue on this route (global per-familiy
forwarding). But I may not have the time to dig into the per-ip element
forwarding. Anyway, I am not sure this could make it for 1.0, so maybe I
should wait for some more debate on this.

BTW, thanks Laine for your review too.

Regards,
-- 
Benjamin Cama <benjamin.cama at telecom-bretagne.eu>