Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Airos in OpenWISP2 #91

Open
wants to merge 342 commits into
base: master
Choose a base branch
from
Open

Add support for Airos in OpenWISP2 #91

wants to merge 342 commits into from

Conversation

edoput
Copy link
Contributor

@edoput edoput commented Jul 15, 2017

This PR is now closed as the work has been included in an external package, netjsonconfig-airos and will be available from the next release (v0.6.3)

this is the airos v8.3 backend as of today

  • tests are ok for both python2 and python3

I will go on with squashing commits where possible to keep size down

Another important things to do is to implement the backward_conversion for the radio converter as of now the radio configuration values are mostly hardware dependents. This may be superseded by an approach where the user writes a very verbose netjson with lots of additional values for the radio object.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment I limited myself to style issues which are usually the first thing noticed when reviewing.

Please see my inline comments, fix the flake8/isort issues and update the PR.
In the meanwhile I'll test with an ubiquiti PowerBeam AC feed.

Fed

]
}

Leaving the `NetJSON Encryption object <http://netjson.org/rfc.html#rfc.section.5.4.2.1>` empty defaults to no encryption at all
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please add a dot . at the end of the line?

}

As a son may be a carrier of a value so we store it in a dictionary instead of adding a *leaf*
with another level of recursion
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing dot at end of the line

As a son may be a carrier of a value so we store it in a dictionary instead of adding a *leaf*
with another level of recursion

As an example here we present the tree `('spam', [ { 'eggs': 2 }, { 'snakes' : { 'loved' : 'python' }}])`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a colon : at the end of the line?


The intermediate representation is the output of the a :ref:`converter`,
it is backend specific and is built as a tree structure made from python
builtins values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing dot at end of the line

A tree is a *acyclic, directional graph* with an element called *root*.

The root of our tree is stored in the first element of a tuple, along with
the root's direct sons as a list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing a colon : at the end of the line?

@@ -0,0 +1,268 @@
from netjsonconfig import AirOS

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the name of the file, can we find a more professional name?

})

o.to_intermediate()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra blank lines everywhere! :-D :-P


expected = [
{
'1.comment': '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check spaces



class TestDiscoveryConverter(ConverterTest):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least this one you may remove it :-P

},
{
'device.1.profile': 'AUTO',
'device.1.status': 'disabled',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check spaces

wpasupplicant_schema,
])
override_schema
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not on a single line? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because I'm mad and I REALLY LOVE my enter key

