[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv3 1/3] v2.0: allow guest to guest IPv6 without gateway definition



On 12/03/2012 11:13 AM, Gene Czarcinski wrote:
> This patch adds the capability for virtual guests to do IPv6
> communication via a virtual network interface with no IPv6
> (gateway) addresses specified.  This capability currently
> exists for IPv4.
>
> This patch allows creation of a completely isolated IPv6 network.
>
> Note that virtual guests cannot communication with the virtualization
> host via this interface.  Also note that:
>       net.ipv6.conf.<interface_name>.disable_ipv6 = 1
>
> Also not that starting libvirtd has set the following:
>   net.bridge.bridge-nf-call-arptables = 1
>   net.bridge.bridge-nf-call-ip6tables = 1
>   net.bridge.bridge-nf-call-iptables = 1
> although /etc/syslog.conf has them all set to 0.
>
> To control this behavior so that it is not enabled by default, the parameter
> ipv6='yes' on the <network> statement has been added.
>
> Documentation related to this patch has been updated.
> The network schema has also been updated.
> ---
>  docs/formatnetwork.html.in  | 28 +++++++++++++++++++++++++++-
>  docs/schemas/network.rng    | 10 ++++++++++
>  src/conf/network_conf.c     |  8 ++++++++
>  src/conf/network_conf.h     |  5 +++++
>  src/network/bridge_driver.c | 25 ++++++++++++++++++++-----
>  5 files changed, 70 insertions(+), 6 deletions(-)
>
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index 49206dd..a3a5ced 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -33,7 +33,7 @@
>      </p>
>  
>      <pre>
> -      &lt;network&gt;
> +      &lt;network ipv6='yes'&gt;
>          &lt;name&gt;default&lt;/name&gt;
>          &lt;uuid&gt;3e3fce45-4f53-4fa7-bb32-11f34168b82b&lt;/uuid&gt;
>          ...</pre>
> @@ -52,6 +52,12 @@
>          The format must be RFC 4122 compliant, eg <code>3e3fce45-4f53-4fa7-bb32-11f34168b82b</code>.
>          If omitted when defining/creating a new network, a random
>          UUID is generated. <span class="since">Since 0.3.0</span></dd>
> +      <dt><code>ipv6='yes'</code></dt>
> +      <dd>The new, optional parameter <code>ipv6='yes'</code> enables
> +        a network definition with no IPv6 gateway addresses specified
> +        to have guest-to-guest communications.  For further information,
> +        see the example below for the example with no gateway addresses.
> +        <span class="since">Since 1.0.1</span></dd>
>      </dl>
>  
>      <h3><a name="elementsConnect">Connectivity</a></h3>
> @@ -773,5 +779,25 @@
>          &lt;/forward&gt;
>        &lt;/network&gt;</pre>
>  
> +    <h3><a name="examplesNoGateway">Network config with no gateway addresses</a></h3>
> +
> +    <p>
> +    A valid network definition can contain no IPv4 or IPv6 addresses.  Such a definition
> +    can be used for a "very private" or "very isolated" network since it will not be
> +    possible to communicate with the virtualization host via this network.  However,
> +    this virtual network interface can be used for communication between virtual guest
> +    systems.  This works for IPv4 and <span class="since">(Since 1.0.1)</span> IPv6.
> +    However, the new ipv6='yes' must be added for guest-to-guest IPv6
> +    communication.
> +    </p>

I reformatted this paragraph to fit within 80 columns.

