[feature] OpenWrt: Added support for new bridge syntax #196 #222

pandafy · 2022-03-15T14:07:14Z

netjsonconfig renders and parses configuration for bridges
in both old and new syntax.
Added STP and IGMP options in OpenWrt bridge interface schema

Closes #196

coveralls · 2022-03-15T14:09:36Z

Coverage increased (+0.03%) to 99.629% when pulling 5f94e92 on issues/196-bridge-syntax into abc136b on master.

nemesifier

Good progress!

I see that macaddr is missing, it also has to be added to the new bridge syntax because it's a layer 2 configuration.

Please also ensure the coverage does not decrease.

netjsonconfig/backends/openwrt/converters/interfaces.py

nemesifier

@okraits @nickberry17 this is a delicate one.

We'll be testing it in the next 2 weeks, I suggest you to do the same.

docs/source/backends/openwrt.rst

nemesifier · 2022-03-16T12:31:52Z

docs/source/backends/openwrt.rst

@@ -595,6 +632,14 @@ Will be rendered as follows::
            option ifname 'eth0.2'
            option proto 'none'

+    config device 'device_lan_bridge'
+            option igmp_snooping '1'
+            option name 'lan_bridge'


the name should be br-lan_bridge. Let's simplify the example and call it just lan, so it will be called br-lan.

docs/source/backends/openwrt.rst

tests/openwrt/test_wireless.py

nemesifier

Great progress @pandafy!

Can you add a test for:

{
  "interfaces": [
    {
      "name": "wan",
      "type": "ethernet",
      "macaddr": "00:11:22:33:44:55",
      "addresses": [
        {
          "proto": "dhcp",
          "family": "ipv4"
        }
      ]
    }
  ]
}

Which should be translated to:

package network

config device 'device_wan'
      option name 'wan'
      option macaddr '00:11:22:33:44:55'

config interface 'wan'
	option ifname 'wan'
        option device 'wan'
	option macaddr '00:11:22:33:44:55'
	option proto 'dhcp'

We discussed this, that L2 options need to be defined like this also for non bridge interfaces. We need to do this because if the local OpenWrt config has the device definition OpenWISP won't be able to set the L2 options for that interface.

nemesifier · 2022-03-18T20:34:22Z

docs/source/backends/openwrt.rst

+=============================+=========+===========+=============================================================+
+| ``bridge_members``          | list    | ``[]``    | list of interface names for creating bridge                 |
+-----------------------------+---------+-----------+-------------------------------------------------------------+
+| ``igmp_snooping``           | boolean | ``True``  | sets the ``multicast_snooping`` kernel setting for a bridge |


the default is wrong

nemesifier · 2022-03-18T20:34:38Z

docs/source/backends/openwrt.rst

+-----------------------------+---------+-----------+-------------------------------------------------------------+
+| ``igmp_snooping``           | boolean | ``True``  | sets the ``multicast_snooping`` kernel setting for a bridge |
+-----------------------------+---------+-----------+-------------------------------------------------------------+
+| ``multicast_querier``       | boolean | ``True``  | enables the bridge as a multicast querier                   |


the default is wrong

nemesifier · 2022-03-18T20:35:56Z

docs/source/backends/openwrt.rst

+|                             |         |           | entries in the filtering DB" (range 10-1000000)             |
+-----------------------------+---------+-----------+-------------------------------------------------------------+
+| ``vlan_filtering``          | boolean | ``False`` | enables VLAN aware bridge mode                              |
+-----------------------------+---------+-----------+-------------------------------------------------------------+


you forgot to add max_age

nemesifier · 2022-03-18T20:36:44Z

netjsonconfig/backends/openwrt/schema.py

@@ -155,8 +155,123 @@
                                "description": "sets the \"multicast_snooping\" kernel setting for a bridge",
                                "default": True,


the default has been wrong since the start

nemesifier

This should be fine, I'll do a bit more testing before merging.

nemesifier

I have been testing the result on OpenWrt 21 and its interaction with LuCi, I found a weird issue.

LuCi always wants to migrate the conf, in doing so, it mistakenly create a duplicate device, eg:

# original
config device 'device_lan'            
        option ageing_time '600'      
        option forward_delay '4'               
        option hello_time '4'         
        option max_age '20'           
        option name 'br-lan'          
        option mtu '1500'             
        list ports 'lan1'             
        list ports 'lan2'             
        list ports 'wan'              
        option priority '4001'        
        option stp '1'                
        option type 'bridge'          
        option vlan_filtering '0'

