On 11/24/2014 12:48 PM, Laine Stump wrote:
> The promiscLinks attribute of a network's bridge subelement is used as
> a single switch to simultaneously turn on/off several options related
> to a bridge and its attached devices. When this is done, as many of
> the links to the bridge as possible have promiscuous mode turned off
> and/or flooding turned on. The result is better performance (because
> packets aren't being flooded to all ports, and can be dropped earlier
> when they are of no interest) and better security (since a guest will
> only receive traffic intended for the guest interface's configured MAC
> address).
>
> The attribute looks like this in the configuration:
>
> <network>
> <name>test</name>
> <bridge name='br0' promiscLinks='no'/>
> ...
>
> In order to preserve operational backward compatibility, the default
> for promiscLinks will be considered to be "yes" (although that won't
> be automatically stored in the XML, to allow for existing
> installations to automatically take advantage of this performance
> improvement in the future if we ever decide that it is safe to turn on
> everywhere).
I'm sure this makes sense if you've been looking at it every day for a
while; however, if I'm reading this correctly, the "desired eventual"
state is to set promiscLinks to no, but because we don't set it that way
today *and* because setting it to no means you have to have a specific
kernel (or better) installed, then we have to set this to 'yes' by
default. I guess I'm confused by the message that states the default is
'yes', but won't be saved in the XML "to allow existing installations to
automatically take advantage of this performance improvement in the
future" [assuming they have the right kernel version or later,
right?]... Do you see my confusion? Changing the default in the future
might not be the best solution given the requirement of a specific
kernel which we cannot ever seem to assume...
FWIW: My brain works the other way when considering new knobs. By
default it's not enabled because of a myriad of reasons, but if you set
this attribute to "yes" or "enabled" (or whatever), then this new
feature can be taken advantage of. By setting to yes, you've taken
responsibility to ensure you have the required config.
Because of internal IRC I know you struggled with a name for this and
using fdb='managed|auto' probably only means something to someone that
understands what the fdb is!
So either allow for setting each attribute separately (e.g.
promisc={yes|no} flood={yes|no}) or perhaps consider
'portMACFilter={yes|no}' (defaults to 'no'). If setting things
separately doesn't make sense (or is too confusing to describe), then
one knob is fine. If going with the latter, then I would think it's
easier to describe that by setting this attribute to yes, libvirt will
turn promiscuous mode (eg learning) and flooding off on each port. The
longer attribute name is still less than trustGuestRxFilters. BTW: To
me the name implies, hey this is something I want, even though it comes
with caveats.
The code below looks fine with regard to what you've done and how things
are currently named. I went through all of the above because you were
looking for a better name! Be careful what you ask for!
John
>
> The details of what is turned on/off will be described in a later
> enabling patch. This patch only adds the config knob, documentation,
> and test cases.
> ---
> docs/formatnetwork.html.in | 36 ++++++++++++++---
> docs/schemas/network.rng | 5 +++
> src/conf/network_conf.c | 47 +++++++++++++++++-----
> src/conf/network_conf.h | 1 +
> tests/networkxml2xmlin/host-bridge-no-flood.xml | 6 +++
> .../nat-network-explicit-flood.xml | 21 ++++++++++
> tests/networkxml2xmlout/host-bridge-no-flood.xml | 6 +++
> .../nat-network-explicit-flood.xml | 23 +++++++++++
> tests/networkxml2xmltest.c | 2 +
> 9 files changed, 130 insertions(+), 17 deletions(-)
> create mode 100644 tests/networkxml2xmlin/host-bridge-no-flood.xml
> create mode 100644 tests/networkxml2xmlin/nat-network-explicit-flood.xml
> create mode 100644 tests/networkxml2xmlout/host-bridge-no-flood.xml
> create mode 100644 tests/networkxml2xmlout/nat-network-explicit-flood.xml
>
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index dc438ae..82bdc88 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -81,7 +81,7 @@
>
> <pre>
> ...
> - <bridge name="virbr0" stp="on" delay="5"/>
> + <bridge name="virbr0" stp="on" delay="5" promiscLinks="no"/>
> <domain name="example.com"/>
> <forward mode="nat" dev="eth0"/>
> ...</pre>
> @@ -92,18 +92,42 @@
> defines the name of a bridge device which will be used to construct
> the virtual network. The virtual machines will be connected to this
> bridge device allowing them to talk to each other. The bridge device
> - may also be connected to the LAN. It is recommended that bridge
> - device names started with the prefix <code>vir</code>, but the name
> - <code>virbr0</code> is reserved for the "default" virtual
> - network. This element should always be provided when defining
> + may also be connected to the LAN. When defining
> a new network with a <code><forward></code> mode of
> +
> "nat" or "route" (or an isolated network with
> - no <code><forward></code> element).
> + no <code><forward></code> element), libvirt will
> + automatically generate a unique name for the bridge device if
> + none is given, and this name will be permanently stored in the
> + network configuration so that that the same name will be used
> + every time the network is started. For these types of networks
> + (nat, routed, and isolated), a bridge name beginning with the
> + prefix "virbr" is recommended (and that is what is
> + auto-generated), but not enforced.
> Attribute <code>stp</code> specifies if Spanning Tree Protocol
> is 'on' or 'off' (default is
> 'on'). Attribute <code>delay</code> sets the bridge's forward
> delay value in seconds (default is 0).
> <span class="since">Since 0.3.0</span>
> +
> + <p>
> + The <code>promiscLinks</code> attribute of the bridge
> + element is used to tell libvirt whether or not to attempt a
> + bridge configuration which disables flooding and turns off
> + promiscuous mode on as many ports as possible (this is done
> + by turning off each tap device's learning and unicast_flood
> + settings, and manually managing the bridge's forwarding
> + databas). It can be set to 'yes' or 'no' (defaults to 'yes'
> + to preserve backward compatibility). Turning off promiscuous
> + mode on the links to a bridge can improve performance and
> + provide better security, but can also cause some networking
> + setups to stop working (e.g. vlan tagging, multicast) and is
> + not supported by older kernels.
> + <span class="since">Since 1.2.11, requires kernel 3.17 or
> + newer</span>
> + </p>
> +
> +
> </dd>
> <dt><code>domain</code></dt>
> <dd>
> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
> index 4546f80..5c57165 100644
> --- a/docs/schemas/network.rng
> +++ b/docs/schemas/network.rng
> @@ -64,6 +64,11 @@
> <data type="unsignedLong"/>
> </attribute>
> </optional>
You could add an extra line here if you change this - to be like the
other optional attributes
> + <optional>
> + <attribute name="promiscLinks">
> + <ref name="virYesNo"/>
> + </attribute>
> + </optional>
>
> </element>
> </optional>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 067334e..d6ecc51 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -2108,6 +2108,18 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> }
> VIR_FREE(tmp);
>
> + tmp = virXPathString("string(./bridge[1]/@promiscLinks)", ctxt);
> + if (tmp) {
> + if ((def->promiscLinks
> + = virTristateBoolTypeFromString(tmp)) <= 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Invalid promiscLinks setting '%s' "
> + "in network '%s'"), tmp, def->name);
> + goto error;
> + }
> + VIR_FREE(tmp);
> + }
> +
> tmp = virXPathString("string(./mac[1]/@address)", ctxt);
> if (tmp) {
> if (virMacAddrParse(tmp, &def->mac) < 0) {
> @@ -2290,6 +2302,14 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> def->name);
> goto error;
> }
> + if (def->promiscLinks) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("bridge promiscLinks setting not allowed "
> + "in %s mode (network '%s')"),
> + virNetworkForwardTypeToString(def->forward.type),
> + def->name);
> + goto error;
> + }
> /* fall through to next case */
> case VIR_NETWORK_FORWARD_BRIDGE:
> if (def->delay || stp) {
> @@ -2783,22 +2803,27 @@ virNetworkDefFormatBuf(virBufferPtr buf,
> virBufferAddLit(buf, "</forward>\n");
> }
>
> +
> if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
> - def->forward.type == VIR_NETWORK_FORWARD_NAT ||
> - def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
> + def->forward.type == VIR_NETWORK_FORWARD_NAT ||
> + def->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
> + def->bridge || def->promiscLinks) {
>
> virBufferAddLit(buf, "<bridge");
> - if (def->bridge)
> - virBufferEscapeString(buf, " name='%s'", def->bridge);
> - virBufferAsprintf(buf, " stp='%s' delay='%ld'/>\n",
> - def->stp ? "on" : "off",
> - def->delay);
> - } else if (def->forward.type == VIR_NETWORK_FORWARD_BRIDGE &&
> - def->bridge) {
> - virBufferEscapeString(buf, "<bridge name='%s'/>\n", def->bridge);
> + virBufferEscapeString(buf, " name='%s'", def->bridge);
> + if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
> + def->forward.type == VIR_NETWORK_FORWARD_NAT ||
> + def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
> + virBufferAsprintf(buf, " stp='%s' delay='%ld'",
> + def->stp ? "on" : "off", def->delay);
> + }
> + if (def->promiscLinks) {
> + virBufferAsprintf(buf, " promiscLinks='%s'",
> + virTristateBoolTypeToString(def->promiscLinks));
> + }
> + virBufferAddLit(buf, "/>\n");
ahh - so much clearer!
> }
>
> -
> if (def->mac_specified) {
> char macaddr[VIR_MAC_STRING_BUFLEN];
> virMacAddrFormat(&def->mac, macaddr);
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 660cd2d..0d4aa2e 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -231,6 +231,7 @@ struct _virNetworkDef {
> int connections; /* # of guest interfaces connected to this network */
>
> char *bridge; /* Name of bridge device */
> + int promiscLinks; /* enum virTristateBool - default is YES */
> char *domain;
> unsigned long delay; /* Bridge forward delay (ms) */
> bool stp; /* Spanning tree protocol */
> diff --git a/tests/networkxml2xmlin/host-bridge-no-flood.xml b/tests/networkxml2xmlin/host-bridge-no-flood.xml
> new file mode 100644
> index 0000000..a5378c8
> --- /dev/null
> +++ b/tests/networkxml2xmlin/host-bridge-no-flood.xml
> @@ -0,0 +1,6 @@
> +<network>
> + <name>host-bridge-net</name>
> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8e</uuid>
> + <forward mode="bridge"/>
> + <bridge name="br0" promiscLinks='no'/>
> +</network>
> diff --git a/tests/networkxml2xmlin/nat-network-explicit-flood.xml b/tests/networkxml2xmlin/nat-network-explicit-flood.xml
> new file mode 100644
> index 0000000..a0f79ff
> --- /dev/null
> +++ b/tests/networkxml2xmlin/nat-network-explicit-flood.xml
> @@ -0,0 +1,21 @@
> +<network>
> + <name>default</name>
> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
> + <bridge name="virbr0" promiscLinks='yes'/>
> + <forward mode="nat" dev="eth1"/>
> + <ip address="192.168.122.1" netmask="255.255.255.0">
> + <dhcp>
> + <range start="192.168.122.2" end="192.168.122.254"/>
> + <host mac="00:16:3e:77:e2:ed" name="a.example.com" ip="192.168.122.10"/>
> + <host mac="00:16:3e:3e:a9:1a" name="b.example.com" ip="192.168.122.11"/>
> + </dhcp>
> + </ip>
> + <ip family="ipv4" address="192.168.123.1" netmask="255.255.255.0">
> + </ip>
> + <ip family="ipv6" address="2001:db8:ac10:fe01::1" prefix="64">
> + </ip>
> + <ip family="ipv6" address="2001:db8:ac10:fd01::1" prefix="64">
> + </ip>
> + <ip family="ipv4" address="10.24.10.1">
> + </ip>
> +</network>
> diff --git a/tests/networkxml2xmlout/host-bridge-no-flood.xml b/tests/networkxml2xmlout/host-bridge-no-flood.xml
> new file mode 100644
> index 0000000..f8f1ced
> --- /dev/null
> +++ b/tests/networkxml2xmlout/host-bridge-no-flood.xml
> @@ -0,0 +1,6 @@
> +<network>
> + <name>host-bridge-net</name>
> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8e</uuid>
> + <forward mode='bridge'/>
> + <bridge name='br0' promiscLinks='no'/>
> +</network>
> diff --git a/tests/networkxml2xmlout/nat-network-explicit-flood.xml b/tests/networkxml2xmlout/nat-network-explicit-flood.xml
> new file mode 100644
> index 0000000..ffca9af
> --- /dev/null
> +++ b/tests/networkxml2xmlout/nat-network-explicit-flood.xml
> @@ -0,0 +1,23 @@
> +<network>
> + <name>default</name>
> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid>
> + <forward dev='eth1' mode='nat'>
> + <interface dev='eth1'/>
> + </forward>
> + <bridge name='virbr0' stp='on' delay='0' promiscLinks='yes'/>
> + <ip address='192.168.122.1' netmask='255.255.255.0'>
> + <dhcp>
> + <range start='192.168.122.2' end='192.168.122.254'/>
> + <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10'/>
> + <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11'/>
> + </dhcp>
> + </ip>
> + <ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'>
> + </ip>
> + <ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'>
> + </ip>
> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'>
> + </ip>
> + <ip family='ipv4' address='10.24.10.1'>
> + </ip>
> +</network>
> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c
> index 65ac591..34a5211 100644
> --- a/tests/networkxml2xmltest.c
> +++ b/tests/networkxml2xmltest.c
> @@ -120,6 +120,8 @@ mymain(void)
> DO_TEST("hostdev");
> DO_TEST_FULL("hostdev-pf", VIR_NETWORK_XML_INACTIVE);
> DO_TEST("passthrough-address-crash");
> + DO_TEST("nat-network-explicit-flood");
> + DO_TEST("host-bridge-no-flood");
>
> return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list