On 02/19/2013 05:44 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@xxxxxxxxxxxxxxx>
> ---
> 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 | 63 +++++++++++++++---
> src/util/viriptables.h | 4 ++
> 6 files changed, 242 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..620110c 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 *addrStart = NULL;
> + char *addrEnd = NULL;
> + 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) {
> + addrStart = virXMLPropString(*natAddrNodes, "start");
> + if (addrStart == NULL) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("missing 'start' attribute in <address> element in <nat> in "
> + "<forward> in network %s"), networkName);
> + goto cleanup;
> + }
> + addrEnd = virXMLPropString(*natAddrNodes, "end");
> + if (addrEnd == NULL) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("missing 'end' attribute in <address> element in <nat> in "
> + "<forward> in network %s"), networkName);
> + goto cleanup;
> + }
> + }
> +
> + if (addrStart && virSocketAddrParse(&def->addrStart, addrStart, AF_INET) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Bad ipv4 start address '%s' in <nat> in <forward> in "
> + "network '%s'"), addrStart, networkName);
> + goto cleanup;
> + }
> +
> + if (addrEnd && virSocketAddrParse(&def->addrEnd, addrEnd, AF_INET) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Bad ipv4 end address '%s' in <nat> in <forward> in "
> + "network '%s'"), addrEnd, networkName);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> +cleanup:
> + VIR_FREE(addrStart);
> + VIR_FREE(addrEnd);
You still forgot VIR_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;
> + }
> +
> 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,
Another thing I pointed out in my previous review: this should be named
consistent with the parse function - virNetworkForwardNatDefFormat() (I
know virPortgroupDefFormat() is also named inconsistently (and that's
probably why you named this one the way you did) - I'm going to fix that
in a separate patch).
> + const virNetworkForwardDefPtr fwd)
> +{
> + char *addrStart = NULL;
> + char *addrEnd = NULL;
> + int ret = -1;
> +
> + if (VIR_SOCKET_ADDR_VALID(&fwd->addrStart)) {
> + addrStart = virSocketAddrFormat(&fwd->addrStart);
> + if (!addrStart)
> + goto cleanup;
> + }
> +
> + if (VIR_SOCKET_ADDR_VALID(&fwd->addrEnd)) {
> + addrEnd = virSocketAddrFormat(&fwd->addrEnd);
> + if (!addrEnd)
> + goto cleanup;
> + }
> +
> + if (!addrEnd && !addrStart)
> + return 0;
> +
> + virBufferAddLit(buf, "<nat>\n");
> + virBufferAdjustIndent(buf, 2);
> +
> + virBufferAsprintf(buf, "<address start='%s'", addrStart);
> + if (addrEnd)
> + virBufferAsprintf(buf, " end='%s'", addrEnd);
> + virBufferAsprintf(buf, "/>\n");
Use virBufferAddLit() instead of virBufferAsprintf() when there are no args.
> +
> + virBufferAdjustIndent(buf, -2);
> + virBufferAsprintf(buf, "</nat>\n");
Again, use virBufferAsprintf().
> + ret = 0;
> +
> +cleanup:
> + VIR_FREE(addrStart);
> + VIR_FREE(addrEnd);
> + 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.addrStart)
> + || VIR_SOCKET_ADDR_VALID(&def->forward.addrEnd));
> + 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..11d6c9c 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 addrStart, addrEnd;
> };
>
> typedef struct _virPortGroupDef virPortGroupDef;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index c834f83..9f502a5 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.addrStart,
> + &network->def->forward.addrEnd,
> NULL) < 0) {
> virReportError(VIR_ERR_SYSTEM_ERROR,
> forwardIf ?
> @@ -1601,6 +1603,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addrStart,
> + &network->def->forward.addrEnd,
> "udp") < 0) {
> virReportError(VIR_ERR_SYSTEM_ERROR,
> forwardIf ?
> @@ -1615,6 +1619,8 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addrStart,
> + &network->def->forward.addrEnd,
> "tcp") < 0) {
> virReportError(VIR_ERR_SYSTEM_ERROR,
> forwardIf ?
> @@ -1631,12 +1637,16 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addrStart,
> + &network->def->forward.addrEnd,
> "udp");
> masqerr4:
> iptablesRemoveForwardMasquerade(driver->iptables,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addrStart,
> + &network->def->forward.addrEnd,
> NULL);
> masqerr3:
> iptablesRemoveForwardAllowRelatedIn(driver->iptables,
> @@ -1667,16 +1677,22 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addrStart,
> + &network->def->forward.addrEnd,
> "tcp");
> iptablesRemoveForwardMasquerade(driver->iptables,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addrStart,
> + &network->def->forward.addrEnd,
> "udp");
> iptablesRemoveForwardMasquerade(driver->iptables,
> &ipdef->address,
> prefix,
> forwardIf,
> + &network->def->forward.addrStart,
> + &network->def->forward.addrEnd,
> NULL);
>
> iptablesRemoveForwardAllowRelatedIn(driver->iptables,
> diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> index 41fe780..f44236a 100644
> --- a/src/util/viriptables.c
> +++ b/src/util/viriptables.c
> @@ -805,11 +805,16 @@ iptablesForwardMasquerade(iptablesContext *ctx,
> virSocketAddr *netaddr,
> unsigned int prefix,
> const char *physdev,
> + virSocketAddr *addrStart,
> + virSocketAddr *addrEnd,
> const char *protocol,
> int action)
> {
> - int ret;
> - char *networkstr;
> + int ret = -1;
> + char *networkstr = NULL;
> + char *addrStartStr = NULL;
> + char *addrEndStr = NULL;
> + char *tmpStr = NULL;
I'm changing this to natRangeStr.
> virCommandPtr cmd = NULL;
>
> if (!(networkstr = iptablesFormatNetwork(netaddr, prefix)))
> @@ -820,8 +825,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(addrStart, AF_INET)) {
> + addrStartStr = virSocketAddrFormat(addrStart);
> + if (!addrStartStr)
> + goto cleanup;
> + if (VIR_SOCKET_ADDR_IS_FAMILY(addrEnd, AF_INET)) {
> + addrEndStr = virSocketAddrFormat(addrEnd);
> + if (!addrEndStr)
> + goto cleanup;
indentation is still messed up here.
> + }
> }
>
> cmd = iptablesCommandNew(ctx->nat_postrouting, AF_INET, action);
> @@ -835,13 +850,39 @@ 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 (addrStartStr && addrStartStr[0]) {
> + const char *portStr = "";
> + int r = 0;
>
> - if (protocol && protocol[0])
> - virCommandAddArgList(cmd, "--to-ports", "1024-65535", NULL);
> + if (protocol && protocol[0])
> + portStr = ":1024-65535";
> +
> + if (addrEndStr && addrEndStr[0]) {
> + r = virAsprintf(&tmpStr, "%s-%s%s", addrStartStr, addrEndStr,
> + portStr);
> + } else {
> + r = virAsprintf(&tmpStr, "%s%s", addrStartStr, portStr);
> + }
> +
> + if (r < 0) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + 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);
> + VIR_FREE(tmpStr);
You also need to free addrStartStr and addrEndStr (as mentioned in the
review of V1).
Additionally, I just noticed that in the case of an error that is caught
after virCommandNew(), but prior to iptablesCommandRunAndFree(), we will
be leaking one virCommand. So I'm eliminating the call to
iptablesCommandRunAndFree() and just calling virCommandRun directly,
with virCommandFree() added after the cleanup label.
> return ret;
> }
>
> @@ -863,9 +904,11 @@ iptablesAddForwardMasquerade(iptablesContext *ctx,
> virSocketAddr *netaddr,
> unsigned int prefix,
> const char *physdev,
> + virSocketAddr *addrStart,
> + virSocketAddr *addrEnd,
> const char *protocol)
> {
> - return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, protocol, ADD);
> + return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, addrStart, addrEnd, protocol, ADD);
> }
>
> /**
> @@ -886,9 +929,11 @@ iptablesRemoveForwardMasquerade(iptablesContext *ctx,
> virSocketAddr *netaddr,
> unsigned int prefix,
> const char *physdev,
> + virSocketAddr *addrStart,
> + virSocketAddr *addrEnd,
> const char *protocol)
> {
> - return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, protocol, REMOVE);
> + return iptablesForwardMasquerade(ctx, netaddr, prefix, physdev, addrStart, addrEnd, protocol, REMOVE);
> }
>
>
> diff --git a/src/util/viriptables.h b/src/util/viriptables.h
> index d7fa731..05362da 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 *addrStart,
> + virSocketAddr *addrEnd,
> const char *protocol);
> int iptablesRemoveForwardMasquerade (iptablesContext *ctx,
> virSocketAddr *netaddr,
> unsigned int prefix,
> const char *physdev,
> + virSocketAddr *addrStart,
> + virSocketAddr *addrEnd,
> const char *protocol);
> int iptablesAddOutputFixUdpChecksum (iptablesContext *ctx,
> const char *iface,
ACK with the nits I pointed out above. I made those changes and pushed
the result.
Thanks for the contribution!
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list