AirOS specific JSON-Schema definition
"""
from ...schema import schema as default_schema
from ...schema import DEFAULT_FILE_MODE # noqa - backward compatibility
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove this line too, it's not needed here.

@edoput edoput force-pushed the airos branch 3 times, most recently from 1065736 to 25cd7f6 Compare July 20, 2017 11:09
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.2%) to 96.737% when pulling 7c8e27b on airos into a37cdea on master.

@openwisp openwisp deleted a comment from coveralls Jul 20, 2017
@openwisp openwisp deleted a comment from coveralls Jul 20, 2017
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress! 👍

As a side note among other things, try to get higher test coverage

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More inline feedback

'tx': {
'status': 'enabled',
},
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about a more compact version:

base['flowcontrol'] = {
    'rx': {'status': 'enabled'},
    'tx': {'status': 'enabled'},
}

# handle explicit address policy
if addr['proto'] == 'dhcp':
temp['autoip'] = {}
temp['autoip']['status'] = 'enabled'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't an explicit declaration be more compact and readable? Eg:

temp['autoip'] = {'status': 'enabled'}

result = [
{
'community': 'public',
'contact': '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can take this from general.maintainer, see http://netjson.org/rfc.html#general1

{
'community': 'public',
'contact': '',
'location': '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may add general.location to NetJSON in the near future, take this value from there.

def cleanup(self, output):
stripped = [
a.strip() for a in output.splitlines() if a.strip()
]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation looks wrong here

=============

.. include:: ../_github.rst

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove blank line


{
"type": "DeviceConfiguration",
...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove these dots because they are not being recognized as JSON


.. include:: ../_github.rst

The ``AirOS`` backend allows to generate AirOS v8.3 compatible configurations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be AirOs here and on the rest of the document

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @edoput I think you forgot this one

'comment' : 'traffic vlan'
}
]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep only the python example, having both is redundant and doesn't help the user much

'2.devname' : 'eth0',
'2.id' : '2'
'2.status' : 'enabled',
'2.comment' : 'traffic'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like 8 spaces indentation to me, can you check?

'external': {
'reset': 'enabled',
},
'timezone': 'GMT',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll need to implement this soon, it shouldn't have priority right now but we will have to do it in the 3rd phase

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's some more detailed feedback on the configuration front.

I'm testing a configuration I downloaded from a production device and I'm comparing it with a config
I am generating with netjsonconfig.

The NetJSON I am using is:

{
    "general":{
        "hostname": "PBPomeziaDiegoApriliana",
        "timezone": "Europe/Rome"
    },
    "gui": {
        "language": "en_US"
    },
    "netmode": "bridge",
    "interfaces": [
        {
            "name": "ath0",
            "type": "wireless",
            "wireless": {
                "radio": "ath0",
                "mode": "access_point",
                "ssid": "ninux-diego-pomezia",
                "wds": true,
                "bssid": "04:18:D6:F8:01:F7"
            }
        },
        {
            "type": "ethernet",
            "name": "eth0",
            "mtu": 1500,
            "addresses": [
                {
                    "management": true,
                    "address": "10.8.2.71",
                    "gateway": "10.8.2.1",
                    "mask": 24,
                    "family": "ipv4",
                    "proto": "static"
                }
            ]
        },
        {
            "type": "ethernet",
            "name": "eth0.24",
            "mtu": 1500
        },
        {
            "name": "br0",
            "type": "bridge",
            "bridge_members": ["ath0", "eth0.24"]
        }
    ],
    "radios": [
        {
            "name": "ath0",
            "channel": 1,
            "channel_width": 20,
            "disabled": true,
            "protocol": "802.11n",
            "tx_power": -4
        }
    ],
    "ntp_servers": ["193.204.114.233", "193.204.114.234"],
    "dns_servers": ["193.204.5.4", "8.8.8.8"],
    "user": {
        "name": "root",
        "password": "gpO0LLzr9C8L.LKdsqGEf.",
        "salt": "WdnaHPry"
    }
}

Spanning Tree Protocol for bridges

Seems always enabled, eg:

bridge.1.stp.status=enabled

But may also be disabled:

bridge.1.stp.status=disabled

Use the stp attribute of bridge interfaces to set this.

If the attribute is omitted, it should mean disabled.

Can comment be omitted if empty/not present?

I see bridge.1.comment= in my generated config, but this type of attribute was not present
in my test configuration that I downloaded from a production device.

Therefore I think it may be omitted if no comment is present in the NetJSON. What do you think?

ebtables

My production conf doesn't have this section.

Can this section be omitted if not specified in NetJSON?

igmpproxy

My production conf doesn't have this section.

Can this section be omitted if not specified in NetJSON?

iptables

My production conf doesn't have this section.

Can this section be omitted if not specified in NetJSON?

netconf flowcontrol

I get this:

+netconf.2.flowcontrol.tx.status=enabled
+netconf.2.flowcontrol.rx.status=enabled

Even though I don't define any flowcontrol in the NetJSON.

I think flowcontrol can be omitted if not defined in the NetJSON, what do you think?

netconf autoneg

I get this:

+netconf.2.autoneg=enabled

Even though I don't define any autoneg in the NetJSON.

I think autoneg can be omitted if not explicitly defined, what do you think?

I think we can add a negotiation parameter to the schema of ethernet interfaces for this.

ntp

This works, can you use the same NTP schema that is being used by OpenWrt and Raspbian?

That way we can keep differences to the minimum and we may even be able to add this new section to the official NetJSON spec.

You can ignore the enable_server property.

pwdog

I have this situation:

-pwdog.status=disabled
pwdog.status=enabled

Is this feature enabled by default on AirOS?

resolv

I get this situation:

-resolv.nameserver.status=enabled
+resolv.status=enabled

Are we sure this is ok?

Default route

I have the following:

-route.1.devname=eth0
-route.1.gateway=10.8.2.1
-route.1.ip=0.0.0.0
-route.1.netmask=0
-route.1.status=enabled

This is the default route, you can infer this information
from the gateway attribute of an address of an interface,
probably only the management interface.

sshd

I have this situation:

-QYva4bK2zA4faxwtZbtOyQ7x4IYNt5Z+Vv381cM1g2NyGetS3qs0mPXt9JaQRi4aYre7MtSkOOJymwpJj4pPJz8t/H3pVLyi17FAXXWVh1j42C4Mi7IBgtYn0T4GMn0FGx3+dRv2rDrwDaA0guC9GKH8PzHsbrlH1ibewJ5VLFcUzFy6CY14muXTFNb/XyvK0Z8fCU2UR3VILVDotDJfJCOEhhOs0uKLNOeyoApbeS2N
-sshd.auth.key.1.comment=nemesis@freedom
-sshd.auth.key.1.status=enabled
-sshd.auth.key.1.type=ssh-rsa
+sshd.port=22
+sshd.status=enabled
+sshd.auth.passwd=enabled

The added lines should not be an issue, but SSH keys are removed and this can cause annoyances.

In the OpenWrt backend one can add an SSH key by specifying a custom file
with path /etc/dropbear/authorized_keys but I would have liked to introduce
a more specific and more portable way to specify SSH keys.

Since most evolved networking firmware support some kind of SSH configuration,
I think it's time to define a new NetJSON attribute for this and we could start
doing it in this backend.

A way of doing it in the NetJSON style would be:

{
    "sshd": {
        "port": 22,
        "enabled": True,
        "password_auth": True,
        "keys": [
            {
                "type": "ssh-rsa",
                "key": "QYva4bK2zA4faxwtZbtOyQ7x4IYNt5Z+Vv381cM1g2NyGetS3qs0mPXt9JaQRi4aYre7MtSkOOJymwpJj4pPJz8t/H3pVLyi17FAXXWVh1j42C4Mi7IBgtYn0T4GMn0FGx3+dRv2rDrwDaA0guC9GKH8PzHsbrlH1ibewJ5VLFcUzFy6CY14muXTFNb/XyvK0Z8fCU2UR3VILVDotDJfJCOEhhOs0uKLNOeyoApbeS2N",
                "comment": "nemesis@freedom",
                "enabled": True
            }
        ]
    }
}

Could you create an additional seciton in the airos schema for this and update the Sshd
converter accordingly?

Wireless ap attribute (bssid)

I have:

-wireless.1.ap=04:18:D6:F8:01:F7

But this attribute is needed, you can use the bssid attribute in
NetJSON wireless interfaces.

WPA supplicant

I have this situation:

-wpasupplicant.device.1.status=disabled
+wpasupplicant.status=disabled
+wpasupplicant.profile.1.network.1.key_mgmt.1.name=NONE
+wpasupplicant.profile.1.network.1.priority=100
+wpasupplicant.profile.1.network.2.status=disabled
 wpasupplicant.profile.1.network.1.ssid=ninux-diego-pomezia
+wpasupplicant.profile.1.name=AUTO
+wpasupplicant.device.1.status=disabled
+wpasupplicant.profile.1.network.2.key_mgmt.1.name=NONE
+wpasupplicant.profile.1.network.2.priority=2
+wpasupplicant.device.1.profile=AUTO

Can we avoid adding the necessary lines if not needed?

"mode": "station",
"ssid": "ap-ssid-example",
},
"encryption": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you pointed out, encryption should go in wireless :-D

"ssid": "ap-ssid-example",
"bssid": "00:11:22:33:44:55",
},
"encryption": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you pointed out, encryption should go in wireless :-D

"mode": "station",
"ssid": "ap-ssid-example",
},
"encryption": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you pointed out, encryption should go in wireless :-D

"mode": "access_point",
"ssid": "ap-ssid-example",
},
"encryption": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as you pointed out, encryption should go in wireless :-D

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try also to sort the order of keys so the generated output is always identical (as explained via IM).

@@ -60,6 +60,16 @@
},
},
},
"interface_settings": {
"properties": {
"authoneg": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add default, title and description please

"authoneg": {
"type": "boolean",
},
"flowcontrol": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add default, title and description please

"type": "string",
},
"enabled": {
"type": "boolean",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better if this boolean defaults to True

else:
try:
t = self.wireless()[0]['wireless']
if t['mode'] == 'access_point' and t['encryption']['protocol'] == 'wpa2_personal':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a double space before the first and

@edoput
Copy link
Contributor Author

edoput commented Jul 27, 2017

I'm attaching a diff between the "default configuration" (reset the antenna and enable advanced network configuration) and the default netjson

default.txt

there are minimal differencies except for the radio section , the one not implemented in netjsonconfig.

I'll go over other configurations (station+wpa2 personal, ap,...) and post the diff later

@@ -0,0 +1,152 @@
from interface import encryption, ssid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a relative import here because I'm getting an exception while testing

"""
Return the encryption dict for a wireless interface
"""
return interface['wireless']['encryption']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the encryption key is not mandatory, see: http://netjson.org/rfc.html#rfc.section.5.4.2
if encryption is omitted it means encryption off. I discovered this because I got an exception while testing.

Please, before fixing this issue could you first create a failing test?

@edoput edoput force-pushed the airos branch 5 times, most recently from 82338b0 to d6d8668 Compare August 14, 2017 13:54
@openwisp openwisp deleted a comment from coveralls Aug 31, 2017
@openwisp openwisp deleted a comment from coveralls Aug 31, 2017
@openwisp openwisp deleted a comment from coveralls Aug 31, 2017
@openwisp openwisp deleted a comment from coveralls Aug 31, 2017
@openwisp openwisp deleted a comment from coveralls Aug 31, 2017
@openwisp openwisp deleted a comment from coveralls Aug 31, 2017
@openwisp openwisp deleted a comment from coveralls Aug 31, 2017
@coveralls
Copy link

coveralls commented Aug 31, 2017

Coverage Status

Coverage decreased (-1.04%) to 98.887% when pulling 309e712 on airos into a2611ef on master.

@edoput edoput closed this Jul 4, 2018
@nemesifier
Copy link
Member

@edoput this code can be used as a good starting point for future work, or we may resume it together as well when I have cleared out some urgent matters and in case you want to dedicate some more time on it.

Can we leave it open or do you have anything against this?

@nemesifier nemesifier reopened this Jul 4, 2018
@edoput
Copy link
Contributor Author

edoput commented Jul 4, 2018

Hey, I extracted this work to a different package that can be imported at runtime with my last contribution

This PR is now closed as the work has been included in an external package, netjsonconfig-airos and will be available from the next release (v0.6.3)

Right now is still sitting there as testing on hardware is still too hard but we can manage to include it better in the openwisp controller.

Because the code has been migrated I think that the discussion should too.

I edited the top comment to explain what happened but didn't leave an explanation at the bottom.

@nemesifier
Copy link
Member

@edoput that's great! What about the docs you wrote? Maybe those could go in your README?

We should also find a way to give visibility to your package somehow, so the chances to find somebody who will help us out will increase.
We could do 2 things: add a new page in the netjsonconfig docs which links your package; show this PR in the contributors board, I just did that.

@nemesifier nemesifier changed the title Airos [netjsonconfig] Add support for Airos in OpenWISP 2 Jul 4, 2018
@nemesifier nemesifier changed the title [netjsonconfig] Add support for Airos in OpenWISP 2 Add support for Airos in OpenWISP 2 Jul 4, 2018
@nemesifier nemesifier changed the title Add support for Airos in OpenWISP 2 Add support for Airos in OpenWISP2 Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants