[libvirt] [PATCH 0/3] Three new network features
The first two patches add settings that let you *disable* certain functionality (iptables rules, and the DNS server), while the third enhances the DNS <forwarder> element. Laine Stump (3): network: new network forward mode 'open' network: allow disabling dnsmasq's DNS server network: allow limiting a <forwarder> element to certain domains docs/formatnetwork.html.in | 62 ++++++- docs/schemas/network.rng | 14 +- src/conf/network_conf.c | 110 +++++++++-- src/conf/network_conf.h | 10 +- src/network/bridge_driver.c | 204 +++++++++++++-------- .../nat-network-dns-forwarders.conf | 2 + .../nat-network-dns-forwarders.xml | 2 + tests/networkxml2confdata/open-network.conf | 11 ++ tests/networkxml2confdata/open-network.xml | 9 + .../networkxml2confdata/routed-network-no-dns.conf | 11 ++ .../networkxml2confdata/routed-network-no-dns.xml | 10 + tests/networkxml2conftest.c | 2 + .../nat-network-dns-forwarders.xml | 6 +- .../open-network-with-forward-dev.xml | 9 + tests/networkxml2xmlin/open-network.xml | 9 + tests/networkxml2xmlin/routed-network-no-dns.xml | 10 + .../nat-network-dns-forwarders.xml | 2 + tests/networkxml2xmlout/open-network.xml | 9 + tests/networkxml2xmlout/routed-network-no-dns.xml | 12 ++ tests/networkxml2xmltest.c | 3 + 20 files changed, 405 insertions(+), 102 deletions(-) create mode 100644 tests/networkxml2confdata/open-network.conf create mode 100644 tests/networkxml2confdata/open-network.xml create mode 100644 tests/networkxml2confdata/routed-network-no-dns.conf create mode 100644 tests/networkxml2confdata/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlin/open-network-with-forward-dev.xml create mode 100644 tests/networkxml2xmlin/open-network.xml create mode 100644 tests/networkxml2xmlin/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlout/open-network.xml create mode 100644 tests/networkxml2xmlout/routed-network-no-dns.xml -- 2.7.4
The new forward mode 'open' is just like mode='route', except that no firewall rules are added to assure that any traffic does or doesn't pass. It is assumed that either they aren't necessary, or they will be setup outside the scope of libvirt. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=846810 --- docs/formatnetwork.html.in | 22 ++++++++++++ docs/schemas/network.rng | 1 + src/conf/network_conf.c | 25 +++++++++++-- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 41 +++++++++++++++------- tests/networkxml2confdata/open-network.conf | 11 ++++++ tests/networkxml2confdata/open-network.xml | 9 +++++ tests/networkxml2conftest.c | 1 + .../open-network-with-forward-dev.xml | 9 +++++ tests/networkxml2xmlin/open-network.xml | 9 +++++ tests/networkxml2xmlout/open-network.xml | 9 +++++ tests/networkxml2xmltest.c | 2 ++ 12 files changed, 125 insertions(+), 15 deletions(-) create mode 100644 tests/networkxml2confdata/open-network.conf create mode 100644 tests/networkxml2confdata/open-network.xml create mode 100644 tests/networkxml2xmlin/open-network-with-forward-dev.xml create mode 100644 tests/networkxml2xmlin/open-network.xml create mode 100644 tests/networkxml2xmlout/open-network.xml diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index a9226e5..12d1bed 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -260,6 +260,28 @@ <span class="since">Since 0.4.2</span> </dd> + <dt><code>open</code></dt> + <dd> + As with mode='route', guest network traffic will be + forwarded to the physical network via the host's IP + routing stack, but there will be no firewall rules added + to either enable or prevent any of this traffic. When + forward='open' is set, the <code>dev</code> attribute + cannot be set (because the forward dev is enforced with + firewall rules, and the purpose of forward='open' is to + have a forwarding mode where libvirt doesn't add any + firewall rules). This mode presumes that the local LAN + router has suitable routing table entries to return + traffic to this host, and that some other management + system has been used to put in place any necessary + firewall rules. Although no firewall rules will be added + for the network, it is of course still possible to add + restrictions for specific guests using + <a href="formatnwfilter.html">nwfilter rules</a> on the + guests' interfaces.) + <span class="since">Since 2.2.0</span> + </dd> + <dt><code>bridge</code></dt> <dd> This network describes either 1) an existing host bridge diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index c2c51ae..621f16e 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -103,6 +103,7 @@ <choice> <value>nat</value> <value>route</value> + <value>open</value> <value>bridge</value> <value>passthrough</value> <value>private</value> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a75ca71..6820bde 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -57,7 +57,9 @@ struct _virNetworkObjList { VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, - "none", "nat", "route", "bridge", "private", "vepa", "passthrough", "hostdev") + "none", "nat", "route", "open", + "bridge", "private", "vepa", "passthrough", + "hostdev") VIR_ENUM_IMPL(virNetworkBridgeMACTableManager, VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LAST, @@ -2333,6 +2335,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) case VIR_NETWORK_FORWARD_ROUTE: case VIR_NETWORK_FORWARD_NAT: + case VIR_NETWORK_FORWARD_OPEN: /* It's pointless to specify L3 forwarding without specifying * the network we're on. */ @@ -2351,6 +2354,19 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->name); goto error; } + + if (def->forward.type == VIR_NETWORK_FORWARD_OPEN && def->forward.nifs) { + /* an open network by definition can't place any restrictions + * on what traffic is allowed or where it goes, so specifying + * a forwarding device is nonsensical. + */ + virReportError(VIR_ERR_XML_ERROR, + _("forward dev not allowed for " + "network '%s' with forward mode='%s'"), + def->name, + virNetworkForwardTypeToString(def->forward.type)); + goto error; + } break; case VIR_NETWORK_FORWARD_PRIVATE: @@ -2856,13 +2872,15 @@ virNetworkDefFormatBuf(virBufferPtr buf, 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_OPEN || def->bridge || def->macTableManager) { virBufferAddLit(buf, "<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) { + def->forward.type == VIR_NETWORK_FORWARD_ROUTE || + def->forward.type == VIR_NETWORK_FORWARD_OPEN) { virBufferAsprintf(buf, " stp='%s' delay='%ld'", def->stp ? "on" : "off", def->delay); } @@ -3235,7 +3253,8 @@ virNetworkObjPtr virNetworkLoadConfig(virNetworkObjListPtr nets, 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_ROUTE || + def->forward.type == VIR_NETWORK_FORWARD_OPEN) { if (!def->mac_specified) { virNetworkSetBridgeMacAddr(def); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 7814889..1ce4257 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -46,6 +46,7 @@ typedef enum { VIR_NETWORK_FORWARD_NONE = 0, VIR_NETWORK_FORWARD_NAT, VIR_NETWORK_FORWARD_ROUTE, + VIR_NETWORK_FORWARD_OPEN, VIR_NETWORK_FORWARD_BRIDGE, VIR_NETWORK_FORWARD_PRIVATE, VIR_NETWORK_FORWARD_VEPA, diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 87019cb..23036e8 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -400,6 +400,7 @@ networkUpdateState(virNetworkObjPtr obj, case VIR_NETWORK_FORWARD_NONE: case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: + case VIR_NETWORK_FORWARD_OPEN: /* If bridge doesn't exist, then mark it inactive */ if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1)) obj->active = 0; @@ -1822,7 +1823,8 @@ networkRefreshDaemonsHelper(virNetworkObjPtr net, if (virNetworkObjIsActive(net) && ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || - (net->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { + (net->def->forward.type == VIR_NETWORK_FORWARD_ROUTE) || + (net->def->forward.type == VIR_NETWORK_FORWARD_OPEN))) { /* Only the three L3 network types that are configured by * libvirt will have a dnsmasq or radvd daemon associated * with them. Here we send a SIGHUP to an existing @@ -1858,8 +1860,10 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr net, ((net->def->forward.type == VIR_NETWORK_FORWARD_NONE) || (net->def->forward.type == VIR_NETWORK_FORWARD_NAT) || (net->def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) { - /* Only the three L3 network types that are configured by libvirt - * need to have iptables rules reloaded. + /* Only three of the L3 network types that are configured by + * libvirt need to have iptables rules reloaded. The 4th L3 + * network type, forward='open', doesn't need this because it + * has no iptables rules. */ networkRemoveFirewallRules(net->def); if (networkAddFirewallRules(net->def) < 0) { @@ -2142,7 +2146,8 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, goto err1; /* Add "once per network" rules */ - if (networkAddFirewallRules(network->def) < 0) + if (network->def->forward.type != VIR_NETWORK_FORWARD_OPEN && + networkAddFirewallRules(network->def) < 0) goto err1; for (i = 0; @@ -2244,7 +2249,8 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, err2: if (!save_err) save_err = virSaveLastError(); - networkRemoveFirewallRules(network->def); + if (network->def->forward.type != VIR_NETWORK_FORWARD_OPEN) + networkRemoveFirewallRules(network->def); err1: if (!save_err) @@ -2300,7 +2306,8 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver, ignore_value(virNetDevSetOnline(network->def->bridge, 0)); - networkRemoveFirewallRules(network->def); + if (network->def->forward.type != VIR_NETWORK_FORWARD_OPEN) + networkRemoveFirewallRules(network->def); ignore_value(virNetDevBridgeDelete(network->def->bridge)); @@ -2407,6 +2414,7 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) case VIR_NETWORK_FORWARD_NONE: case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: + case VIR_NETWORK_FORWARD_OPEN: case VIR_NETWORK_FORWARD_LAST: /* by definition these will never be encountered here */ break; @@ -2500,6 +2508,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_NONE: case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: + case VIR_NETWORK_FORWARD_OPEN: if (networkStartNetworkVirtual(driver, network) < 0) goto cleanup; break; @@ -2578,6 +2587,7 @@ networkShutdownNetwork(virNetworkDriverStatePtr driver, case VIR_NETWORK_FORWARD_NONE: case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: + case VIR_NETWORK_FORWARD_OPEN: ret = networkShutdownNetworkVirtual(driver, network); break; @@ -2926,7 +2936,8 @@ networkValidate(virNetworkDriverStatePtr driver, */ 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_ROUTE || + def->forward.type == VIR_NETWORK_FORWARD_OPEN) { /* if no bridge name was given in the config, find a name * unused by any other libvirt networks and assign it. @@ -3367,8 +3378,10 @@ networkUpdate(virNetworkPtr net, * old rules (and remember to load new ones after the * update). */ - networkRemoveFirewallRules(network->def); - needFirewallRefresh = true; + if (network->def->forward.type != VIR_NETWORK_FORWARD_OPEN) { + networkRemoveFirewallRules(network->def); + needFirewallRefresh = true; + } break; default: break; @@ -4050,7 +4063,8 @@ networkAllocateActualDevice(virDomainDefPtr dom, if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) || (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) || - (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE)) { + (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE) || + (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN)) { /* for these forward types, the actual net type really *is* * NETWORK; we just keep the info from the portgroup in * iface->data.network.actual @@ -4594,7 +4608,8 @@ networkReleaseActualDevice(virDomainDefPtr dom, if (iface->data.network.actual && (netdef->forward.type == VIR_NETWORK_FORWARD_NONE || netdef->forward.type == VIR_NETWORK_FORWARD_NAT || - netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE) && + netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE || + netdef->forward.type == VIR_NETWORK_FORWARD_OPEN) && networkUnplugBandwidth(network, iface) < 0) goto error; @@ -4741,6 +4756,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr) case VIR_NETWORK_FORWARD_NONE: case VIR_NETWORK_FORWARD_NAT: case VIR_NETWORK_FORWARD_ROUTE: + case VIR_NETWORK_FORWARD_OPEN: ipdef = virNetworkDefGetIPByIndex(netdef, AF_UNSPEC, 0); if (!ipdef) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -4824,7 +4840,8 @@ networkGetActualType(virDomainNetDefPtr iface) if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) || (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) || - (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE)) { + (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE) || + (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN)) { /* for these forward types, the actual net type really *is* * NETWORK; we just keep the info from the portgroup in * iface->data.network.actual diff --git a/tests/networkxml2confdata/open-network.conf b/tests/networkxml2confdata/open-network.conf new file mode 100644 index 0000000..ff09984 --- /dev/null +++ b/tests/networkxml2confdata/open-network.conf @@ -0,0 +1,11 @@ +##WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE +##OVERWRITTEN AND LOST. Changes to this configuration should be made using: +## virsh net-edit open +## or other application using the libvirt API. +## +## dnsmasq conf file created by libvirt +strict-order +except-interface=lo +bind-dynamic +interface=virbr1 +addn-hosts=/var/lib/libvirt/dnsmasq/open.addnhosts diff --git a/tests/networkxml2confdata/open-network.xml b/tests/networkxml2confdata/open-network.xml new file mode 100644 index 0000000..e0b3f03 --- /dev/null +++ b/tests/networkxml2confdata/open-network.xml @@ -0,0 +1,9 @@ +<network> + <name>open</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward mode='open'/> + <bridge name='virbr1' stp='on' delay='0'/> + <mac address='12:34:56:78:9A:BC'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + </ip> +</network> diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 11e08c0..77acc53 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -117,6 +117,7 @@ mymain(void) DO_TEST("nat-network-dns-srv-record-minimal", restricted); DO_TEST("nat-network-name-with-quotes", restricted); DO_TEST("routed-network", full); + DO_TEST("open-network", full); DO_TEST("nat-network", dhcpv6); DO_TEST("nat-network-dns-txt-record", full); DO_TEST("nat-network-dns-srv-record", full); diff --git a/tests/networkxml2xmlin/open-network-with-forward-dev.xml b/tests/networkxml2xmlin/open-network-with-forward-dev.xml new file mode 100644 index 0000000..33e8bb5 --- /dev/null +++ b/tests/networkxml2xmlin/open-network-with-forward-dev.xml @@ -0,0 +1,9 @@ +<network> + <name>open</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr1"/> + <mac address='12:34:56:78:9A:BC'/> + <forward mode="open" dev="eth0"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + </ip> +</network> diff --git a/tests/networkxml2xmlin/open-network.xml b/tests/networkxml2xmlin/open-network.xml new file mode 100644 index 0000000..2e4b9b2 --- /dev/null +++ b/tests/networkxml2xmlin/open-network.xml @@ -0,0 +1,9 @@ +<network> + <name>open</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr1"/> + <mac address='12:34:56:78:9A:BC'/> + <forward mode="open"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + </ip> +</network> diff --git a/tests/networkxml2xmlout/open-network.xml b/tests/networkxml2xmlout/open-network.xml new file mode 100644 index 0000000..29e9684 --- /dev/null +++ b/tests/networkxml2xmlout/open-network.xml @@ -0,0 +1,9 @@ +<network> + <name>open</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward mode='open'/> + <bridge name='virbr1' stp='on' delay='0'/> + <mac address='12:34:56:78:9a:bc'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + </ip> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 2a2c348..32544d0 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -127,6 +127,8 @@ mymain(void) DO_TEST("empty-allow-ipv6"); DO_TEST("isolated-network"); DO_TEST("routed-network"); + DO_TEST("open-network"); + DO_TEST_PARSE_ERROR("open-network-with-forward-dev"); DO_TEST("nat-network"); DO_TEST("netboot-network"); DO_TEST("netboot-proxy-network"); -- 2.7.4
On Thu, Aug 11, 2016 at 10:41:45PM -0400, Laine Stump wrote:
The new forward mode 'open' is just like mode='route', except that no firewall rules are added to assure that any traffic does or doesn't pass. It is assumed that either they aren't necessary, or they will be setup outside the scope of libvirt.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=846810 --- docs/formatnetwork.html.in | 22 ++++++++++++ docs/schemas/network.rng | 1 + src/conf/network_conf.c | 25 +++++++++++-- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 41 +++++++++++++++------- tests/networkxml2confdata/open-network.conf | 11 ++++++ tests/networkxml2confdata/open-network.xml | 9 +++++ tests/networkxml2conftest.c | 1 + .../open-network-with-forward-dev.xml | 9 +++++ tests/networkxml2xmlin/open-network.xml | 9 +++++ tests/networkxml2xmlout/open-network.xml | 9 +++++ tests/networkxml2xmltest.c | 2 ++ 12 files changed, 125 insertions(+), 15 deletions(-) create mode 100644 tests/networkxml2confdata/open-network.conf create mode 100644 tests/networkxml2confdata/open-network.xml create mode 100644 tests/networkxml2xmlin/open-network-with-forward-dev.xml create mode 100644 tests/networkxml2xmlin/open-network.xml create mode 100644 tests/networkxml2xmlout/open-network.xml
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index a9226e5..12d1bed 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -260,6 +260,28 @@ <span class="since">Since 0.4.2</span> </dd>
+ <dt><code>open</code></dt> + <dd> + As with mode='route', guest network traffic will be + forwarded to the physical network via the host's IP + routing stack, but there will be no firewall rules added + to either enable or prevent any of this traffic. When + forward='open' is set, the <code>dev</code> attribute + cannot be set (because the forward dev is enforced with + firewall rules, and the purpose of forward='open' is to + have a forwarding mode where libvirt doesn't add any + firewall rules). This mode presumes that the local LAN + router has suitable routing table entries to return + traffic to this host, and that some other management + system has been used to put in place any necessary + firewall rules. Although no firewall rules will be added + for the network, it is of course still possible to add + restrictions for specific guests using + <a href="formatnwfilter.html">nwfilter rules</a> on the + guests' interfaces.) + <span class="since">Since 2.2.0</span> + </dd> +
Isn't this basically the same as forward mode="bridge", except that we still create the bridge ourselves, instead of requiring it to be pre-created ? If so, I wonder if its better add a attribute 'create=yes|no' to the <bridge> element instead ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 08/12/2016 03:52 AM, Daniel P. Berrange wrote:
The new forward mode 'open' is just like mode='route', except that no firewall rules are added to assure that any traffic does or doesn't pass. It is assumed that either they aren't necessary, or they will be setup outside the scope of libvirt.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=846810 --- docs/formatnetwork.html.in | 22 ++++++++++++ docs/schemas/network.rng | 1 + src/conf/network_conf.c | 25 +++++++++++-- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 41 +++++++++++++++------- tests/networkxml2confdata/open-network.conf | 11 ++++++ tests/networkxml2confdata/open-network.xml | 9 +++++ tests/networkxml2conftest.c | 1 + .../open-network-with-forward-dev.xml | 9 +++++ tests/networkxml2xmlin/open-network.xml | 9 +++++ tests/networkxml2xmlout/open-network.xml | 9 +++++ tests/networkxml2xmltest.c | 2 ++ 12 files changed, 125 insertions(+), 15 deletions(-) create mode 100644 tests/networkxml2confdata/open-network.conf create mode 100644 tests/networkxml2confdata/open-network.xml create mode 100644 tests/networkxml2xmlin/open-network-with-forward-dev.xml create mode 100644 tests/networkxml2xmlin/open-network.xml create mode 100644 tests/networkxml2xmlout/open-network.xml
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index a9226e5..12d1bed 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -260,6 +260,28 @@ <span class="since">Since 0.4.2</span> </dd>
+ <dt><code>open</code></dt> + <dd> + As with mode='route', guest network traffic will be + forwarded to the physical network via the host's IP + routing stack, but there will be no firewall rules added + to either enable or prevent any of this traffic. When + forward='open' is set, the <code>dev</code> attribute + cannot be set (because the forward dev is enforced with + firewall rules, and the purpose of forward='open' is to + have a forwarding mode where libvirt doesn't add any + firewall rules). This mode presumes that the local LAN + router has suitable routing table entries to return + traffic to this host, and that some other management + system has been used to put in place any necessary + firewall rules. Although no firewall rules will be added + for the network, it is of course still possible to add + restrictions for specific guests using + <a href="formatnwfilter.html">nwfilter rules</a> on the + guests' interfaces.) + <span class="since">Since 2.2.0</span> + </dd> + Isn't this basically the same as forward mode="bridge", except that we still create the bridge ourselves, instead of requiring it to be
On Thu, Aug 11, 2016 at 10:41:45PM -0400, Laine Stump wrote: pre-created ?
Sigh. If only that was the case :-/ (Seriously, I wish we had the last 5 years' experiences as a guide when we discussed the addition of forward mode='bridge' (and vepa and private) back in 2011. <forward mode='bridge'> can mean one of two types of connections, depending on a couple other settings: 1) if there is a <bridge> element giving a device name, then guests are connected to the named bridge (which must already exist) with a tap device. No dnsmasq process is started, and no iptables rules are added. <ip> elements are forbidden. 2) if there is no <bridge> element, then guests are connected to the physical device named in <forward dev='xyz'/> using macvtap bridge mode. Again, no dnsmasq, no iptables, and <ip> elements are forbidden. At the time I'd suggested adding a higher level "type" attribute to the network to support these very different types, but was discouraged from that because existing management applications would be confused by these new networks that appeared to be identical to existing networks (because the management app was unaware of the newly added "type" attribute). The forward mode was chosen because it was an existing attribute that all management apps already checked; the idea was that at the very least they would see an unrecognized value and claim ignorance, and at best they might display the bridge name or forwarding device name, giving some sort of clue about the network. (This morning I had been feeling guilty about creating such a complicated mess, but then I looked back at the design discussions on the mailing list (it went through several versions before I wrote any code) and see that it really was a group fail.. er, effort :-P.)
If so, I wonder if its better add a attribute 'create=yes|no' to the <bridge> element instead ?
There would still be the issue of dns/dhcp servers - default (and only) behavior for mode='bridge' is to never set these up, but the aim of this patch is provide a way to turn off iptables rules without affecting anything else. (Hmm, but if no IP addresses are given, which is *always* the case for the existing bridge modes, then it's not possible to create a dns server anyway) Also there is the problem that a network with mode='nat|route' auto-generates a bridge device name if none is given, but mode='bridge' with no bridge name given is taken to mean "use macvtap bridge mode". Of course that mode requires that there be a forward dev manually specified, so maybe a further complication of the rules for identification could do it... Let's see... If there is <forward mode='bridge'> and: 1) if there are any <ip> elements in the forward: then we assume that it is a libvirt-managed network (i.e. exactly what this patch does for <forward mode='open'/> - bridge device created/destroyed, dnsmasq run as needed, but no iptables rules)\> 2) if no <ip> elements, and <bridge create='yes'> (the first time it's started bridge name may or may not be specified) same as 1 (except that there couldn't be any dnsmasq due to lack of <ip>.) 3) if no <ip> elements, and <bridge name='xyz'> is defined: then it uses an existing bridge, no iptables, no dnsmasq (what is currently done) 4) <forward mode='bridge' dev='xyz'/>, no <bridge> element: macvtap bridge mode Okay. I think that is unambiguous (although a bit complicated, but it already was complicated anyway). Unless my description makes you nauseous, I'll redo the patch that way and see how ugly it is. Two questions though: 1) Currently if someone has <forward mode='bridge'>, <bridge name='xyz'/>, and tries to add any <ip> elements, that is an error; should we continue to make this an error unless they also specify <bridge create='yes'>? Or should we assume/automatically add create='yes' in that case? 2) should we begin automatically adding "create='yes'" to the <bridge> element of <forward mode='nat|route'> networks? Or leave them as-is? (internally, it would be nice to have the create attribute there, as it makes a lot of decisions easier to code; you could argue it's redundant in almost all cases though)
On Fri, Aug 12, 2016 at 11:19:00AM -0400, Laine Stump wrote:
On 08/12/2016 03:52 AM, Daniel P. Berrange wrote:
The new forward mode 'open' is just like mode='route', except that no firewall rules are added to assure that any traffic does or doesn't pass. It is assumed that either they aren't necessary, or they will be setup outside the scope of libvirt.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=846810 --- docs/formatnetwork.html.in | 22 ++++++++++++ docs/schemas/network.rng | 1 + src/conf/network_conf.c | 25 +++++++++++-- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 41 +++++++++++++++------- tests/networkxml2confdata/open-network.conf | 11 ++++++ tests/networkxml2confdata/open-network.xml | 9 +++++ tests/networkxml2conftest.c | 1 + .../open-network-with-forward-dev.xml | 9 +++++ tests/networkxml2xmlin/open-network.xml | 9 +++++ tests/networkxml2xmlout/open-network.xml | 9 +++++ tests/networkxml2xmltest.c | 2 ++ 12 files changed, 125 insertions(+), 15 deletions(-) create mode 100644 tests/networkxml2confdata/open-network.conf create mode 100644 tests/networkxml2confdata/open-network.xml create mode 100644 tests/networkxml2xmlin/open-network-with-forward-dev.xml create mode 100644 tests/networkxml2xmlin/open-network.xml create mode 100644 tests/networkxml2xmlout/open-network.xml
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index a9226e5..12d1bed 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -260,6 +260,28 @@ <span class="since">Since 0.4.2</span> </dd> + <dt><code>open</code></dt> + <dd> + As with mode='route', guest network traffic will be + forwarded to the physical network via the host's IP + routing stack, but there will be no firewall rules added + to either enable or prevent any of this traffic. When + forward='open' is set, the <code>dev</code> attribute + cannot be set (because the forward dev is enforced with + firewall rules, and the purpose of forward='open' is to + have a forwarding mode where libvirt doesn't add any + firewall rules). This mode presumes that the local LAN + router has suitable routing table entries to return + traffic to this host, and that some other management + system has been used to put in place any necessary + firewall rules. Although no firewall rules will be added + for the network, it is of course still possible to add + restrictions for specific guests using + <a href="formatnwfilter.html">nwfilter rules</a> on the + guests' interfaces.) + <span class="since">Since 2.2.0</span> + </dd> + Isn't this basically the same as forward mode="bridge", except that we still create the bridge ourselves, instead of requiring it to be
On Thu, Aug 11, 2016 at 10:41:45PM -0400, Laine Stump wrote: pre-created ?
Sigh. If only that was the case :-/
[snip]
If so, I wonder if its better add a attribute 'create=yes|no' to the <bridge> element instead ?
ok, ignore my suggestion. There's nothing wrong with what you've proposed. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12.08.2016 04:41, Laine Stump wrote:
The new forward mode 'open' is just like mode='route', except that no firewall rules are added to assure that any traffic does or doesn't pass. It is assumed that either they aren't necessary, or they will be setup outside the scope of libvirt.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=846810 --- docs/formatnetwork.html.in | 22 ++++++++++++ docs/schemas/network.rng | 1 + src/conf/network_conf.c | 25 +++++++++++-- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 41 +++++++++++++++------- tests/networkxml2confdata/open-network.conf | 11 ++++++ tests/networkxml2confdata/open-network.xml | 9 +++++ tests/networkxml2conftest.c | 1 + .../open-network-with-forward-dev.xml | 9 +++++ tests/networkxml2xmlin/open-network.xml | 9 +++++ tests/networkxml2xmlout/open-network.xml | 9 +++++ tests/networkxml2xmltest.c | 2 ++ 12 files changed, 125 insertions(+), 15 deletions(-) create mode 100644 tests/networkxml2confdata/open-network.conf create mode 100644 tests/networkxml2confdata/open-network.xml create mode 100644 tests/networkxml2xmlin/open-network-with-forward-dev.xml create mode 100644 tests/networkxml2xmlin/open-network.xml create mode 100644 tests/networkxml2xmlout/open-network.xml
Okay, looks like we have an agreement on the XML then. The code is solid. ACK Michal
If you define a libvirt virtual network with one or more IP addresses, it starts up an instance of dnsmasq. It's always been possible to avoid dnsmasq's dhcp server (simply don't include a <dhcp> element), but until now it wasn't possible to avoid having the DNS server listening; even if the network has no <dns> element, it is started using default settings. This patch adds a new attribute to <dns>: enable='yes|no'. For backward compatibility, it defaults to 'yes', but if you don't want a DNS server created for the network, you can simply add: <dns enable='no'/> to the network configuration, and next time the network is started there will be no dns server created (if there is dhcp configuration, dnsmasq will be started with "port=0" which disables the DNS server; if there is no dhcp configuration, dnsmasq won't be started at all). --- docs/formatnetwork.html.in | 12 ++ docs/schemas/network.rng | 5 + src/conf/network_conf.c | 36 ++++- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 146 ++++++++++++--------- .../networkxml2confdata/routed-network-no-dns.conf | 11 ++ .../networkxml2confdata/routed-network-no-dns.xml | 10 ++ tests/networkxml2conftest.c | 1 + tests/networkxml2xmlin/routed-network-no-dns.xml | 10 ++ tests/networkxml2xmlout/routed-network-no-dns.xml | 12 ++ tests/networkxml2xmltest.c | 1 + 11 files changed, 179 insertions(+), 66 deletions(-) create mode 100644 tests/networkxml2confdata/routed-network-no-dns.conf create mode 100644 tests/networkxml2confdata/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlin/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlout/routed-network-no-dns.xml diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 12d1bed..e103dd7 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -886,6 +886,18 @@ server <span class="since">Since 0.9.3</span>. <p> + The dns element can have an optional <code>enable</code> + attribute <span class="since">Since 2.2.0</span>. + If <code>enable</code> is "no", then no DNS server will be + setup by libvirt for this network (and any other + configuration in <code><dns></code> will be ignored). + If <code>enable</code> is "yes" or unspecified (including + the complete absence of any <code><dns></code> + element) then a DNS server will be setup by libvirt to + listen on all IP addresses specified in the network's + configuration. + </p> + <p> The dns element can have an optional <code>forwardPlainNames</code> attribute <span class="since">Since 1.1.2</span>. diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 621f16e..12d4b34 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -248,6 +248,11 @@ <optional> <element name="dns"> <optional> + <attribute name="enable"> + <ref name="virYesNo"/> + </attribute> + </optional> + <optional> <attribute name="forwardPlainNames"> <ref name="virYesNo"/> </attribute> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6820bde..490574f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1335,6 +1335,7 @@ virNetworkDNSDefParseXML(const char *networkName, xmlNodePtr *txtNodes = NULL; xmlNodePtr *fwdNodes = NULL; char *forwardPlainNames = NULL; + char *enable = NULL; int nhosts, nsrvs, ntxts, nfwds; size_t i; int ret = -1; @@ -1342,6 +1343,18 @@ virNetworkDNSDefParseXML(const char *networkName, ctxt->node = node; + enable = virXPathString("string(./@enable)", ctxt); + if (enable) { + def->enable = virTristateBoolTypeFromString(enable); + if (def->enable <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid dns enable setting '%s' " + "in network '%s'"), + enable, networkName); + goto cleanup; + } + } + forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt); if (forwardPlainNames) { def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames); @@ -1440,6 +1453,7 @@ virNetworkDNSDefParseXML(const char *networkName, ret = 0; cleanup: + VIR_FREE(enable); VIR_FREE(forwardPlainNames); VIR_FREE(fwdNodes); VIR_FREE(hostNodes); @@ -2496,12 +2510,22 @@ virNetworkDNSDefFormat(virBufferPtr buf, { size_t i, j; - if (!(def->forwardPlainNames || def->nfwds || def->nhosts || + if (!(def->enable || def->forwardPlainNames || def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) return 0; virBufferAddLit(buf, "<dns"); - /* default to "yes", but don't format it in the XML */ + if (def->enable) { + const char *fwd = virTristateBoolTypeToString(def->enable); + + if (!fwd) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown enable type %d in network"), + def->enable); + return -1; + } + virBufferAsprintf(buf, " enable='%s'", fwd); + } if (def->forwardPlainNames) { const char *fwd = virTristateBoolTypeToString(def->forwardPlainNames); @@ -2512,10 +2536,10 @@ virNetworkDNSDefFormat(virBufferPtr buf, return -1; } virBufferAsprintf(buf, " forwardPlainNames='%s'", fwd); - if (!(def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) { - virBufferAddLit(buf, "/>\n"); - return 0; - } + } + if (!(def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) { + virBufferAddLit(buf, "/>\n"); + return 0; } virBufferAddLit(buf, ">\n"); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 1ce4257..9ebd4a7 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -128,6 +128,7 @@ struct _virNetworkDNSHostDef { typedef struct _virNetworkDNSDef virNetworkDNSDef; typedef virNetworkDNSDef *virNetworkDNSDefPtr; struct _virNetworkDNSDef { + int enable; /* enum virTristateBool */ int forwardPlainNames; /* enum virTristateBool */ size_t ntxts; virNetworkDNSTxtDefPtr txts; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 23036e8..49c0a2f 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -916,6 +916,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network, int nbleases = 0; size_t i; virNetworkDNSDefPtr dns = &network->def->dns; + bool wantDNS = dns->enable != VIR_TRISTATE_BOOL_NO; virNetworkIPDefPtr tmpipdef, ipdef, ipv4def, ipv6def; bool ipv6SLAAC; char *saddr = NULL, *eaddr = NULL; @@ -948,7 +949,13 @@ networkDnsmasqConfContents(virNetworkObjPtr network, "strict-order\n", network->def->name); - if (network->def->dns.forwarders) { + /* if dns is disabled, set its listening port to 0, which + * tells dnsmasq to not listen + */ + if (!wantDNS) + virBufferAddLit(&configbuf, "port=0\n"); + + if (wantDNS && network->def->dns.forwarders) { virBufferAddLit(&configbuf, "no-resolv\n"); for (i = 0; i < network->def->dns.nfwds; i++) { virBufferAsprintf(&configbuf, "server=%s\n", @@ -968,7 +975,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network, network->def->domain); } - if (network->def->dns.forwardPlainNames == VIR_TRISTATE_BOOL_NO) { + if (wantDNS && network->def->dns.forwardPlainNames == VIR_TRISTATE_BOOL_NO) { virBufferAddLit(&configbuf, "domain-needed\n"); /* need to specify local=// whether or not a domain is * specified, unless the config says we should forward "plain" @@ -1061,64 +1068,66 @@ networkDnsmasqConfContents(virNetworkObjPtr network, } } - for (i = 0; i < dns->ntxts; i++) { - virBufferAsprintf(&configbuf, "txt-record=%s,%s\n", - dns->txts[i].name, - dns->txts[i].value); - } - - for (i = 0; i < dns->nsrvs; i++) { - /* service/protocol are required, and should have been validated - * by the parser. - */ - if (!dns->srvs[i].service) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing required 'service' " - "attribute in SRV record of network '%s'"), - network->def->name); - goto cleanup; + if (wantDNS) { + for (i = 0; i < dns->ntxts; i++) { + virBufferAsprintf(&configbuf, "txt-record=%s,%s\n", + dns->txts[i].name, + dns->txts[i].value); } - if (!dns->srvs[i].protocol) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing required 'service' " - "attribute in SRV record of network '%s'"), - network->def->name); - goto cleanup; - } - /* RFC2782 requires that service and protocol be preceded by - * an underscore. - */ - virBufferAsprintf(&configbuf, "srv-host=_%s._%s", - dns->srvs[i].service, dns->srvs[i].protocol); - /* domain is optional - it defaults to the domain of this network */ - if (dns->srvs[i].domain) - virBufferAsprintf(&configbuf, ".%s", dns->srvs[i].domain); + for (i = 0; i < dns->nsrvs; i++) { + /* service/protocol are required, and should have been validated + * by the parser. + */ + if (!dns->srvs[i].service) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing required 'service' " + "attribute in SRV record of network '%s'"), + network->def->name); + goto cleanup; + } + if (!dns->srvs[i].protocol) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Missing required 'service' " + "attribute in SRV record of network '%s'"), + network->def->name); + goto cleanup; + } + /* RFC2782 requires that service and protocol be preceded by + * an underscore. + */ + virBufferAsprintf(&configbuf, "srv-host=_%s._%s", + dns->srvs[i].service, dns->srvs[i].protocol); - /* If target is empty or ".", that means "the service is - * decidedly not available at this domain" (RFC2782). In that - * case, any port, priority, or weight is irrelevant. - */ - if (dns->srvs[i].target && STRNEQ(dns->srvs[i].target, ".")) { - - virBufferAsprintf(&configbuf, ",%s", dns->srvs[i].target); - /* port, priority, and weight are optional, but are - * identified by their position in the line. If an item is - * unspecified, but something later in the line *is* - * specified, we need to give the default value for the - * unspecified item. (According to the dnsmasq manpage, - * the default for port is 1). + /* domain is optional - it defaults to the domain of this network */ + if (dns->srvs[i].domain) + virBufferAsprintf(&configbuf, ".%s", dns->srvs[i].domain); + + /* If target is empty or ".", that means "the service is + * decidedly not available at this domain" (RFC2782). In that + * case, any port, priority, or weight is irrelevant. */ - if (dns->srvs[i].port || - dns->srvs[i].priority || dns->srvs[i].weight) - virBufferAsprintf(&configbuf, ",%d", - dns->srvs[i].port ? dns->srvs[i].port : 1); - if (dns->srvs[i].priority || dns->srvs[i].weight) - virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].priority); - if (dns->srvs[i].weight) - virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].weight); + if (dns->srvs[i].target && STRNEQ(dns->srvs[i].target, ".")) { + + virBufferAsprintf(&configbuf, ",%s", dns->srvs[i].target); + /* port, priority, and weight are optional, but are + * identified by their position in the line. If an item is + * unspecified, but something later in the line *is* + * specified, we need to give the default value for the + * unspecified item. (According to the dnsmasq manpage, + * the default for port is 1). + */ + if (dns->srvs[i].port || + dns->srvs[i].priority || dns->srvs[i].weight) + virBufferAsprintf(&configbuf, ",%d", + dns->srvs[i].port ? dns->srvs[i].port : 1); + if (dns->srvs[i].priority || dns->srvs[i].weight) + virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].priority); + if (dns->srvs[i].weight) + virBufferAsprintf(&configbuf, ",%d", dns->srvs[i].weight); + } + virBufferAddLit(&configbuf, "\n"); } - virBufferAddLit(&configbuf, "\n"); } /* Find the first dhcp for both IPv4 and IPv6 */ @@ -1198,7 +1207,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network, virBufferAsprintf(&configbuf, "dhcp-range=%s,%s", saddr, eaddr); if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) - virBufferAsprintf(&configbuf, ",%d", prefix); + virBufferAsprintf(&configbuf, ",%d", prefix); virBufferAddLit(&configbuf, "\n"); VIR_FREE(saddr); @@ -1225,7 +1234,7 @@ networkDnsmasqConfContents(virNetworkObjPtr network, virBufferAsprintf(&configbuf, "dhcp-range=%s,static", bridgeaddr); if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) - virBufferAsprintf(&configbuf, ",%d", prefix); + virBufferAsprintf(&configbuf, ",%d", prefix); virBufferAddLit(&configbuf, "\n"); VIR_FREE(bridgeaddr); } @@ -1278,8 +1287,10 @@ networkDnsmasqConfContents(virNetworkObjPtr network, /* Likewise, always create this file and put it on the * commandline, to allow for runtime additions. */ - virBufferAsprintf(&configbuf, "addn-hosts=%s\n", - dctx->addnhostsfile->path); + if (wantDNS) { + virBufferAsprintf(&configbuf, "addn-hosts=%s\n", + dctx->addnhostsfile->path); + } /* Are we doing RA instead of radvd? */ if (DNSMASQ_RA_SUPPORT(caps)) { @@ -1375,17 +1386,32 @@ static int networkStartDhcpDaemon(virNetworkDriverStatePtr driver, virNetworkObjPtr network) { + virNetworkIPDefPtr ipdef; + size_t i; + bool needDnsmasq = false; virCommandPtr cmd = NULL; char *pidfile = NULL; int ret = -1; dnsmasqContext *dctx = NULL; - if (!virNetworkDefGetIPByIndex(network->def, AF_UNSPEC, 0)) { + if (!(ipdef = virNetworkDefGetIPByIndex(network->def, AF_UNSPEC, 0))) { /* no IP addresses, so we don't need to run */ ret = 0; goto cleanup; } + /* see if there are any IP addresses that need a dhcp server */ + for (i = 0; ipdef && !needDnsmasq; + ipdef = virNetworkDefGetIPByIndex(network->def, AF_UNSPEC, i + 1)) { + if (ipdef->nranges || ipdef->nhosts) + needDnsmasq = true; + } + + if (!needDnsmasq && network->def->dns.enable == VIR_TRISTATE_BOOL_NO) { + ret = 0; + goto cleanup; + } + if (virFileMakePath(driver->pidDir) < 0) { virReportSystemError(errno, _("cannot create directory %s"), diff --git a/tests/networkxml2confdata/routed-network-no-dns.conf b/tests/networkxml2confdata/routed-network-no-dns.conf new file mode 100644 index 0000000..83cc85e --- /dev/null +++ b/tests/networkxml2confdata/routed-network-no-dns.conf @@ -0,0 +1,11 @@ +##WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE +##OVERWRITTEN AND LOST. Changes to this configuration should be made using: +## virsh net-edit local +## or other application using the libvirt API. +## +## dnsmasq conf file created by libvirt +strict-order +port=0 +except-interface=lo +bind-dynamic +interface=virbr1 diff --git a/tests/networkxml2confdata/routed-network-no-dns.xml b/tests/networkxml2confdata/routed-network-no-dns.xml new file mode 100644 index 0000000..70d0417 --- /dev/null +++ b/tests/networkxml2confdata/routed-network-no-dns.xml @@ -0,0 +1,10 @@ +<network> + <name>local</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr1"/> + <mac address='12:34:56:78:9A:BC'/> + <forward mode="route" dev="eth1"/> + <dns enable='no'/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + </ip> +</network> diff --git a/tests/networkxml2conftest.c b/tests/networkxml2conftest.c index 77acc53..5c71c79 100644 --- a/tests/networkxml2conftest.c +++ b/tests/networkxml2conftest.c @@ -117,6 +117,7 @@ mymain(void) DO_TEST("nat-network-dns-srv-record-minimal", restricted); DO_TEST("nat-network-name-with-quotes", restricted); DO_TEST("routed-network", full); + DO_TEST("routed-network-no-dns", full); DO_TEST("open-network", full); DO_TEST("nat-network", dhcpv6); DO_TEST("nat-network-dns-txt-record", full); diff --git a/tests/networkxml2xmlin/routed-network-no-dns.xml b/tests/networkxml2xmlin/routed-network-no-dns.xml new file mode 100644 index 0000000..70d0417 --- /dev/null +++ b/tests/networkxml2xmlin/routed-network-no-dns.xml @@ -0,0 +1,10 @@ +<network> + <name>local</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr1"/> + <mac address='12:34:56:78:9A:BC'/> + <forward mode="route" dev="eth1"/> + <dns enable='no'/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + </ip> +</network> diff --git a/tests/networkxml2xmlout/routed-network-no-dns.xml b/tests/networkxml2xmlout/routed-network-no-dns.xml new file mode 100644 index 0000000..f68ce8a --- /dev/null +++ b/tests/networkxml2xmlout/routed-network-no-dns.xml @@ -0,0 +1,12 @@ +<network> + <name>local</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='route'> + <interface dev='eth1'/> + </forward> + <bridge name='virbr1' stp='on' delay='0'/> + <mac address='12:34:56:78:9a:bc'/> + <dns enable='no'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + </ip> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 32544d0..b17674e 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -127,6 +127,7 @@ mymain(void) DO_TEST("empty-allow-ipv6"); DO_TEST("isolated-network"); DO_TEST("routed-network"); + DO_TEST("routed-network-no-dns"); DO_TEST("open-network"); DO_TEST_PARSE_ERROR("open-network-with-forward-dev"); DO_TEST("nat-network"); -- 2.7.4
On 12.08.2016 04:41, Laine Stump wrote:
If you define a libvirt virtual network with one or more IP addresses, it starts up an instance of dnsmasq. It's always been possible to avoid dnsmasq's dhcp server (simply don't include a <dhcp> element), but until now it wasn't possible to avoid having the DNS server listening; even if the network has no <dns> element, it is started using default settings.
This patch adds a new attribute to <dns>: enable='yes|no'. For backward compatibility, it defaults to 'yes', but if you don't want a DNS server created for the network, you can simply add:
<dns enable='no'/>
to the network configuration, and next time the network is started there will be no dns server created (if there is dhcp configuration, dnsmasq will be started with "port=0" which disables the DNS server; if there is no dhcp configuration, dnsmasq won't be started at all). --- docs/formatnetwork.html.in | 12 ++ docs/schemas/network.rng | 5 + src/conf/network_conf.c | 36 ++++- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 146 ++++++++++++--------- .../networkxml2confdata/routed-network-no-dns.conf | 11 ++ .../networkxml2confdata/routed-network-no-dns.xml | 10 ++ tests/networkxml2conftest.c | 1 + tests/networkxml2xmlin/routed-network-no-dns.xml | 10 ++ tests/networkxml2xmlout/routed-network-no-dns.xml | 12 ++ tests/networkxml2xmltest.c | 1 + 11 files changed, 179 insertions(+), 66 deletions(-) create mode 100644 tests/networkxml2confdata/routed-network-no-dns.conf create mode 100644 tests/networkxml2confdata/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlin/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlout/routed-network-no-dns.xml
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 12d1bed..e103dd7 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -886,6 +886,18 @@ server <span class="since">Since 0.9.3</span>.
<p> + The dns element can have an optional <code>enable</code> + attribute <span class="since">Since 2.2.0</span>. + If <code>enable</code> is "no", then no DNS server will be + setup by libvirt for this network (and any other + configuration in <code><dns></code> will be ignored). + If <code>enable</code> is "yes" or unspecified (including + the complete absence of any <code><dns></code> + element) then a DNS server will be setup by libvirt to + listen on all IP addresses specified in the network's + configuration. + </p>
Le sigh. I wish that we could just disable dns if the tag is not present in the nework XML. But we can't do that, can we?
+ <p> The dns element can have an optional <code>forwardPlainNames</code> attribute <span class="since">Since 1.1.2</span>.
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6820bde..490574f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1335,6 +1335,7 @@ virNetworkDNSDefParseXML(const char *networkName, xmlNodePtr *txtNodes = NULL; xmlNodePtr *fwdNodes = NULL; char *forwardPlainNames = NULL; + char *enable = NULL; int nhosts, nsrvs, ntxts, nfwds; size_t i; int ret = -1; @@ -1342,6 +1343,18 @@ virNetworkDNSDefParseXML(const char *networkName,
ctxt->node = node;
+ enable = virXPathString("string(./@enable)", ctxt); + if (enable) { + def->enable = virTristateBoolTypeFromString(enable); + if (def->enable <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid dns enable setting '%s' " + "in network '%s'"), + enable, networkName); + goto cleanup; + } + } + forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt); if (forwardPlainNames) { def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames); @@ -1440,6 +1453,7 @@ virNetworkDNSDefParseXML(const char *networkName,
ret = 0; cleanup: + VIR_FREE(enable); VIR_FREE(forwardPlainNames); VIR_FREE(fwdNodes); VIR_FREE(hostNodes); @@ -2496,12 +2510,22 @@ virNetworkDNSDefFormat(virBufferPtr buf, { size_t i, j;
- if (!(def->forwardPlainNames || def->nfwds || def->nhosts || + if (!(def->enable || def->forwardPlainNames || def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) return 0;
virBufferAddLit(buf, "<dns"); - /* default to "yes", but don't format it in the XML */ + if (def->enable) { + const char *fwd = virTristateBoolTypeToString(def->enable); + + if (!fwd) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown enable type %d in network"), + def->enable); + return -1;
I don't think check is needed. We've validated the forward mode when parsing the XML. Also, I think that we need slightly different approach here. I mean, for "<dns enable='no'/>" case we just want to put that string into XML and nothing more. With this code I'm able to get the following which makes not much sense to me: <dns enable='no'> <txt name='example' value='example value'/> </dns>
+ } + virBufferAsprintf(buf, " enable='%s'", fwd); + } if (def->forwardPlainNames) { const char *fwd = virTristateBoolTypeToString(def->forwardPlainNames);
The rest of the patch looks okay. ACK if you fix the XML formatting issue. Michal
On Aug 18, 2016 5:01 AM, "Michal Privoznik" <mprivozn@redhat.com> wrote:
On 12.08.2016 04:41, Laine Stump wrote:
If you define a libvirt virtual network with one or more IP addresses, it starts up an instance of dnsmasq. It's always been possible to avoid dnsmasq's dhcp server (simply don't include a <dhcp> element), but until now it wasn't possible to avoid having the DNS server listening; even if the network has no <dns> element, it is started using default settings.
This patch adds a new attribute to <dns>: enable='yes|no'. For backward compatibility, it defaults to 'yes', but if you don't want a DNS server created for the network, you can simply add:
<dns enable='no'/>
to the network configuration, and next time the network is started there will be no dns server created (if there is dhcp configuration, dnsmasq will be started with "port=0" which disables the DNS server; if there is no dhcp configuration, dnsmasq won't be started at all). --- docs/formatnetwork.html.in | 12 ++ docs/schemas/network.rng | 5 + src/conf/network_conf.c | 36 ++++- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 146
++++++++++++---------
.../networkxml2confdata/routed-network-no-dns.conf | 11 ++ .../networkxml2confdata/routed-network-no-dns.xml | 10 ++ tests/networkxml2conftest.c | 1 + tests/networkxml2xmlin/routed-network-no-dns.xml | 10 ++ tests/networkxml2xmlout/routed-network-no-dns.xml | 12 ++ tests/networkxml2xmltest.c | 1 + 11 files changed, 179 insertions(+), 66 deletions(-) create mode 100644 tests/networkxml2confdata/routed-network-no-dns.conf create mode 100644 tests/networkxml2confdata/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlin/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlout/routed-network-no-dns.xml
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 12d1bed..e103dd7 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -886,6 +886,18 @@ server <span class="since">Since 0.9.3</span>.
<p> + The dns element can have an optional <code>enable</code> + attribute <span class="since">Since 2.2.0</span>. + If <code>enable</code> is "no", then no DNS server will be + setup by libvirt for this network (and any other + configuration in <code><dns></code> will be ignored). + If <code>enable</code> is "yes" or unspecified (including + the complete absence of any <code><dns></code> + element) then a DNS server will be setup by libvirt to + listen on all IP addresses specified in the network's + configuration. + </p>
Le sigh. I wish that we could just disable dns if the tag is not present in the nework XML. But we can't do that, can we?
Nope :-(. Backward compatibility and all that.
+ <p> The dns element can have an optional <code>forwardPlainNames</code> attribute <span class="since">Since 1.1.2</span>.
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6820bde..490574f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1335,6 +1335,7 @@ virNetworkDNSDefParseXML(const char *networkName, xmlNodePtr *txtNodes = NULL; xmlNodePtr *fwdNodes = NULL; char *forwardPlainNames = NULL; + char *enable = NULL; int nhosts, nsrvs, ntxts, nfwds; size_t i; int ret = -1; @@ -1342,6 +1343,18 @@ virNetworkDNSDefParseXML(const char *networkName,
ctxt->node = node;
+ enable = virXPathString("string(./@enable)", ctxt); + if (enable) { + def->enable = virTristateBoolTypeFromString(enable); + if (def->enable <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid dns enable setting '%s' " + "in network '%s'"), + enable, networkName); + goto cleanup; + } + } + forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt); if (forwardPlainNames) { def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames); @@ -1440,6 +1453,7 @@ virNetworkDNSDefParseXML(const char *networkName,
ret = 0; cleanup: + VIR_FREE(enable); VIR_FREE(forwardPlainNames); VIR_FREE(fwdNodes); VIR_FREE(hostNodes); @@ -2496,12 +2510,22 @@ virNetworkDNSDefFormat(virBufferPtr buf, { size_t i, j;
- if (!(def->forwardPlainNames || def->nfwds || def->nhosts || + if (!(def->enable || def->forwardPlainNames || def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) return 0;
virBufferAddLit(buf, "<dns"); - /* default to "yes", but don't format it in the XML */ + if (def->enable) { + const char *fwd = virTristateBoolTypeToString(def->enable); + + if (!fwd) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown enable type %d in network"), + def->enable); + return -1;
I don't think check is needed. We've validated the forward mode when parsing the XML.
Depends on your philosophy. If you trust that nothing could have modified the value from the time it was parsed until the time it's formated, then yes. If you don't trust that (or are uncomfortable that a pointer from a function that could potentially return NULL is used as an argument to an sprintf-like function without checking for NULL), then no. Libvirt code disagrees about this a lot, even within code written by the same person (e.g. me). These days I tend mor towards the "defensive programming" style of checking everything unless something in the direct call chain guarantees that it has been validated.
Also, I think that we need slightly different approach here. I mean, for "<dns enable='no'/>" case we just want to put that string into XML and nothing more. With this code I'm able to get the following which makes not much sense to me:
<dns enable='no'> <txt name='example' value='example value'/> </dns>
I thought about that before making it this way. The idea is to allow temporarily turning off the DNS server without losing all the config. I'm not married to that approach though (especially since we don't do that for other configures AFAIK). So I can modify the behavior
+ } + virBufferAsprintf(buf, " enable='%s'", fwd); + } if (def->forwardPlainNames) { const char *fwd =
virTristateBoolTypeToString(def->forwardPlainNames);
The rest of the patch looks okay. ACK if you fix the XML formatting issue.
Michal
On 18.08.2016 20:42, Laine Stump wrote:
On Aug 18, 2016 5:01 AM, "Michal Privoznik" <mprivozn@redhat.com> wrote:
On 12.08.2016 04:41, Laine Stump wrote:
If you define a libvirt virtual network with one or more IP addresses, it starts up an instance of dnsmasq. It's always been possible to avoid dnsmasq's dhcp server (simply don't include a <dhcp> element), but until now it wasn't possible to avoid having the DNS server listening; even if the network has no <dns> element, it is started using default settings.
This patch adds a new attribute to <dns>: enable='yes|no'. For backward compatibility, it defaults to 'yes', but if you don't want a DNS server created for the network, you can simply add:
<dns enable='no'/>
to the network configuration, and next time the network is started there will be no dns server created (if there is dhcp configuration, dnsmasq will be started with "port=0" which disables the DNS server; if there is no dhcp configuration, dnsmasq won't be started at all). --- docs/formatnetwork.html.in | 12 ++ docs/schemas/network.rng | 5 + src/conf/network_conf.c | 36 ++++- src/conf/network_conf.h | 1 + src/network/bridge_driver.c | 146
++++++++++++---------
.../networkxml2confdata/routed-network-no-dns.conf | 11 ++ .../networkxml2confdata/routed-network-no-dns.xml | 10 ++ tests/networkxml2conftest.c | 1 + tests/networkxml2xmlin/routed-network-no-dns.xml | 10 ++ tests/networkxml2xmlout/routed-network-no-dns.xml | 12 ++ tests/networkxml2xmltest.c | 1 + 11 files changed, 179 insertions(+), 66 deletions(-) create mode 100644 tests/networkxml2confdata/routed-network-no-dns.conf create mode 100644 tests/networkxml2confdata/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlin/routed-network-no-dns.xml create mode 100644 tests/networkxml2xmlout/routed-network-no-dns.xml
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 12d1bed..e103dd7 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -886,6 +886,18 @@ server <span class="since">Since 0.9.3</span>.
<p> + The dns element can have an optional <code>enable</code> + attribute <span class="since">Since 2.2.0</span>. + If <code>enable</code> is "no", then no DNS server will be + setup by libvirt for this network (and any other + configuration in <code><dns></code> will be ignored). + If <code>enable</code> is "yes" or unspecified (including + the complete absence of any <code><dns></code> + element) then a DNS server will be setup by libvirt to + listen on all IP addresses specified in the network's + configuration. + </p>
Le sigh. I wish that we could just disable dns if the tag is not present in the nework XML. But we can't do that, can we?
Nope :-(. Backward compatibility and all that.
Yep, I feared that.
+ <p> The dns element can have an optional <code>forwardPlainNames</code> attribute <span class="since">Since 1.1.2</span>.
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6820bde..490574f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1335,6 +1335,7 @@ virNetworkDNSDefParseXML(const char *networkName, xmlNodePtr *txtNodes = NULL; xmlNodePtr *fwdNodes = NULL; char *forwardPlainNames = NULL; + char *enable = NULL; int nhosts, nsrvs, ntxts, nfwds; size_t i; int ret = -1; @@ -1342,6 +1343,18 @@ virNetworkDNSDefParseXML(const char *networkName,
ctxt->node = node;
+ enable = virXPathString("string(./@enable)", ctxt); + if (enable) { + def->enable = virTristateBoolTypeFromString(enable); + if (def->enable <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid dns enable setting '%s' " + "in network '%s'"), + enable, networkName); + goto cleanup; + } + } + forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt); if (forwardPlainNames) { def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames); @@ -1440,6 +1453,7 @@ virNetworkDNSDefParseXML(const char *networkName,
ret = 0; cleanup: + VIR_FREE(enable); VIR_FREE(forwardPlainNames); VIR_FREE(fwdNodes); VIR_FREE(hostNodes); @@ -2496,12 +2510,22 @@ virNetworkDNSDefFormat(virBufferPtr buf, { size_t i, j;
- if (!(def->forwardPlainNames || def->nfwds || def->nhosts || + if (!(def->enable || def->forwardPlainNames || def->nfwds || def->nhosts || def->nsrvs || def->ntxts)) return 0;
virBufferAddLit(buf, "<dns"); - /* default to "yes", but don't format it in the XML */ + if (def->enable) { + const char *fwd = virTristateBoolTypeToString(def->enable); + + if (!fwd) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown enable type %d in network"), + def->enable); + return -1;
I don't think check is needed. We've validated the forward mode when parsing the XML.
Depends on your philosophy. If you trust that nothing could have modified the value from the time it was parsed until the time it's formated, then yes. If you don't trust that (or are uncomfortable that a pointer from a function that could potentially return NULL is used as an argument to an sprintf-like function without checking for NULL), then no.
So I've just checked and the worst case scenario is that we produce " enable='(null)'" into the XML (without any crash), which to me is a risk I can live with. Moreover, if the value has been modified, we can't be entirely sure it was modified to something outside boundaries. It might as well be changed from 'no' to 'yes' (or vice versa) which is not any worse than the previous case IMO.
Libvirt code disagrees about this a lot, even within code written by the same person (e.g. me). These days I tend more towards the "defensive programming" style of checking everything unless something in the direct call chain guarantees that it has been validated.
Okay, I can live with that.
Also, I think that we need slightly different approach here. I mean, for "<dns enable='no'/>" case we just want to put that string into XML and nothing more. With this code I'm able to get the following which makes not much sense to me:
<dns enable='no'> <txt name='example' value='example value'/> </dns>
I thought about that before making it this way. The idea is to allow temporarily turning off the DNS server without losing all the config. I'm not married to that approach though (especially since we don't do that for other configures AFAIK). So I can modify the behavior.
Right. We don't usually keep 'backups' around. I think our users are used to that fact and if they don't want to lose config they just make a copy of it somewhere before making any changes. Michal
On 08/19/2016 02:50 AM, Michal Privoznik wrote:
On 18.08.2016 20:42, Laine Stump wrote:
On Aug 18, 2016 5:01 AM, "Michal Privoznik" <mprivozn@redhat.com> wrote:
On 12.08.2016 04:41, Laine Stump wrote:
def->nsrvs || def->ntxts)) return 0;
virBufferAddLit(buf, "<dns"); - /* default to "yes", but don't format it in the XML */ + if (def->enable) { + const char *fwd = virTristateBoolTypeToString(def->enable); + + if (!fwd) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown enable type %d in network"), + def->enable); + return -1;
I don't think check is needed. We've validated the forward mode when parsing the XML.
Depends on your philosophy. If you trust that nothing could have modified the value from the time it was parsed until the time it's formated, then yes. If you don't trust that (or are uncomfortable that a pointer from a function that could potentially return NULL is used as an argument to an sprintf-like function without checking for NULL), then no. So I've just checked and the worst case scenario is that we produce " enable='(null)'" into the XML (without any crash),
Yes, as long as you are using glibc's sprintf, which checks for a NULL argument being used for%s and replaces it with a pointer to "(null)". As far as I know this isn't guaranteed by any standard though, and libvirt is built on platforms that don't use glibc.
which to me is a risk I can live with.
If all platforms used (or behaved the same as) glibc, then I would agree (although having the error log there would make it easier to find any future bugs).
Moreover, if the value has been modified, we can't be entirely sure it was modified to something outside boundaries. It might as well be changed from 'no' to 'yes' (or vice versa) which is not any worse than the previous case IMO.
I don't follow the chain of logic there.
On 19.08.2016 17:26, Laine Stump wrote:
Moreover, if the value has been modified, we can't be entirely sure it was modified to something outside boundaries. It might as well be changed from 'no' to 'yes' (or vice versa) which is not any worse than the previous case IMO.
I don't follow the chain of logic there.
You say that you're worried about hidden change of value of a variable in our code. Long story short. int enable; enable = parseXML(); if (validate(enable) < 0) { /* valid values are 1 or 2 */ virReportError(); die(); } /* For demonstrational purposes assume: */ enable = 1; /* now the netowrk object lives its own life and something might accidentally change enable */ enable = 3; /* but where does this 3 come from? What if the buggy code changes that to say 2? */ enable = 2; formatXML(enable); Yes, we won't crash, but we will not produce correct XML either. Valid one, but not what user expected. Then again, I can live with that check being there, it's just that I don't find it much helpful and consistent with the rest of our code. Michal
For some unknown reason the original implementation of the <forwarder> element only took advantage of part of the functionality in the dnsmasq feature it exposes - it allowed specifying the ip address of a DNS server which *all* DNS requests would be forwarded to, like this: <forwarder addr='192.168.123.25'/> This is a frontend for dnsmasq's "server" option, which also allows you to specify a domain that must be matched in order for a request to be forwarded to a particular server. This patch adds support for specifying the domain. For example: <forwarder domain='example.com' addr='192.168.1.1'/> <forwarder domain='www.example.com'/> <forwarder domain='travesty.org' addr='10.0.0.1'/> would forward requests for bob.example.com, ftp.example.com and joe.corp.example.com all to the DNS server at 192.168.1.1, but would forward requests for travesty.org and www.travesty.org to 10.0.0.1. And due to the second line, requests for www.example.com, and odd.www.example.com would be resolved by the libvirt network's own DNS server (i.e. thery wouldn't be immediately forwarded) even though they also match 'example.com' - the match is given to the entry with the longest matching domain. DNS requests not matching any of the entries would be resolved by the libvirt network's own DNS server. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1331796 --- docs/formatnetwork.html.in | 28 +++++++++---- docs/schemas/network.rng | 8 +++- src/conf/network_conf.c | 49 ++++++++++++++++++---- src/conf/network_conf.h | 8 +++- src/network/bridge_driver.c | 17 +++++++- .../nat-network-dns-forwarders.conf | 2 + .../nat-network-dns-forwarders.xml | 2 + .../nat-network-dns-forwarders.xml | 6 ++- .../nat-network-dns-forwarders.xml | 2 + 9 files changed, 101 insertions(+), 21 deletions(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index e103dd7..13ca32d 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -847,7 +847,8 @@ <dns> <txt name="example" value="example value" /> <forwarder addr="8.8.8.8"/> - <forwarder addr="8.8.4.4"/> + <forwarder domain='example.com' addr="8.8.4.4"/> + <forwarder domain='www.example.com'/> <srv service='name' protocol='tcp' domain='test-domain-name' target='.' port='1024' priority='10' weight='10'/> <host ip='192.168.122.2'> <hostname>myhost</hostname> @@ -915,12 +916,25 @@ Currently supported sub-elements of <code><dns></code> are: <dl> <dt><code>forwarder</code></dt> - <dd>A <code>dns</code> element can have 0 or - more <code>forwarder</code> elements. Each forwarder - element defines an IP address to be used as forwarder in - DNS server configuration. The addr attribute is required - and defines the IP address of every - forwarder. <span class="since">Since 1.1.3</span> + <dd>The dns element can have 0 or + more <code><forwarder></code> elements. Each + forwarder element defines an alternate DNS server to use + for some, or all, DNS requests sent to this network's DNS + server. There are two attributes - <code>domain</code>, + and <code>addr</code>; at least one of these must be + specified in any <code><forwarder></code> + element. If both <code>domain</code> and <code>addr</code> + are specified, then all requests that match the given + domain will be forwarded to the DNS server at addr. If + only <code>domain</code> is specified, then all matching + domains will be resolved locally (or via the host's + standard DNS forwarding if they can't be resolved + locally). If an <code>addr</code> is specified by itself, + then all DNS requests to the network's DNS server will be + forwarded to the DNS server at that address with no + exceptions. <code>addr</code> <span class="since">Since + 1.1.3</span>, <code>domain</code> <span class="since">Since + 2.2.0</span>. </dd> <dt><code>txt</code></dt> <dd>A <code>dns</code> element can have 0 or more <code>txt</code> elements. diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 12d4b34..1a18e64 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -260,7 +260,13 @@ <interleave> <zeroOrMore> <element name="forwarder"> - <attribute name="addr"><ref name="ipAddr"/></attribute> + <optional> + <attribute name="addr"><ref name="ipAddr"/></attribute> + </optional> + <optional> + <attribute name="domain"><ref name="dnsName"/></attribute> + </optional> + <empty/> </element> </zeroOrMore> <zeroOrMore> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 490574f..a7d42cc 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -349,12 +349,20 @@ virNetworkDNSSrvDefClear(virNetworkDNSSrvDefPtr def) VIR_FREE(def->target); } + +static void +virNetworkDNSForwarderClear(virNetworkDNSForwarderPtr def) +{ + VIR_FREE(def->domain); +} + + static void virNetworkDNSDefClear(virNetworkDNSDefPtr def) { if (def->forwarders) { while (def->nfwds) - VIR_FREE(def->forwarders[--def->nfwds]); + virNetworkDNSForwarderClear(&def->forwarders[--def->nfwds]); VIR_FREE(def->forwarders); } if (def->txts) { @@ -1379,14 +1387,25 @@ virNetworkDNSDefParseXML(const char *networkName, goto cleanup; for (i = 0; i < nfwds; i++) { - def->forwarders[i] = virXMLPropString(fwdNodes[i], "addr"); - if (virSocketAddrParse(NULL, def->forwarders[i], AF_UNSPEC) < 0) { + char *addr = virXMLPropString(fwdNodes[i], "addr"); + + if (addr && virSocketAddrParse(&def->forwarders[i].addr, + addr, AF_UNSPEC) < 0) { virReportError(VIR_ERR_XML_ERROR, - _("Invalid forwarder IP address '%s' " - "in network '%s'"), - def->forwarders[i], networkName); + _("Invalid forwarder IP address '%s' " + "in network '%s'"), + addr, networkName); + VIR_FREE(addr); + goto cleanup; + } + def->forwarders[i].domain = virXMLPropString(fwdNodes[i], "domain"); + if (!(addr || def->forwarders[i].domain)) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Invalid forwarder element, must contain " + "at least one of addr or domain")); goto cleanup; } + VIR_FREE(addr); def->nfwds++; } } @@ -2546,8 +2565,22 @@ virNetworkDNSDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); for (i = 0; i < def->nfwds; i++) { - virBufferAsprintf(buf, "<forwarder addr='%s'/>\n", - def->forwarders[i]); + + virBufferAddLit(buf, "<forwarder"); + if (def->forwarders[i].domain) { + virBufferEscapeString(buf, " domain='%s'", + def->forwarders[i].domain); + } + if (VIR_SOCKET_ADDR_VALID(&def->forwarders[i].addr)) { + char *addr = virSocketAddrFormat(&def->forwarders[i].addr); + + if (!addr) + return -1; + + virBufferAsprintf(buf, " addr='%s'", addr); + VIR_FREE(addr); + } + virBufferAddLit(buf, "/>\n"); } for (i = 0; i < def->ntxts; i++) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 9ebd4a7..3b227db 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -125,6 +125,12 @@ struct _virNetworkDNSHostDef { char **names; }; + +typedef struct _virNetworkDNSForwarder { + virSocketAddr addr; + char *domain; +} virNetworkDNSForwarder, *virNetworkDNSForwarderPtr; + typedef struct _virNetworkDNSDef virNetworkDNSDef; typedef virNetworkDNSDef *virNetworkDNSDefPtr; struct _virNetworkDNSDef { @@ -137,7 +143,7 @@ struct _virNetworkDNSDef { size_t nsrvs; virNetworkDNSSrvDefPtr srvs; size_t nfwds; - char **forwarders; + virNetworkDNSForwarderPtr forwarders; }; typedef struct _virNetworkIPDef virNetworkIPDef; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 49c0a2f..74f75d0 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -958,8 +958,21 @@ networkDnsmasqConfContents(virNetworkObjPtr network, if (wantDNS && network->def->dns.forwarders) { virBufferAddLit(&configbuf, "no-resolv\n"); for (i = 0; i < network->def->dns.nfwds; i++) { - virBufferAsprintf(&configbuf, "server=%s\n", - network->def->dns.forwarders[i]); + virNetworkDNSForwarderPtr fwd = &network->def->dns.forwarders[i]; + + virBufferAddLit(&configbuf, "server="); + if (fwd->domain) + virBufferAsprintf(&configbuf, "/%s/", fwd->domain); + if (VIR_SOCKET_ADDR_VALID(&fwd->addr)) { + char *addr = virSocketAddrFormat(&fwd->addr); + + if (!addr) + goto cleanup; + virBufferAsprintf(&configbuf, "%s\n", addr); + } else { + /* "don't forward requests for this domain" */ + virBufferAddLit(&configbuf, "#\n"); + } } } diff --git a/tests/networkxml2confdata/nat-network-dns-forwarders.conf b/tests/networkxml2confdata/nat-network-dns-forwarders.conf index 8bf3b9c..0bd76bf 100644 --- a/tests/networkxml2confdata/nat-network-dns-forwarders.conf +++ b/tests/networkxml2confdata/nat-network-dns-forwarders.conf @@ -8,6 +8,8 @@ strict-order no-resolv server=8.8.8.8 server=8.8.4.4 +server=/example.com/192.168.1.1 +server=/www.example.com/# except-interface=lo bind-dynamic interface=virbr0 diff --git a/tests/networkxml2confdata/nat-network-dns-forwarders.xml b/tests/networkxml2confdata/nat-network-dns-forwarders.xml index 8fab78e..5d4f3fa 100644 --- a/tests/networkxml2confdata/nat-network-dns-forwarders.xml +++ b/tests/networkxml2confdata/nat-network-dns-forwarders.xml @@ -6,6 +6,8 @@ <dns> <forwarder addr='8.8.8.8'/> <forwarder addr='8.8.4.4'/> + <forwarder domain='example.com' addr='192.168.1.1'/> + <forwarder domain='www.example.com'/> </dns> <ip address='192.168.122.1' netmask='255.255.255.0'> </ip> diff --git a/tests/networkxml2xmlin/nat-network-dns-forwarders.xml b/tests/networkxml2xmlin/nat-network-dns-forwarders.xml index 4d7310d..426dd45 100644 --- a/tests/networkxml2xmlin/nat-network-dns-forwarders.xml +++ b/tests/networkxml2xmlin/nat-network-dns-forwarders.xml @@ -4,8 +4,10 @@ <forward dev='eth0' mode='nat'/> <bridge name='virbr0' stp='on' delay='0' /> <dns> - <forwarder addr='8.8.8.8' /> - <forwarder addr='8.8.4.4' /> + <forwarder addr='8.8.8.8'/> + <forwarder addr='8.8.4.4'/> + <forwarder domain='example.com' addr='192.168.1.1'/> + <forwarder domain='www.example.com'/> </dns> <ip address='192.168.122.1' netmask='255.255.255.0'> </ip> diff --git a/tests/networkxml2xmlout/nat-network-dns-forwarders.xml b/tests/networkxml2xmlout/nat-network-dns-forwarders.xml index 930a42a..c05ad55 100644 --- a/tests/networkxml2xmlout/nat-network-dns-forwarders.xml +++ b/tests/networkxml2xmlout/nat-network-dns-forwarders.xml @@ -8,6 +8,8 @@ <dns> <forwarder addr='8.8.8.8'/> <forwarder addr='8.8.4.4'/> + <forwarder domain='example.com' addr='192.168.1.1'/> + <forwarder domain='www.example.com'/> </dns> <ip address='192.168.122.1' netmask='255.255.255.0'> </ip> -- 2.7.4
On 12.08.2016 04:41, Laine Stump wrote:
For some unknown reason the original implementation of the <forwarder> element only took advantage of part of the functionality in the dnsmasq feature it exposes - it allowed specifying the ip address of a DNS server which *all* DNS requests would be forwarded to, like this:
<forwarder addr='192.168.123.25'/>
This is a frontend for dnsmasq's "server" option, which also allows you to specify a domain that must be matched in order for a request to be forwarded to a particular server. This patch adds support for specifying the domain. For example:
<forwarder domain='example.com' addr='192.168.1.1'/> <forwarder domain='www.example.com'/> <forwarder domain='travesty.org' addr='10.0.0.1'/>
would forward requests for bob.example.com, ftp.example.com and joe.corp.example.com all to the DNS server at 192.168.1.1, but would forward requests for travesty.org and www.travesty.org to 10.0.0.1. And due to the second line, requests for www.example.com, and odd.www.example.com would be resolved by the libvirt network's own DNS server (i.e. thery wouldn't be immediately forwarded) even though they also match 'example.com' - the match is given to the entry with the longest matching domain. DNS requests not matching any of the entries would be resolved by the libvirt network's own DNS server.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1331796 --- docs/formatnetwork.html.in | 28 +++++++++---- docs/schemas/network.rng | 8 +++- src/conf/network_conf.c | 49 ++++++++++++++++++---- src/conf/network_conf.h | 8 +++- src/network/bridge_driver.c | 17 +++++++- .../nat-network-dns-forwarders.conf | 2 + .../nat-network-dns-forwarders.xml | 2 + .../nat-network-dns-forwarders.xml | 6 ++- .../nat-network-dns-forwarders.xml | 2 + 9 files changed, 101 insertions(+), 21 deletions(-)
ACK Michal
participants (3)
-
Daniel P. Berrange -
Laine Stump -
Michal Privoznik