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

Thomas Lamprecht t.lamprecht at proxmox.com
Wed May 8 14:28:44 CEST 2019


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
> 





More information about the pve-devel mailing list