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

[feature] Implemented device deactivation and reactivation #625 #840

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

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Feb 29, 2024

Closes #625

Blockers

TODOS

  • we shall make all the field of a deactivated device readonly
  • if the device has any related OpenVPN template with certificates, the certificates shall be flagged as revoked
  • removes any config and template from the device, ensuring the revoked certificates do not get deleted (because we need those to be in the CRL)
  • sets the config status to deactivating (this is a new status)
  • once the config is applied by the agent (we must make sure it's deactivated) the config status should be set to "deactivated" instead of "modified"
  • once the config is set to deactivated, the controller HTTP API should return 404 for these devices, so that the agent stops asking for the checksum
  • I think we should hide the delete button of the admin and show "deactivate" instead, we could show the delete button only if the config status is "deactivated" or if the device does not have a config object
  • we should add the deactivate admin list action, if we can add it before the delete action it's better
  • the admin action which deactivates multiple items at once must deal with the case in which one or multiple devices are already deactivated in a graceful way, informing the user and without failing
  • ensure we emit a signal when this happens, I believe a config status change is emitted but let's double check (so we can listen for this event in other modules)
  • we will also need to update the API to prevent changing any details on the deactivated devices
  • add an admin action which sets the config status to applied;
    • it must deal with the case in which one or multiple devices are already activated in graceful way, informing the user and without failing

@pandafy pandafy force-pushed the issues/625-device-deactivation branch 2 times, most recently from ae5a5ca to 2c16d95 Compare March 1, 2024 17:48
@pandafy pandafy force-pushed the issues/625-device-deactivation branch from 2c16d95 to 5f9523f Compare March 1, 2024 18:02
@@ -368,7 +368,6 @@ def update_config(self):
except Exception as e:
logger.exception(e)
else:
self.device.config.set_status_applied()
Copy link
Member Author

Choose a reason for hiding this comment

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

@nemesifier why do we need this? It is creating an issue for marking the device as deactivated. Instead of changing the status from deactivating to deactivated, this code is changing the status to applied. Can we rely on the report status request from the agent for this?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, proceed, there may be some test to change because of this removal.

Copy link
Member

Choose a reason for hiding this comment

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

@pandafy pandafy force-pushed the issues/625-device-deactivation branch from 4382340 to b427efd Compare March 7, 2024 17:31
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.

Looking good!

I did a quick code review, find my comments below.

Please let's take advantage to introduce the improvement discussed yesterday:

diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py
index 764ae7f..cd573fb 100644
--- a/openwisp_controller/config/admin.py
+++ b/openwisp_controller/config/admin.py
@@ -641,6 +641,10 @@ class DeviceAdmin(MultitenantAdminMixin, BaseConfigAdmin, UUIDAdmin):
     ip.short_description = _('IP address')
 
     def config_status(self, obj):
+        if obj.is_deactivated:
+            return _('deactivated')
+        if not obj.config:
+            return _('unknown')
         return obj.config.status
 
     config_status.short_description = _('config status')

Another note:

Screenshot from 2024-03-07 16-07-39

Can we hide the save and save continue button on deactivated devices?

What else is missing?

  • Do not allow to delete devices which are not deactivated first via admin action
  • Admin inlines
  • It would be great to show the readonly config value as indented JSON (now it shows OrderedDict(...)).

API changes:

  • Disable API endpoints to make changes to deactivated devices
  • I think we need to allow to enable and disable a device via API too, right?
    We could split the API changes in a separate issue but we need to discuss this first

Anything else?

openwisp_controller/config/admin.py Outdated Show resolved Hide resolved
openwisp_controller/config/base/config.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Implement device deactivation and reactivation
2 participants