[libvirt] [PATCH v4 1/2] net: support set public ip range for forward mode nat
Laine Stump
laine at laine.org
Thu Feb 14 19:06:10 UTC 2013
On 02/11/2013 09:54 AM, Natanael Copa wrote:
> Support setting which public ip to use for NAT via attribute
> address in subelement <nat> in <forward>:
>
> ...
> <forward mode='nat'>
> <address start='1.2.3.4' end='1.2.3.10'/>
> </forward>
> ...
>
> This will construct an iptables line using:
>
> '-j SNAT --to-source <start>-<end>'
>
> instead of:
>
> '-j MASQUERADE'
>
> Signed-off-by: Natanael Copa <ncopa at alpinelinux.org>
> ---
> docs/formatnetwork.html.in | 18 ++++++
> src/conf/network_conf.c | 152 ++++++++++++++++++++++++++++++++++++++++++--
> src/conf/network_conf.h | 3 +
> src/network/bridge_driver.c | 16 +++++
> src/util/viriptables.c | 56 +++++++++++++---
> src/util/viriptables.h | 4 ++
> 6 files changed, 235 insertions(+), 14 deletions(-)
>
> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
> index 7b42529..5fbd0a9 100644
> --- a/docs/formatnetwork.html.in
> +++ b/docs/formatnetwork.html.in
> @@ -136,6 +136,24 @@
> network, and to/from the host to the guests, are
> unrestricted and not NATed.<span class="since">Since
> 0.4.2</span>
> +
> + <p><span class="since">Since 1.0.3</span> it is possible to
> + specify a public IPv4 address range to be used for the NAT by
> + using the <code><nat></code> and
> + <code><address></code> subelements.
> + <pre>
> +...
> + <forward mode='nat'>
> + <nat>
> + <address start='1.2.3.4' end='1.2.3.10'/>
> + </nat>
> + </forward>
> +...
> + </pre>
> + An singe IPv4 address can be set by setting
> + <code>start</code> and <code>end</code> attributes to
> + the same value.
> + </p>
> </dd>
>
> <dt><code>route</code></dt>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 3604ff7..61d086a 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1325,6 +1325,80 @@ cleanup:
> }
>
> static int
> +virNetworkForwardNatDefParseXML(const char *networkName,
> + xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + virNetworkForwardDefPtr def)
> +{
> + int ret = -1;
> + xmlNodePtr *natAddrNodes = NULL;
> + int nNatAddrs;
> + char *addr_start = NULL;
> + char *addr_end = NULL;
Although you'll find both underscore_separated variable names and
studlyCapped variable names in libvirt, there seems to be more of a
preference for the latter. I'm changing all addr_start to addrStart, and
all addr_end to addrEnd.
> + xmlNodePtr save = ctxt->node;
> +
> + ctxt->node = node;
> +
> + if (def->type != VIR_NETWORK_FORWARD_NAT) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("The <nat> element can only be used when <forward> 'mode' is 'nat' in network %s"),
> + networkName);
> + goto cleanup;
> + }
> +
> + /* addresses for SNAT */
> + nNatAddrs = virXPathNodeSet("./address", ctxt, &natAddrNodes);
> + if (nNatAddrs < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("invalid <address> element found in <forward> of "
> + "network %s"), networkName);
> + goto cleanup;
> + } else if (nNatAddrs > 1) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Only one <address> element is allowed in <nat> in "
> + "<forward> in network %s"), networkName);
> + goto cleanup;
> + } else if (nNatAddrs == 1) {
> + addr_start = virXMLPropString(*natAddrNodes, "start");
> + if (addr_start == NULL) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("missing 'start' attribute in <address> element in <nat> in "
> + "<forward> in network %s"), networkName);
> + goto cleanup;
> + }
> + addr_end = virXMLPropString(*natAddrNodes, "end");
> + if (addr_end == NULL) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("missing 'end' attribute in <address> element in <nat> in "
> + "<forward> in network %s"), networkName);
> + goto cleanup;
> + }
> + }
> +
> + if (addr_start && virSocketAddrParse(&def->addr_start, addr_start, AF_INET) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Bad ipv4 start address '%s' in <nat> in <forward> in "
> + "network '%s'"), addr_start, networkName);
> + goto cleanup;
> + }
> +
> + if (addr_end && virSocketAddrParse(&def->addr_end, addr_end, AF_INET) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Bad ipv4 end address '%s' in <nat> in <forward> in "
> + "network '%s'"), addr_end, networkName);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + VIR_FREE(addr_start);
> + VIR_FREE(addr_end);
You forgot to free natAddrNodes.
> + ctxt->node = save;
> + return ret;
> +}
> +
> +static int
> virNetworkForwardDefParseXML(const char *networkName,
> xmlNodePtr node,
> xmlXPathContextPtr ctxt,
> @@ -1334,7 +1408,8 @@ virNetworkForwardDefParseXML(const char *networkName,
> xmlNodePtr *forwardIfNodes = NULL;
> xmlNodePtr *forwardPfNodes = NULL;
> xmlNodePtr *forwardAddrNodes = NULL;
> - int nForwardIfs, nForwardAddrs, nForwardPfs;
> + xmlNodePtr *forwardNatNodes = NULL;
> + int nForwardIfs, nForwardAddrs, nForwardPfs, nForwardNats;
> char *forwardDev = NULL;
> char *forwardManaged = NULL;
> char *type = NULL;
> @@ -1384,6 +1459,24 @@ virNetworkForwardDefParseXML(const char *networkName,
> goto cleanup;
> }
>
> + nForwardNats = virXPathNodeSet("./nat", ctxt, &forwardNatNodes);
> + if (nForwardNats < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("invalid <nat> element found in <forward> of network %s"),
> + networkName);
> + goto cleanup;
> + } else if (nForwardNats > 1) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Only one <nat> element is allowed in <forward> of network %s"),
> + networkName);
> + goto cleanup;
> + } else if (nForwardNats == 1) {
> + if (virNetworkForwardNatDefParseXML(networkName,
> + *forwardNatNodes,
> + ctxt, def) < 0)
> + goto cleanup;
I think the official rules for libvirt may only say that braces are
necessary if the body of a compound statement is > 1 line, but I prefer
to also add braces when the conditional is > 1 line.
> + }
> +
> if (((nForwardIfs > 0) + (nForwardAddrs > 0) + (nForwardPfs > 0)) > 1) {
> virReportError(VIR_ERR_XML_ERROR,
> _("<address>, <interface>, and <pf> elements in <forward> "
> @@ -1525,6 +1618,7 @@ cleanup:
> VIR_FREE(forwardPfNodes);
> VIR_FREE(forwardIfNodes);
> VIR_FREE(forwardAddrNodes);
> + VIR_FREE(forwardNatNodes);
> ctxt->node = save;
> return ret;
> }
> @@ -2079,13 +2173,54 @@ virPortGroupDefFormat(virBufferPtr buf,
> }
>
> static int
> +virNatDefFormat(virBufferPtr buf,
> + const virNetworkForwardDefPtr fwd)
This should be named consistent with the parse function -
virNetworkForwardNatDefFormat().
> +{
> + char *addr_start = NULL;
> + char *addr_end = NULL;
> + int ret = -1;
> +
> + if (VIR_SOCKET_ADDR_VALID(&fwd->addr_start)) {
> + addr_start = virSocketAddrFormat(&fwd->addr_start);
> + if (!addr_start)
> + goto cleanup;
> + }
> +
> + if (VIR_SOCKET_ADDR_VALID(&fwd->addr_end)) {
> + addr_end = virSocketAddrFormat(&fwd->addr_end);
> + if (!addr_end)
> + goto cleanup;
> + }
> +
> + if (!addr_end && !addr_start)
> + return 0;
> +
> + virBufferAddLit(buf, "<nat>\n");
> + virBufferAdjustIndent(buf, 2);
> +
> + virBufferAsprintf(buf, "<address start='%s'", addr_start);
> + if (addr_end)
> + virBufferAsprintf(buf, " end='%s'", addr_end);
> + virBufferAsprintf(buf, "/>\n");
> +
> + virBufferAdjustIndent(buf, -2);
> + virBufferAsprintf(buf, "</nat>\n");
> + ret = 0;
> +
> +cleanup:
> + VIR_FREE(addr_start);
> + VIR_FREE(addr_end);
> + return ret;
> +}
> +
> +static int
> virNetworkDefFormatInternal(virBufferPtr buf,
> const virNetworkDefPtr def,
> unsigned int flags)
> {
> unsigned char *uuid;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> - int ii;
> + int ii, shortforward;
>
> virBufferAddLit(buf, "<network");
> if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->connections > 0)) {
> @@ -2122,10 +2257,17 @@ virNetworkDefFormatInternal(virBufferPtr buf,
> else
> virBufferAddLit(buf, " managed='no'");
> }
> - virBufferAsprintf(buf, "%s>\n",
> - (def->forward.nifs || def->forward.npfs) ? "" : "/");
> + shortforward = !(def->forward.nifs || def->forward.npfs
> + || VIR_SOCKET_ADDR_VALID(&def->forward.addr_start)
> + || VIR_SOCKET_ADDR_VALID(&def->forward.addr_end));
Yes, nice refactor to eliminate the duplicated conditional below.
> + virBufferAsprintf(buf, "%s>\n", shortforward ? "/" : "");
> virBufferAdjustIndent(buf, 2);
>
> + if (def->forward.type == VIR_NETWORK_FORWARD_NAT) {
> + if (virNatDefFormat(buf, &def->forward) < 0)
> + goto error;
> + }
> +
> /* For now, hard-coded to at most 1 forward.pfs */
> if (def->forward.npfs)
> virBufferEscapeString(buf, "<pf dev='%s'/>\n",
> @@ -2155,7 +2297,7 @@ virNetworkDefFormatInternal(virBufferPtr buf,
> }
> }
> virBufferAdjustIndent(buf, -2);
> - if (def->forward.npfs || def->forward.nifs)
> + if (!shortforward)
> virBufferAddLit(buf, "</forward>\n");
> }
>
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index 4c634ed..1a598e3 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -174,6 +174,9 @@ struct _virNetworkForwardDef {
>
> size_t nifs;
> virNetworkForwardIfDefPtr ifs;
> +
> + /* adresses for SNAT */
> + virSocketAddr addr_start, addr_end;
> };
>
> typedef struct _virPortGroupDef virPortGroupDef;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index c834f83..6d74c1f 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1587,6 +1587,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addr_start,
> + &network->def->forward.addr_end,
> NULL) < 0) {
> virReportError(VIR_ERR_SYSTEM_ERROR,
> forwardIf ?
> @@ -1601,6 +1603,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addr_start,
> + &network->def->forward.addr_end,
> "udp") < 0) {
> virReportError(VIR_ERR_SYSTEM_ERROR,
> forwardIf ?
> @@ -1615,6 +1619,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addr_start,
> + &network->def->forward.addr_end,
> "tcp") < 0) {
> virReportError(VIR_ERR_SYSTEM_ERROR,
> forwardIf ?
> @@ -1631,12 +1637,16 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addr_start,
> + &network->def->forward.addr_end,
> "udp");
> masqerr4:
> iptablesRemoveForwardMasquerade(driver->iptables,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addr_start,
> + &network->def->forward.addr_end,
> NULL);
> masqerr3:
> iptablesRemoveForwardAllowRelatedIn(driver->iptables,
> @@ -1667,16 +1677,22 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addr_start,
> + &network->def->forward.addr_end,
> "tcp");
> iptablesRemoveForwardMasquerade(driver->iptables,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addr_start,
> + &network->def->forward.addr_end,
> "udp");
> iptablesRemoveForwardMasquerade(driver->iptables,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addr_start,
> + &network->def->forward.addr_end,
> NULL);
>
> iptablesRemoveForwardAllowRelatedIn(driver->iptables,
> diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> index 41fe780..3f0dcf0 100644
> --- a/src/util/viriptables.c
> +++ b/src/util/viriptables.c
> @@ -805,11 +805,15 @@ iptablesForwardMasquerade(iptablesContext *ctx,
> virSocketAddr *netaddr,
> unsigned int prefix,
> const char *physdev,
> + virSocketAddr *addr_start,
> + virSocketAddr *addr_end,
> const char *protocol,
> int action)
> {
> - int ret;
> - char *networkstr;
> + int ret = -1;
> + char *networkstr = NULL;
> + char *addr_start_str = NULL;
> + char *addr_end_str = NULL;
> virCommandPtr cmd = NULL;
>
> if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
> @@ -820,8 +824,18 @@ iptablesForwardMasquerade(iptablesContext *ctx,
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Attempted to NAT '%s'. NAT is only supported for IPv4."),
> networkstr);
> - VIR_FREE(networkstr);
> - return -1;
> + goto cleanup;
> + }
> +
> + if (VIR_SOCKET_ADDR_IS_FAMILY(addr_start, AF_INET)) {
> + addr_start_str = virSocketAddrFormat(addr_start);
> + if (!addr_start_str)
> + goto cleanup;
> + if (VIR_SOCKET_ADDR_IS_FAMILY(addr_end, AF_INET)) {
> + addr_end_str = virSocketAddrFormat(addr_end);
> + if (!addr_end_str)
> + goto cleanup;
The indentation went wrong here.
> + }
> }
>
> cmd = iptablesCommandNew(ctx->nat_postrouting, AF_INET, action);
> @@ -835,12 +849,32 @@ iptablesForwardMasquerade(iptablesContext *ctx,
> if (physdev && physdev[0])
> virCommandAddArgList(cmd, "--out-interface", physdev, NULL);
>
> - virCommandAddArgList(cmd, "--jump", "MASQUERADE", NULL);
> + /* Use --jump SNAT if public addr is specified */
> + if (addr_start_str && addr_start_str[0]) {
> + char tmpstr[sizeof("123.123.123.123-123.123.123.123:65535-65535")];
Rather than doing this and calling snprintf, we prefer to call
virAsprintf(), which allocates the memory as required and doesn't
consume space on the stack (of course then you have to free the memory
afterwards). I changed tmpstr into addrRangeStr, declared at the
toplevel of the function, changed the snprintf's to virAsprintfs() (with
proper checking for error return), and freed addrRangeStr during the
function's cleanup.
> + const char *portstr = "";
> +
> + memset(tmpstr, 0, sizeof(tmpstr));
> + if (protocol && protocol[0])
> + portstr = ":1024-65535";
> + if (addr_end_str && addr_end_str[0]) {
> + snprintf(tmpstr, sizeof(tmpstr), "%s-%s%s",
> + addr_start_str, addr_end_str, portstr);
> + } else {
> + snprintf(tmpstr, sizeof(tmpstr), "%s%s", addr_start_str, portstr);
> + }
>
> - if (protocol && protocol[0])
> - virCommandAddArgList(cmd, "--to-ports", "1024-65535", NULL);
> + virCommandAddArgList(cmd, "--jump", "SNAT",
> + "--to-source", tmpstr, NULL);
> + } else {
> + virCommandAddArgList(cmd, "--jump", "MASQUERADE", NULL);
> +
> + if (protocol && protocol[0])
> + virCommandAddArgList(cmd, "--to-ports", "1024-65535", NULL);
> + }
>
> ret = iptablesCommandRunAndFree(cmd);
> +cleanup:
> VIR_FREE(networkstr);
You didn't free addr_start_str or addr_end_str.
> return ret;
> }
> @@ -863,9 +897,11 @@ iptablesAddForwardMasquerade(iptablesContext *ctx,
> virSocketAddr *netaddr,
> unsigned int prefix,
> const char *physdev,
> + virSocketAddr *addr_start,
> + virSocketAddr *addr_end,
> const char *protocol)
> {
> - return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, protocol, ADD);
> + return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, addr_start, addr_end, protocol, ADD);
> }
>
> /**
> @@ -886,9 +922,11 @@ iptablesRemoveForwardMasquerade(iptablesContext *ctx,
> virSocketAddr *netaddr,
> unsigned int prefix,
> const char *physdev,
> + virSocketAddr *addr_start,
> + virSocketAddr *addr_end,
> const char *protocol)
> {
> - return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, protocol, REMOVE);
> + return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, addr_start, addr_end, protocol, REMOVE);
> }
>
>
> diff --git a/src/util/viriptables.h b/src/util/viriptables.h
> index d7fa731..4241380 100644
> --- a/src/util/viriptables.h
> +++ b/src/util/viriptables.h
> @@ -107,11 +107,15 @@ int iptablesAddForwardMasquerade (iptablesContext *ctx,
> virSocketAddr *netaddr,
> unsigned int prefix,
> const char *physdev,
> + virSocketAddr *addr_start,
> + virSocketAddr *addr_end,
> const char *protocol);
> int iptablesRemoveForwardMasquerade (iptablesContext *ctx,
> virSocketAddr *netaddr,
> unsigned int prefix,
> const char *physdev,
> + virSocketAddr *addr_start,
> + virSocketAddr *addr_end,
> const char *protocol);
> int iptablesAddOutputFixUdpChecksum (iptablesContext *ctx,
> const char *iface,
If I'd seen the need to change snprintf() to virAsprintf() when I
started, I might have just marked up this diff and asked for a V3, but
by the time I got there, I'd already done the other trivial changes I
listed above, so I made the virAsprintf() change as well.
Can someone check the interdiff I'm attaching to this message. If that
gets an ACK, then
I'll ACK the combination of that with the original (because the
interdiff addresses everything I mention in my review) and push.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-changes-from-review-of-nat-address-range-patch.patch
Type: text/x-patch
Size: 18769 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130214/767e022b/attachment-0001.bin>
More information about the libvir-list
mailing list