[pve-devel] [PATCH ifupdown2 2/3] fix ipforwarding
Stoiko Ivanov
s.ivanov at proxmox.com
Wed May 8 18:16:50 CEST 2019
On Mon, 6 May 2019 06:25:00 +0200
Alexandre Derumier <aderumier at odiso.com> wrote:
> Currently ifupdown2 disable forwarding if not defined with ip-forward
> on interfaces, or if bridge don't have an address (breaking inet dhcp)
>
> We need to keep classic ifupdown behaviour
>
> Signed-off-by: Alexandre Derumier <aderumier at odiso.com>
> ---
> ...e-ipforward-interface-value-if-not-define.patch | 100
> +++++++++++++++++++++ ...autoconfiguration-of-forwarding-of-bridge.patch
> | 31 +++++++ debian/patches/series |
> 2 + 3 files changed, 133 insertions(+)
> create mode 100644
> debian/patches/pve/0006-don-t-change-ipforward-interface-value-if-not-define.patch
> create mode 100644
> debian/patches/pve/0007-disable-autoconfiguration-of-forwarding-of-bridge.patch
>
> diff --git
> a/debian/patches/pve/0006-don-t-change-ipforward-interface-value-if-not-define.patch
> b/debian/patches/pve/0006-don-t-change-ipforward-interface-value-if-not-define.patch
> new file mode 100644 index 0000000..facd462 --- /dev/null
> +++
> b/debian/patches/pve/0006-don-t-change-ipforward-interface-value-if-not-define.patch
> @@ -0,0 +1,100 @@ +From 053252ec9cc5f60db1cdcb44800690708bb9c11b Mon
> Sep 17 00:00:00 2001 +From: Alexandre Derumier <aderumier at odiso.com>
> +Date: Mon, 6 May 2019 06:04:48 +0200
> +Subject: [PATCH] don't change ipforward interface value if not
> defined +
> +Signed-off-by: Alexandre Derumier <aderumier at odiso.com>
> +---
> + ifupdown2/addons/address.py | 73
> ++++++++++++++++++++-------------------------
> + 1 file changed, 33 insertions(+), 40 deletions(-)
> +
> +diff --git a/ifupdown2/addons/address.py
> b/ifupdown2/addons/address.py +index d3a76cf..a2f86f3 100644
> +--- a/ifupdown2/addons/address.py
> ++++ b/ifupdown2/addons/address.py
> +@@ -715,49 +715,42 @@ class address(moduleBase):
> + self.log_error('%s: \'ip6-forward\' is not
> supported for '
> + 'bridge port' %ifaceobj.name)
> + return
> +- setting_default_value = False
> +- if not ipforward:
> +- setting_default_value = True
> +- ipforward = (self.ipforward or
> +- self.get_mod_subattr('ip-forward',
> 'default')) +- ipforward =
if I understood the code correctly this deletion removes the
possibility to override the forwarding by setting a policy
via /etc/network/ifupdown2/policy.d/*json. While we probably won't be
using this mechanism, I guess some of our users will eventually pick
this up and then wonder - I couldn't find too much
documentation on the policies online - but with [0] and the source, the
following json seems to work:
```
{
"address": {
"defaults": {"ip-forward" : "off",
"ip6-forward" : "off"
}
}
}
```
Unless the policy.d snippets are not meant to be used this is probably
easiest fixed with changing the if below
> utils.boolean_support_binary(ipforward) +- # File read has
> been used for better performance +- # instead of using sysctl
> command +- running_ipforward = self.read_file_oneline(
> +-
> '/proc/sys/net/ipv4/conf/%s/forwarding'
> +- %ifaceobj.name) +- if
> ipforward != running_ipforward: +- try:
> +- self.sysctl_set('net.ipv4.conf.%s.forwarding'
> +-
> %('/'.join(ifaceobj.name.split("."))),
> +- ipforward) +- except
> Exception as e: +- if not setting_default_value:
> ++
> ++ if ipforward:
to:
if (ipforward or self.ipforward):
as a small suggestion to your pull-request upstream.
> ++ ipforward = utils.boolean_support_binary(ipforward)
> ++ # File read has been used for better performance
> ++ # instead of using sysctl command
> ++ running_ipforward = self.read_file_oneline(
> ++
> '/proc/sys/net/ipv4/conf/%s/forwarding'
> ++ %ifaceobj.name) ++
> ++ if ipforward != running_ipforward:
> ++ try:
> ++
> ++ self.sysctl_set('net.ipv4.conf.%s.forwarding'
> ++
> %('/'.join(ifaceobj.name.split("."))),
> ++ ipforward) ++
> except Exception as e:
> + ifaceobj.status = ifaceStatus.ERROR
> + self.logger.error('%s: %s' %(ifaceobj.name,
> str(e)))
> +
> +- setting_default_value = False
> +- if not ip6forward:
> +- setting_default_value = True
> +- ip6forward = (self.ip6forward or
> +- self.get_mod_subattr('ip6-forward',
> 'default')) +- ip6forward =
> utils.boolean_support_binary(ip6forward) +- # File read has
> been used for better performance +- # instead of using sysctl
> command +- running_ip6forward = self.read_file_oneline(
> +-
> '/proc/sys/net/ipv6/conf/%s/forwarding'
> +- %ifaceobj.name) +- if
> ip6forward != running_ip6forward: +- try:
> +- self.sysctl_set('net.ipv6.conf.%s.forwarding'
> +-
> %('/'.join(ifaceobj.name.split("."))),
> +- ip6forward) +- except
> Exception as e: +- # There is chance of ipv6 being
> removed because of, +- # for example, setting mtu <
> 1280 +- # In such cases, log error only if user has
> configured +- # ip6-forward
> +- if not setting_default_value:
> ++ if ip6forward:
> ++ ip6forward = utils.boolean_support_binary(ip6forward)
> ++ # File read has been used for better performance
> ++ # instead of using sysctl command
> ++ running_ip6forward = self.read_file_oneline(
> ++
> '/proc/sys/net/ipv6/conf/%s/forwarding'
> ++ %ifaceobj.name) ++
> if ip6forward != running_ip6forward: ++ try:
> ++ self.sysctl_set('net.ipv6.conf.%s.forwarding'
> ++
> %('/'.join(ifaceobj.name.split("."))),
> ++ ip6forward) ++
> except Exception as e: ++ # There is chance of
> ipv6 being removed because of, ++ # for example,
> setting mtu < 1280 ++ # In such cases, log error
> only if user has configured ++ # ip6-forward
> + ifaceobj.status = ifaceStatus.ERROR
> + self.logger.error('%s: %s' %(ifaceobj.name,
> str(e)))
> +
> +--
> +2.11.0
> +
> diff --git
> a/debian/patches/pve/0007-disable-autoconfiguration-of-forwarding-of-bridge.patch
> b/debian/patches/pve/0007-disable-autoconfiguration-of-forwarding-of-bridge.patch
> new file mode 100644 index 0000000..c8f67fb --- /dev/null
> +++
> b/debian/patches/pve/0007-disable-autoconfiguration-of-forwarding-of-bridge.patch
> @@ -0,0 +1,31 @@ +From baf59fa021b197fffe9679a15e51a92dd9450e38 Mon
> Sep 17 00:00:00 2001 +From: Alexandre Derumier <aderumier at odiso.com>
> +Date: Mon, 6 May 2019 06:13:07 +0200
> +Subject: [PATCH] disable autoconfiguration of forwarding of bridge.
> +
> +Current ifupdwon2 behaviour of ifupdown2 is to disable forwarding
> +on a bridge if no address is defined.
> +
> +This is breaking dhcp interfaces
> +
> +Signed-off-by: Alexandre Derumier <aderumier at odiso.com>
> +---
> + ifupdown2/addons/address.py | 2 +-
> + 1 file changed, 1 insertion(+), 1 deletion(-)
> +
> +diff --git a/ifupdown2/addons/address.py
> b/ifupdown2/addons/address.py +index a2f86f3..05aaa64 100644
> +--- a/ifupdown2/addons/address.py
> ++++ b/ifupdown2/addons/address.py
> +@@ -690,7 +690,7 @@ class address(moduleBase):
> + self.logger.error('%s: %s' %(ifaceobj.name,
> str(e)))
> +
> + if (ifaceobj.link_kind & ifaceLinkKind.BRIDGE):
> +- self._set_bridge_forwarding(ifaceobj)
> ++ #self._set_bridge_forwarding(ifaceobj)
I'm not sure whether _set_bridge_forwarding does make sense as it is
currently:
* if no 'address' is configured on a bridge is disables forwarding
(which is what you intended to fix)
* if an 'address' is configured it unconditionally enables forwarding
which is not necessary for forwarding on layer2 (which a bridge
does), and maybe not a wanted effect (why should a host route between
2 networks/interfaces, just because one of them happens to be a
bridge)
or am I missing something?
small nit: _set_bridge_forwarding is not used anywhere else - so we
could delete the complete function (unless the patch stays only with us
and is not applied upstream)
> + return
> + if not self.syntax_check_sysctls(ifaceobj):
> + return
> +--
> +2.11.0
> +
> diff --git a/debian/patches/series b/debian/patches/series
> index 5975ab1..e03c55a 100644
> --- a/debian/patches/series
> +++ b/debian/patches/series
> @@ -3,4 +3,6 @@
> pve/0002-don-t-remove-tap-veth-fwpr-interfaces-from-bridge-on.patch
> pve/0003-add-dummy-mtu-bridgevlanport-modules.patch
> pve/0004-debian-fixup-networking.service.patch
> pve/0005-allow-vlan-subinterface-in-a-vlan-aware-bridge.patch
> +pve/0006-don-t-change-ipforward-interface-value-if-not-define.patch
> +pve/0007-disable-autoconfiguration-of-forwarding-of-bridge.patch
> extra/0001-addons-bridge-fix-NoneType-object-has-no-attribute-_.patch
[0]https://github.com/CumulusNetworks/vrf/tree/master/ifupdown2/policy.d
More information about the pve-devel
mailing list