> +
> +    <pre>
> +      &lt;network ipv6='yes'&gt;
> +        &lt;name&gt;nogw&lt;/name&gt;
> +        &lt;uuid&gt;7a3b7497-1ec7-8aef-6d5c-38dff9109e93&lt;/uuid&gt;
> +        &lt;bridge name="virbr2" stp="on" delay="0" /&gt;
> +        &lt;mac address='00:16:3E:5D:C7:9E'/&gt;
> +      &lt;/network&gt;</pre>
> +
>    </body>
>  </html>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 4abfd91..0d67f7f 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -17,6 +17,16 @@
>            <data type="unsignedInt"/>
>          </attribute>
>        </optional>
> +      <!-- Enables IPv6 guest-to-guest communications on a network
> +           with no gateways addresses specified -->
> +      <optional>
> +        <attribute name="ipv6">
> +         <choice>
> +          <value>yes</value>
> +          <value>no</value>
> +          </choice>
> +         </attribute>
> +       </optional>
>        <interleave>
>  
>          <!-- The name of the network, used to refer to it through the API
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 6ce2e63..3f9e13c 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1264,6 +1264,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
>          def->uuid_specified = true;
>      }
>  
> +    /* check if definitions with no IPv6 gateway addresses is to
> +     * allow guest-to-guest communications.
> +     */
> +    if (virXPathBoolean("boolean(./@ipv6)", ctxt) == 1)
> +        def->ipv6nogw = true;

I don't think virXPathBoolean does what you think it does (although I
haven't figured out exactly what it *does* do, I've noticed that 1) none
of the rest of libvirt uses it for yes/no attributes, and 2) in the
libxml2 source code, when an XPATH_BOOLEAN is formatted into a string,
it becomes "True" or "False". I'm changing the above lines to the following:

    ipv6nogwStr = virXPathString("string(./@ipv6)", ctxt);
    if (ipv6nogwStr) {
        if (STREQ(ipv6nogwStr, "yes")) {
            def->ipv6nogw = true;
        } else if (STRNEQ(ipv6nogwStr, "no")) {
            virReportError(VIR_ERR_XML_ERROR,
                           _("Invalid ipv6 setting '%s' in network '%s'"),
                           ipv6nogwStr, def->name);
            goto error;
        }
        VIR_FREE(ipv6nogwStr);
    }

(with the necessary NULL-initialized declaration of ipv6nogwStr at the
beginning of the function, and VIR_FREE(ipv6nogwStr) at the error: label).

Depending on the amount of changes I make beyond that, I'll either
attach an interdiff to this mail, or send an update of your patch with
that change.

> +
>      /* Parse network domain information */
>      def->domain = virXPathString("string(./domain[1]/@name)", ctxt);
>  
> @@ -1839,6 +1845,8 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags)
>      if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->connections > 0)) {
>          virBufferAsprintf(&buf, " connections='%d'", def->connections);
>      }
> +    if (def->ipv6nogw)
> +        virBufferAddLit(&buf, " ipv6='yes'");
>      virBufferAddLit(&buf, ">\n");
>      virBufferAdjustIndent(&buf, 2);
>      virBufferEscapeString(&buf, "<name>%s</name>\n", def->name);
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 3e46304..949b3d2 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -184,6 +184,11 @@ struct _virNetworkDef {
>      virMacAddr mac; /* mac address of bridge device */
>      bool mac_specified;
>  
> +    /* specified if ip6tables rules added
> +     * when no ipv6 gateway addresses specified.
> +     */
> +    bool ipv6nogw;

Heh. Someday we should go through the driver object structures and
change all of the ints and bitfields used as booleans into bool...

> +
>      int forwardType;    /* One of virNetworkForwardType constants */
>      int managed;        /* managed attribute for hostdev mode */
>  
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 75f3c3a..cb2997d 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1617,13 +1617,18 @@ networkRemoveRoutingIptablesRules(struct network_driver *driver,
>      }
>  }
>  
> -/* Add all once/network rules required for IPv6 (if any IPv6 addresses are defined) */
> +/* Add all once/network rules required for IPv6.
> + * If no IPv6 addresses are defined and <network ipv6='yes'> is
> + * specified, then allow IPv6 commuinications between virtual systems.

I changed "virtual systems" to "guests".

> + * If any IPv6 addresses are defined, then add the rules for regular operation.

Changed to "then add all rules for regular operation (including
inter-guest communication)."

> + */
>  static int
>  networkAddGeneralIp6tablesRules(struct network_driver *driver,
>                                 virNetworkObjPtr network)
>  {
>  
> -    if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0))
> +    if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) &&
> +                !network->def->ipv6nogw)
>          return 0;