# luci's duplicate
config device              
        option name 'br-lan' 
        option type 'bridge'      
        list ports 'lan1'                      
        list ports 'lan2'  
        list ports 'wan'     
        option mtu '1500'

This means mainly one thing: if we deploy this as is we'll go through HELL and we will regret it greatly. Many users do use LuCi to manage their routers, so when they'll log in LuCi, LuCi will force this migration, creating dupes and this will potentially generate issues that are now impossible to foresee.

I have double checked that if we generate and send the new syntax only, this issue doens't happen, eg:

config device 'device_lan'
	option ageing_time '600'
	option forward_delay '4'
	option hello_time '4'
	option max_age '20'
	option mtu '1500'
	option name 'br-lan'
	list ports 'lan1'
	list ports 'lan2'
	list ports 'wan'
	option priority '4001'
	option stp '1'
	option type 'bridge'
	option vlan_filtering '0'

config interface 'lan'
	option auto '1'
	option device 'br-lan'
	option enabled '1'
	option ip6assign '60'
	option proto 'dhcp'

I have been exploring possible solutions.

I still think that creating a new backend for OpenWrt >= 21 is not good for us because maintaining the duplicated converters and docs doesn't justify the effort.

A possible solution is to add an attribute in the backend which tells the code whether it should work in OpenWrt 21 or greater mode or in legacy mode.
This would allow to avoid the double configuration which would cause issues on OpenWrt 21, moreover, we can avoid generating the new syntax on older systems which has the benefit to avoid touching them when we'll deploy the new version of this library, which is definitely a good thing.

We can default this attribute to be OpenWrt 21 or greater.
We can update the docs to show the OpenWrt 21 syntax only, we should mention the attribute which allows the library to work in legacy mode and suggest looking at the docs of previous stable versions of the library for a reference of the generated syntax pre OpenWrt 21.

In OpenWISP controller we have the operating system info of the device and therefore we are able to know whether a specific device is running openwrt >= 21 or not, therefore, if the OpenWrt backend is being used, when instantiating the it we can pass the right attribute according to the Device.os field.
Created issue: openwisp/openwisp-controller#618

In the legacy netjsonconfig.OpenWisp backend instead, this attribute must default to be the legacy option.

See also my comment below.

netjsonconfig/backends/openwrt/converters/interfaces.py

nemesifier · 2022-03-19T17:51:22Z

netjsonconfig/backends/openwrt/schema.py

+                                "description": "enables VLAN aware bridge mode",
+                                "default": False,
+                                "format": "checkbox",
+                                "propertyOrder": 5,


my understanding is that vlan_filtering is used for defining VLANs in bridges, so it may make sense to add this when dealing with #195, therefore I think it can be removed here.

@codesankalp please address this request (also remove it from the docs please).

tests/openwrt/test_interfaces.py

codesankalp · 2022-03-22T18:10:42Z

Updated tests by keeping these points in mind (as mentioned in openwrt docs):

in config interface, option ifname has been renamed to device (since it refers to a device section)
in config device of type bridge, ifname has been renamed to ports
for new installs, the generated configuration now creates separate sections for layer 2 (config device) and layer 3 (config interface) configuration

tests/openwrt/test_interfaces_dsa.py

codesankalp · 2022-03-27T11:07:22Z

tests/openwrt/test_interfaces_dsa.py

