[WIP] Openwrt firewall

[jonathanunderwood]

This builds on #116 to complete the work needed. The parser and renderers are largely incomplete, and there aren't really any tests of note. So, breaking the work down:

• [x] Merge master so that this branch will merge cleanly
• [x] Add rules parser and tests
• [x] Add schema for missing rules parameters
• [x] Add parser, renderer and tests for missing rules parameters
• [x] Add zones parser and tests
• [x] Add firewall defaults parser and tests
• [x] Add forwardings parser and tests
• [x] Add missing name and enabled fields to forwarding
• [x] Add redirect schema
• [x] Add redirect renderer, parser and tests
• [x] Add includes schema, parser, renderer and tests
• [ ] Define and use a network regex throughout schema
• [ ] Review test coverage, ensure ValidationError tests are included
• [ ] Review schema for opportunities to restrict values
• [ ] Check that all types have name and enabled fields which are tested
• [ ] Add documentation
• [ ] Review required fields for all objects for consistency with upstream (ensure name is required everywhere except defaults)

Note: this reference document is the source of truth for OpenWRT firewall configuration parameters.

[coveralls]

Coverage decreased (-0.2%) to 99.705% when pulling 8c6ac92739973f3ccafde00760b55c5f9a9e14c0 on openwrt-firewall into 0b598e193f92a87684fe1d99c9a8a42f1dd729a1 on master.

[jonathanunderwood]

@nemesisdesign How do you recommend dealing with a UCI parameter that can either be a string or a list? One such example is the proto parameter in a firewall rule. Both of the following rules are legitimate:

config rule 'rule_Allow_Ping'
    option name 'Allow-Ping'
    option src 'wan'
    option proto 'icmp'
    option family 'ipv4'
    list icmp_type 'echo-request'
    option target 'ACCEPT'
    option enabled '0'

config rule 'rule_Allow_Isolated_DHCP'
    option name 'Allow-Isolated-DHCP'
    option src 'isolated'
    list proto 'udp'
    option dest_port '67-68'
    option target 'ACCEPT'

At present, in the schema, this parameter is specified as a string. One option would be to define it to be an array, and always co-erce it to a list when parsing. But perhaps there are other options/precedent?

[jonathanunderwood]

@nemesisdesign How do you recommend dealing with a UCI parameter that can either be a string or a list? One such example is the proto parameter in a firewall rule. Both of the following rules are legitimate: [snip] At present, in the schema, this parameter is specified as a string. One option would be to define it to be an array, and always co-erce it to a list when parsing. But perhaps there are other options/precedent?

For now, I have gone with this approach.

[nemesifier]

@nemesisdesign How do you recommend dealing with a UCI parameter that can either be a string or a list? One such example is the proto parameter in a firewall rule. Both of the following rules are legitimate: [snip] At present, in the schema, this parameter is specified as a string. One option would be to define it to be an array, and always co-erce it to a list when parsing. But perhaps there are other options/precedent?

For now, I have gone with this approach.

Yes that should work!

[jonathanunderwood]

@nemesisdesign currently qa checks are failing with this:

./netjsonconfig/backends/openwrt/converters/firewall.py:200:5: C901 'Firewall.__netjson_redirect' is too complex (14)

I could split __netjson_redirect() up, but it seems like a pointless exercise, since none of the functions that resulted would be re-used. Perhaps we should increase the complexity threshold?

[jonathanunderwood]

@nemesisdesign currently qa checks are failing with this:

./netjsonconfig/backends/openwrt/converters/firewall.py:200:5: C901 'Firewall.__netjson_redirect' is too complex (14)

I could split __netjson_redirect() up, but it seems like a pointless exercise, since none of the functions that resulted would be re-used. Perhaps we should increase the complexity threshold?

Have refactored and reduced complexity sufficiently.

[jonathanunderwood]

Thanks @jonathanunderwood! I'm looking to find a chance to test your work this week.

Great! There's still a LOT of work to do, though. Test coverage is currently incomplete, the schema is incomplete, the parsers and renders are incomplete. That all said, would be great to review what's there so I can build in feedback moving forward.

[TravisBuddy]

Travis tests have failed

Hey @jonathanunderwood, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

View build log

./run-qa-checks