Fixed odd indentation of !network->def->ipv6nogw, and put braces around
the body (since the expression is multi-line.

>  
>      /* Catch all rules to block forwarding to/from bridges */
> @@ -1653,6 +1658,10 @@ networkAddGeneralIp6tablesRules(struct network_driver *driver,
>          goto err3;
>      }
>  
> +    /* if no IPv6 addresses are defined, we are done. */
> +    if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0))
> +        return 0;
> +
>      /* allow DNS over IPv6 */
>      if (iptablesAddTcpInput(driver->iptables, AF_INET6,
>                              network->def->bridge, 53) < 0) {
> @@ -1689,11 +1698,17 @@ static void
>  networkRemoveGeneralIp6tablesRules(struct network_driver *driver,
>                                    virNetworkObjPtr network)
>  {
> -    if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0))
> +    if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) &&
> +                !network->def->ipv6nogw)
>          return;
> +    if (virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) {
> +        iptablesRemoveUdpInput(driver->iptables, AF_INET6, network->def->bridge, 53);
> +        iptablesRemoveTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53);
> +    }
>  
> -    iptablesRemoveUdpInput(driver->iptables, AF_INET6, network->def->bridge, 53);
> -    iptablesRemoveTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53);
> +    /* the following rules are there if no IPv6 address has been defined
> +     * but network->def->ipv6nogw == true
> +     */
>      iptablesRemoveForwardAllowCross(driver->iptables, AF_INET6, network->def->bridge);
>      iptablesRemoveForwardRejectIn(driver->iptables, AF_INET6, network->def->bridge);
>      iptablesRemoveForwardRejectOut(driver->iptables, AF_INET6, network->def->bridge);

