[feature] Add support for SNMP credentials #471

[purhan]

Closes #471, closes #461

[coveralls]

Coverage increased (+0.007%) to 98.545% when pulling 4dfd03b on issues/471-snmp-connections into 6d07b05 on master.

[nemesifier]

we should not add a new DB column for this.
I think in OpenWISP Monitoring we should remove the field from being readonly, make it readonly with JS unless there's an SNMP check added, in that case we allow to change it.

[purhan]

@nemesisdesign but we need some way to track that the user wants to change it, otherwise it will automatically update from the API requests (https://github.com/openwisp/openwisp-controller/pull/496/files#diff-47782e8d264d4907d7893cff2e77b42523d4fb90af8aaef26e78c7a34e996713R75). What do you think?

[nemesifier]

should not be an issue.. openwrt devices will send the right management IP so we can keep it, non openwrt devices will not send management IP and will be empty.

Please verify the following:

• modify your openwrt device to not have "management_interface" in /etc/config/openwisp
• restart openwisp config
• try setting a custom management IP
• restart openwisp config again
• check whether the management IP is modified or not

[purhan]

@nemesisdesign in that case, the management IP resets to an empty string.

[pandafy]

I confirm that the management IP changes after config restarts. I believe this is right. We should add a hint in the admin UI.

"IP address of the management interface configured on the device. The information received from the device will overwrite any changes done here"

[nemesifier]

Ok thank you @purhan and @pandafy for double checking.

Here's my reasoning.

1. OpenWrt devices running openwisp-config and snmpd

In this case, we can assume the management IP will be set if management_ip is specified correctly in /etc/config/openwisp.

Hence we do not to add code that excludes the management IP update for this case.

2. AirOS devices running only snmp deamon, or OpenWrt running only SNMPd

Management IP will not be set at all, nor reset automatically because openwisp-config is not running.

Hence we do not to add code that excludes the management IP update for this case as well.

This code is not useful

For the reasons explained above I believe this code is not useful.

What should we look for? What would be useful?

I think we should strive for a solution which is general enough so that it can be flexibly used in different cases.

I believe a solution which can work reliably in many cases is the following:

• allow the field to be writable
• use JS to make it readonly by default, but provide two buttons: one to clear the management IP and one to edit it
  • clear action just resets it to empty string
  • edit action will allow the user to changethe value
• make the help text more detailed, something like @pandafy indicated, eg:

This is the IP address used by OpenWISP to reach the device when performing any type of push operation or active check. The value of this field is generally sent by the device and hence does not need to be changed, but can be changed or cleared manually if needed.

[purhan]

@nemesisdesign @pandafy Do you think perhaps a clear ip button is redundant? If the user wants to reset it, they can just edit it to be empty? This is how I have made it for now but I can change if you say so.

[nemesifier]

Will check and let you know.

[pandafy]

In a review earlier Federico suggested to update the help text for management IP. That is missing from this PR.

This is the IP address used by OpenWISP to reach the device when performing any type of push operation or active check. The value of this field is generally sent by the device and hence does not need to be changed, but can be changed or cleared manually if needed

Instead of adding a big button like below,

I would prefer a small icon that goes well with rest of Django's admin

@purhan if you are not already working with the new these, please pull those changes from the master branch.

[pandafy]

Thinking out loud here

Instead of toggilng between a div and an input field. We can render the management ip field as a disabled field, just like UUID.
When a user clicks on edit icon, it will enable the field.

When the field is editable, two options should be shown to the user, one to cancel the operation and other to save the value.

Cancel function is self explanatory: revert changes and make the field disabled again.

Save function will create a PATCH request which will only update the management IP of the device on the server. If the operation is successful, the updated value is reflected in the field and the field get disabled again,

Use of disabled input field is just for making development and maintenance easier. We can also stick with the currently implement with div if that looks nicer to your eyes.

The other part regarding PATCH request occurred to me, since it was not very intuitive that I will need to click the "Save" button on the top/bottom of the page to save the value I entered in the new field that just appeared.

Second thoughts, instead of making a PATCH request the save function could trigger "Save and continue editing" operation which already exists in Django admin.

What do you think @purhan and @nemesisdesign ?

[nemesifier]

In a review earlier Federico suggested to update the help text for management IP. That is missing from this PR.

This is the IP address used by OpenWISP to reach the device when performing any type of push operation or active check. The value of this field is generally sent by the device and hence does not need to be changed, but can be changed or cleared manually if needed

Instead of adding a big button like below,

I would prefer a small icon that goes well with rest of Django's admin

@purhan if you are not already working with the new these, please pull those changes from the master branch.

New theme you meant @pandafy?
@purhan I agree with Gagan, an edit icon is going to be consistent with the rest of the admin.

Thinking out loud here

Instead of toggilng between a div and an input field. We can render the management ip field as a disabled field, just like UUID.
When a user clicks on edit icon, it will enable the field.

When the field is editable, two options should be shown to the user, one to cancel the operation and other to save the value.

Cancel function is self explanatory: revert changes and make the field disabled again.

Save function will create a PATCH request which will only update the management IP of the device on the server. If the operation is successful, the updated value is reflected in the field and the field get disabled again,

Use of disabled input field is just for making development and maintenance easier. We can also stick with the currently implement with div if that looks nicer to your eyes.

The other part regarding PATCH request occurred to me, since it was not very intuitive that I will need to click the "Save" button on the top/bottom of the page to save the value I entered in the new field that just appeared.

Second thoughts, instead of making a PATCH request the save function could trigger "Save and continue editing" operation which already exists in Django admin.

What do you think @purhan and @nemesisdesign ?

@pandafy @purhan I think we should keep this simple for now and what Purhan is doing is good enough to start using it.
I suggest focusing on refining the backend and networking features because once we get those right we can kind of forget about them for a while, the UI is something which makes sense to improve only once the feature gains more usage and we're really sure the time invested in it is worth it.

[pandafy]

Good work Purhan!

Few minor changes are requested regarding UI. With them, this should be complete.

[nemesifier]

Ok we are on the right track! 👍

I see one problem though: it is possible to set a management IP which is already taken, which will be an issue. I am have created a dedicated issue (#523) because it's a non-blocking issue, I mean that we can continue testing and iterating this feature also without validation, but validation will be needed for real usage.

See my comments below. Once those are solved I think we can merge so I can deploy this to a staging system and we can start testing the feature.

[nemesifier]

Thanks for updating it @Aryamanz29, I will test it after the release of OpenWISP 22.

[nemesifier]

@Aryamanz29 good progress, see my comments below, I think after those changes this PR is ready.

We should then proceed with openwisp/openwisp-monitoring#309.

[nemesifier]

I changed the help_text of management_ip in the past migrations and removed 0041_alter_management_ip_help_text.py because I think we can live happily without it.

I found a couple of details which I think can be improved which I didn't see before, should be quick to fix.

[nemesifier]

Almost.. see below.