@@ -1135,7 +1136,7 @@ def test_parse_l2_options_bridge(self):
            {
                "name": "wan",
                "type": "ethernet",
-                "macaddr": "00:11:22:33:44:55",
+                "mac": "00:11:22:33:44:55",


This changes doesn't affect the new syntax.

codesankalp · 2022-03-28T13:18:19Z

tests/openwrt/test_interfaces_dsa.py

 config device 'device_custom_if0'
    option name 'custom_if0'

 config interface 'custom_if0'
-    option device '/dev/usb/modem1'


if two devices are present then only one is present in to_netjson_loop and i didn't edit the _get_uci_blocks function to pass devices in list because it will raise validation error according to schema.

https://forum.openwrt.org/t/interface-with-two-device-and-custom-proto-collides-in-openwrt-21-syntax/123539

nemesifier · 2022-03-28T23:49:17Z

netjsonconfig/backends/openwrt/schema.py

+                                "description": "enables VLAN aware bridge mode",
+                                "default": False,
+                                "format": "checkbox",
+                                "propertyOrder": 5,


@codesankalp please address this request (also remove it from the docs please).

- netjsonconfig renders and parses configuration for bridges in both old and new syntax. - Added STP and IGMP options in OpenWrt bridge interface schema Closes #196

netjsonconfig/backends/openwrt/converters/interfaces.py

tests/openwrt/test_dialup.py

nemesifier

We are testing it with openwisp/openwisp-controller#621.

nemesifier

There's some issues I found below, I will fix most of those except the seemingly mistaken test which I am puzzled why is giving that result, @codesankalp @pandafy any idea?

nemesifier · 2022-04-08T21:40:25Z

tests/openwrt/test_wireless.py

@@ -179,8 +179,11 @@ def test_parse_multiple_wifi(self):
    }
    _wifi_bridge_uci = """package network

+config device 'device_eth0_1'


This does not look right.
There's a bridge here which does not generate the new device config type, why is that?

The previous set_dsa method was changing the self.dsa property for the whole block. So when conversion was happening, self.dsa was False for bridge.
Fixed this in 5f94e92

netjsonconfig/backends/openwrt/converters/interfaces.py

netjsonconfig/backends/openwrt/converters/wireless.py

…g dsa property

codesankalp · 2022-04-09T15:11:32Z

netjsonconfig/backends/openwrt/converters/interfaces.py

        """
