[pve-devel] [PATCH ifupdown2 2/3] fix ipforwarding

Alexandre DERUMIER aderumier at odiso.com
Thu May 9 09:06:29 CEST 2019


>>to:
>>if (ipforward or self.ipforward):
>>
>>as a small suggestion to your pull-request upstream.

Oh, didn't notice that. 
Thanks !


----- Mail original -----
De: "Stoiko Ivanov" <s.ivanov at proxmox.com>
À: "pve-devel" <pve-devel at pve.proxmox.com>
Envoyé: Mercredi 8 Mai 2019 18:16:50
Objet: Re: [pve-devel] [PATCH ifupdown2 2/3] fix ipforwarding

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 


_______________________________________________ 
pve-devel mailing list 
pve-devel at pve.proxmox.com 
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 




More information about the pve-devel mailing list