One other thing that we forgot about - a test case for
networkxml2xmltest (xml2argv is only useful for things that affect
dhcp). I've added a very simple test case to networkxml2xml(in|out) and
added it to the list in networkxml2xmltest.c - it defines a network with
no IP addresses, but with "ipv6='yes'". I also added "ipv6='no'" to
networkxml2xmlin/isolated-network.xml to make sure that it is accepted
by the parser and results in ipv6nogw *not* being set (it of course
won't show up in the output).

ACK with the changes I've squashed in. I've attached an interdiff of the
changes I've made to this message, and will push as soon as someone else
ACKs those additional changes.

Thanks for the contribution! I know it's sometimes a pain to get things
all the way to pushing, but it really does help keep the expansion
manageable and prevent unwanted surprises for our diverse set of users.
>From 35d259fa52534a8b64a1784674820e06034b03c8 Mon Sep 17 00:00:00 2001
From: Laine Stump <laine laine org>
Date: Mon, 3 Dec 2012 15:53:04 -0500
Subject: [PATCH] Squash into 'allow guest to guest IPv6 without gateway
 definition'

---
 src/conf/network_conf.c                      | 16 ++++++++++++++--
 src/network/bridge_driver.c                  |  9 ++++++---
 tests/networkxml2xmlin/empty-allow-ipv6.xml  |  6 ++++++
 tests/networkxml2xmlin/isolated-network.xml  |  2 +-
 tests/networkxml2xmlout/empty-allow-ipv6.xml |  6 ++++++
 tests/networkxml2xmltest.c                   |  1 +
 6 files changed, 34 insertions(+), 6 deletions(-)
 create mode 100644 tests/networkxml2xmlin/empty-allow-ipv6.xml
 create mode 100644 tests/networkxml2xmlout/empty-allow-ipv6.xml

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 3f9e13c..8c77c50 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1226,6 +1226,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
     xmlNodePtr virtPortNode = NULL;
     xmlNodePtr forwardNode = NULL;
     int nIps, nPortGroups, nForwardIfs, nForwardPfs, nForwardAddrs;
+    char *ipv6nogwStr = NULL;
     char *forwardDev = NULL;
     char *forwardManaged = NULL;
     char *type = NULL;
@@ -1267,8 +1268,18 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
     /* check if definitions with no IPv6 gateway addresses is to
      * allow guest-to-guest communications.
      */
-    if (virXPathBoolean("boolean(./@ipv6)", ctxt) == 1)
-        def->ipv6nogw = true;
+    ipv6nogwStr = virXPathString("string(./@ipv6)", ctxt);
+    if (ipv6nogwStr) {
+        if (STREQ(ipv6nogwStr, "yes")) {
+            def->ipv6nogw = true;
+        } else if (STRNEQ(ipv6nogwStr, "no")) {
+            virReportError(VIR_ERR_XML_ERROR,
+                           _("Invalid ipv6 setting '%s' in network '%s'"),
+                           ipv6nogwStr, def->name);
+            goto error;
+        }
+        VIR_FREE(ipv6nogwStr);
+    }
 
     /* Parse network domain information */
     def->domain = virXPathString("string(./domain[1]/@name)", ctxt);
@@ -1597,6 +1608,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
     VIR_FREE(portGroupNodes);
     VIR_FREE(forwardIfNodes);
     VIR_FREE(forwardPfNodes);
+    VIR_FREE(ipv6nogwStr);
     VIR_FREE(forwardDev);
     ctxt->node = save;
     return NULL;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index cb2997d..e42303f 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1618,9 +1618,11 @@ networkRemoveRoutingIptablesRules(struct network_driver *driver,
 }
 
 /* Add all once/network rules required for IPv6.
+
  * If no IPv6 addresses are defined and <network ipv6='yes'> is
- * specified, then allow IPv6 commuinications between virtual systems.
- * If any IPv6 addresses are defined, then add the rules for regular operation.
+ * specified, then allow IPv6 commuinications between guests connected
+ * to this network. If any IPv6 addresses are defined, then add all
+ * rules for regular operation (including inter-guest communication).
  */
 static int
 networkAddGeneralIp6tablesRules(struct network_driver *driver,
@@ -1628,8 +1630,9 @@ networkAddGeneralIp6tablesRules(struct network_driver *driver,
 {
 
     if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) &&
-                !network->def->ipv6nogw)
+        !network->def->ipv6nogw) {
         return 0;
+    }
 
     /* Catch all rules to block forwarding to/from bridges */
 
diff --git a/tests/networkxml2xmlin/empty-allow-ipv6.xml b/tests/networkxml2xmlin/empty-allow-ipv6.xml
new file mode 100644
index 0000000..9d1e4e4
--- /dev/null
+++ b/tests/networkxml2xmlin/empty-allow-ipv6.xml
@@ -0,0 +1,6 @@
+<network ipv6='yes'>
+  <name>empty</name>
+  <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid>
+  <bridge name='virbr7' />
+  <mac address='52:54:00:17:3F:47'/>
+</network>
diff --git a/tests/networkxml2xmlin/isolated-network.xml b/tests/networkxml2xmlin/isolated-network.xml
index 0d562ea..5aa88c1 100644
--- a/tests/networkxml2xmlin/isolated-network.xml
+++ b/tests/networkxml2xmlin/isolated-network.xml
@@ -1,4 +1,4 @@
-<network>
+<network ipv6='no'>
   <name>private</name>
   <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
   <bridge name="virbr2" />
diff --git a/tests/networkxml2xmlout/empty-allow-ipv6.xml b/tests/networkxml2xmlout/empty-allow-ipv6.xml
new file mode 100644
index 0000000..6ee55ad
--- /dev/null
+++ b/tests/networkxml2xmlout/empty-allow-ipv6.xml
@@ -0,0 +1,6 @@
+<network ipv6='yes'>
+  <name>empty</name>
+  <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9c</uuid>
+  <bridge name='virbr7' stp='on' delay='0' />
+  <mac address='52:54:00:17:3F:47'/>
+</network>
diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c
index e57d190..1cd74d1 100644
--- a/tests/networkxml2xmltest.c
+++ b/tests/networkxml2xmltest.c
@@ -92,6 +92,7 @@ mymain(void)
     } while (0)
 #define DO_TEST(name) DO_TEST_FULL(name, 0)
 
+    DO_TEST("empty-allow-ipv6");
     DO_TEST("isolated-network");
     DO_TEST("routed-network");
     DO_TEST("nat-network");
-- 
1.7.11.7


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]