-        self.dsa = (
+        self.dsa_interface = (


I had to create new property instead of using it as a function because while parsing the proto or type can be popped which can give errors.

nemesifier

I think we are ready.

pandafy marked this pull request as draft March 15, 2022 14:07

pandafy force-pushed the issues/196-bridge-syntax branch from 7929e7e to 8d22152 Compare March 15, 2022 14:09

nemesifier requested changes Mar 15, 2022

View reviewed changes

netjsonconfig/backends/openwrt/converters/interfaces.py Outdated Show resolved Hide resolved

netjsonconfig/backends/openwrt/converters/interfaces.py Outdated Show resolved Hide resolved

pandafy force-pushed the issues/196-bridge-syntax branch 2 times, most recently from 495f0f8 to 511628f Compare March 15, 2022 18:37

nemesifier reviewed Mar 15, 2022

View reviewed changes

pandafy commented Mar 16, 2022

View reviewed changes

docs/source/backends/openwrt.rst Show resolved Hide resolved

nemesifier requested changes Mar 16, 2022

View reviewed changes

nemesifier reviewed Mar 16, 2022

View reviewed changes

tests/openwrt/test_wireless.py Outdated Show resolved Hide resolved

nemesifier marked this pull request as ready for review March 16, 2022 23:58

nemesifier requested changes Mar 17, 2022

View reviewed changes

nemesifier reviewed Mar 18, 2022

View reviewed changes

nemesifier requested changes Mar 19, 2022

View reviewed changes

nemesifier requested changes Mar 22, 2022

View reviewed changes

tests/openwrt/test_interfaces_dsa.py Show resolved Hide resolved

nemesifier requested changes Mar 25, 2022

View reviewed changes

tests/openwrt/test_interfaces_dsa.py Outdated Show resolved Hide resolved

codesankalp force-pushed the issues/196-bridge-syntax branch from 1d52d54 to 8ac49b2 Compare March 26, 2022 09:37

codesankalp reviewed Mar 27, 2022

View reviewed changes

codesankalp force-pushed the issues/196-bridge-syntax branch from 75a6bdb to 96ecb9e Compare March 27, 2022 11:10

codesankalp reviewed Mar 28, 2022

View reviewed changes

nemesifier requested changes Mar 28, 2022

View reviewed changes

codesankalp force-pushed the issues/196-bridge-syntax branch 2 times, most recently from 7bf4bf2 to 8f10232 Compare March 30, 2022 11:46

pandafy added 4 commits March 30, 2022 17:27

[feature] OpenWrt: Added support for new bridge syntax #196

2afd872

- netjsonconfig renders and parses configuration for bridges in both old and new syntax. - Added STP and IGMP options in OpenWrt bridge interface schema Closes #196

[requested-changes] Added docs and other fixes

2f1f0eb

[requested-change] Added device option to bridge interface

4414f3a

[requested-change] Added L2 options in device config

8b2daf9

codesankalp added 8 commits March 30, 2022 17:27

[change] Added dsa in OpenWRT init method and separated tests

e1b2d9a

[tests] Changed tests according to new syntax

e7a6020

[tests] Added device definition for each interface

60a6ed2

[change] OpenWrt converter changed to generate new syntax

de88970

[change] OpenWrt parser changed to parse new syntax

5928051

[tests] Removed one device from custom proto

6eed4e3

[change] Removed vlan_filtering option

361bb93

[change] Render and parse custom proto

c94c130

codesankalp force-pushed the issues/196-bridge-syntax branch from 8f10232 to c94c130 Compare March 30, 2022 11:57

nemesifier requested changes Mar 30, 2022

View reviewed changes

netjsonconfig/backends/openwrt/converters/interfaces.py Show resolved Hide resolved

tests/openwrt/test_dialup.py Outdated Show resolved Hide resolved

[change] Added more l2 options

93bf9f0

codesankalp force-pushed the issues/196-bridge-syntax branch from bbd8b9f to 8ddcd27 Compare March 31, 2022 15:47

[change] New syntax should apply to loopback, ethernet and bridge

53371b9

codesankalp force-pushed the issues/196-bridge-syntax branch from 8ddcd27 to 53371b9 Compare March 31, 2022 15:54

codesankalp requested a review from nemesifier March 31, 2022 15:59

nemesifier reviewed Apr 5, 2022

View reviewed changes

[minor] Updated docs and refactored code

7c566ab

pandafy force-pushed the issues/196-bridge-syntax branch 2 times, most recently from 8e06988 to 0e6b8f9 Compare April 6, 2022 21:16

[requested-chages] Updated docs and Openwisp backend default dsa

5a7c128

pandafy force-pushed the issues/196-bridge-syntax branch from 0e6b8f9 to 5a7c128 Compare April 6, 2022 21:20

nemesifier requested changes Apr 8, 2022

View reviewed changes

nemesifier and others added 4 commits April 8, 2022 17:43

[chores] Removed unneeded rounded parenthesis

d2ff699

[fix] Avoided noqa: C901 on _track_bridged_wifi

3edfee2

[chores] Fixed typo in comment of OpenWrtParser

03c60b1

[change] Set dsa_interface property for each block instead of changin…

5f94e92

…g dsa property

codesankalp reviewed Apr 9, 2022

View reviewed changes

nemesifier approved these changes Apr 11, 2022

View reviewed changes

nemesifier merged commit fbe5b8e into master Apr 11, 2022

nemesifier deleted the issues/196-bridge-syntax branch November 29, 2022 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] OpenWrt: Added support for new bridge syntax #196 #222

[feature] OpenWrt: Added support for new bridge syntax #196 #222

pandafy commented Mar 15, 2022

coveralls commented Mar 15, 2022 •

edited

Loading

nemesifier left a comment

nemesifier left a comment

nemesifier Mar 16, 2022

nemesifier left a comment

nemesifier Mar 18, 2022

nemesifier Mar 18, 2022

nemesifier Mar 18, 2022

nemesifier Mar 18, 2022

nemesifier left a comment

nemesifier left a comment •

edited

Loading

nemesifier Mar 19, 2022

nemesifier Mar 28, 2022

codesankalp commented Mar 22, 2022

codesankalp Mar 27, 2022

codesankalp Mar 28, 2022

nemesifier Mar 28, 2022

nemesifier left a comment

nemesifier left a comment

nemesifier Apr 8, 2022

codesankalp Apr 9, 2022

codesankalp Apr 9, 2022

nemesifier left a comment

		@@ -155,8 +155,123 @@
		"description": "sets the \"multicast_snooping\" kernel setting for a bridge",
		"default": True,

[feature] OpenWrt: Added support for new bridge syntax #196 #222

[feature] OpenWrt: Added support for new bridge syntax #196 #222

Conversation

pandafy commented Mar 15, 2022

coveralls commented Mar 15, 2022 • edited Loading

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codesankalp commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

coveralls commented Mar 15, 2022 •

edited

Loading

nemesifier left a comment •

edited

Loading