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

[Alexandre DERUMIER]

> We need to keep classic ifupdown behaviour 

>>is there any upstream issue/discussion regarding this? 

yes (I'm always trying to push upstream)

https://github.com/CumulusNetworks/ifupdown2/issues/98

and PR
https://github.com/CumulusNetworks/ifupdown2/pull/101



----- Mail original -----
De: "Thomas Lamprecht" <t.lamprecht at proxmox.com>
À: "pve-devel" <pve-devel at pve.proxmox.com>, "aderumier" <aderumier at odiso.com>
Envoyé: Mercredi 8 Mai 2019 14:28:44
Objet: Re: [pve-devel] [PATCH ifupdown2 2/3] fix ipforwarding

On 5/6/19 6:25 AM, Alexandre Derumier 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 

is there any upstream issue/discussion regarding this? 

> 
> 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')) 

why not just change the default? I.e., 

----8<---- 
diff --git a/ifupdown2/addons/address.py b/ifupdown2/addons/address.py 
index d3a76cf..96afcb6 100644 
--- a/ifupdown2/addons/address.py 
+++ b/ifupdown2/addons/address.py 
@@ -99,12 +99,12 @@ class address(moduleBase): 
'ip-forward' : 
{ 'help': 'ip forwarding flag', 
'validvals': ['on', 'off', 'yes', 'no', '0', '1'], 
- 'default' : 'off', 
+ 'default' : 'on', 
'example' : ['ip-forward off']}, 
'ip6-forward' : 
{ 'help': 'ipv6 forwarding flag', 
'validvals': ['on', 'off', 'yes', 'no', '0', '1'], 
- 'default' : 'off', 
+ 'default' : 'on', 
'example' : ['ip6-forward off']}, 
'mpls-enable' : 
{ 'help': 'mpls enable flag', 
-- 

> +- 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: 
> +- if not setting_default_value: 
> ++ 
> ++ if ipforward: 
> ++ 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) 

hmm, why not changing it in the definition of _set_bridge_forwarding ? 
So forwarding would still get enabled if it was disabled in the other case, 
where addresses are configured. 

> + 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 
>