SUCCESS: Blank endline check successful!
Skipped 1 files
SUCCESS: Isort check successful!
--- netjsonconfig/backends/openwrt/schema.py	2020-10-16 15:15:55.636319 +0000
+++ netjsonconfig/backends/openwrt/schema.py	2020-10-16 15:16:22.174998 +0000
@@ -958,11 +958,11 @@
         "default": "10/minute",
         "propertyOrder": 11,
     },
     "device": {
         "type": "array",
-        "title":  "Raw devices to attach to this zone.",
+        "title": "Raw devices to attach to this zone.",
         "description": "A list of raw device names to associate with this zone. ",
         "items": {
             "type": "string",
             "title": "A device to attach to the zone.",
             "description": "A device to attach to the zone."
would reformat netjsonconfig/backends/openwrt/schema.py
Oh no! 💥 💔 💥
1 file would be reformatted, 70 files would be left unchanged.
ERROR: Black check failed! Hint: did you forget running openwisp-qa-format?
./netjsonconfig/backends/openwrt/schema.py:963:17: E241 multiple spaces after ':'
ERROR: Flake8 check failed!
SUCCESS: Commit message check successful!
File manage.py not found, skipping Make Migration Check.

TravisBuddy Request Identifier: 9b726690-0fc2-11eb-8acb-27a1e4e1eb88

[TravisBuddy]

Travis tests have failed

Hey @jonathanunderwood, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

View build log

./run-qa-checks

SUCCESS: Blank endline check successful!
Skipped 1 files
SUCCESS: Isort check successful!
All done! ✨ 🍰 ✨
71 files would be left unchanged.
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Your commit message does not follow our commit message style guidelines:

- missing prefix in the commit short description
  Eg: "[feature/fix/change] Action performed"

Please read our guidelines at: http://openwisp.io/docs/developer/contributing.html#commit-message-style-guidelines
Checked commit message:


Fix formatting error


ERROR: Commit message check failed!
File manage.py not found, skipping Make Migration Check.

TravisBuddy Request Identifier: 81b96240-0fcb-11eb-8acb-27a1e4e1eb88

[TravisBuddy]

Travis tests have failed

Hey @jonathanunderwood, Please read the following log in order to understand the failure reason. It'll be awesome if you fix what's wrong and commit the changes.

2nd Build

View build log

./run-qa-checks

SUCCESS: Blank endline check successful!
Skipped 1 files
SUCCESS: Isort check successful!
All done! ✨ 🍰 ✨
71 files would be left unchanged.
SUCCESS: Black check successful!
SUCCESS: Flake8 check successful!
Your commit message does not follow our commit message style guidelines:

- please add a capital letter after the prefix

Please read our guidelines at: http://openwisp.io/docs/developer/contributing.html#commit-message-style-guidelines
Checked commit message:


[openwrt] remove debugging print statements


ERROR: Commit message check failed!
File manage.py not found, skipping Make Migration Check.

TravisBuddy Request Identifier: 3e0149c0-0fd3-11eb-8acb-27a1e4e1eb88

[nemesifier]

@jonathanunderwood here's the contributing guidelines. I'll make sure to make it more evident in this repo. Thank you for your patience.

[nemesifier]

Haven't had time to dedicate to this yet but I'll surely will at some point in the future and help finish it. Thanks @okraits @jonathanunderwood.

[jonathanunderwood]

@nemesisdesign any idea what's going on with the test execution - travis/coveralls seems to have been waiting forever?

[nemesifier]

@jonathanunderwood travis CI does not offer free plans for open source anymore and they're not renewing our OSS credits, we're moving to all the repositories of OpenWISP to Github actions #178, but it's taking time to do it for all the modules.

You can run tests locally for now, when it's ready for review or need feedback let us know.

[ValerioBob]

Is there any intention to carry on this branch?

[ValerioBob]

@nemesisdesign I find this feature very useful and I want to contribute. If it is ok I can spend some of my free time on to carry on this work. I managed to merge it with the master branch: the test coverage is ok but the schema has some problems when used by the frontend, more specifically using 'allOf' and '$ref'. Replacing the '$ref's with the corresponding fragments fixed it, so it's just written in a wrong way. Please let me know if I can contribute to this branch and any type of information that can be useful

[jonathanunderwood]

@ValerioBob Sadly I ran out of time to work on this back in 2021. There's actually not that much left to do, so please feel free to dig in and add commits. I'll try and find some time to come back to it in the coming weeks as well.

[ValerioBob]

@jonathanunderwood Hi! It would be awesome! Rn I don't have so much time but I will in some weeks. On my repositories you can find a fork with my contributions. As I remember with the Firewall I was almost done