-
-
Notifications
You must be signed in to change notification settings - Fork 182
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] Added support for ZeroTier VPN backend #778
Conversation
2c56913
to
b2db3c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @Aryamanz29! 👏🏼
This is still a draft, so it was expected to find shortcomings. I am documenting the issue I encountered below, so @Aryamanz29 can also address them before the PR is ready for review.
- It is expected from user to put port along with hostname in the "Host" field of the VPN object (i.e.
127.0.0.1:9993
). This is puzzling to me. I did a quick search but didn't find any setting to change the listen port for ZeroTier service API. Can you please verify this @Aryamanz29 ? If it is not possible to change the listen port on ZeroTier controller, then we can hard code this value. - If the configuration encounters any error, the page is reloaded, but the JS does not hide unnecessary fields.
- Open for discussion: When creating a new ZeroTier VPN, there are certain fields which are read only. Thus, I don't see a point in rendering them.
These fields are automatically generated. I think, we should treat them as we treat key-pair values for OpenVPN and WireGuard. Set them as configuration variable at VPN level. - Use checkbox input for all boolean fields in JSONSchema (This needs to be done in NetJSONConfig)
- I was not able to confirm this but something in ZeroTier VPN configuration is triggering "unsaved-changes.js" on the change page of existing VPN object.. When moving to a different webpage, the "unsaved changes" alert box shows up even when nothing is changed. @Aryamanz29 can you please confirm this?
openwisp_controller/config/migrations/0048_alter_vpn_auth_token.py
Outdated
Show resolved
Hide resolved
Meet Notes - 20th June
|
874c6ee
to
1308e0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the notes taken during our last weekly meeting:
- Use zerotier central controller as a model for naming UI fields, labels and help texts
- When a zerotier VPN server is created we need to automate the addition of the server itself as a member to the zerotier network (let's use the internal IP address field)
- Let's introduce a retry mechanism in case the API requests fail
- Let's apply the same retry mechanism to all API calls, look at OpenWISP monitoring for inspiration
- The retry mechanism should retry only the exceptions which are considered recoverable, that means not retrying when we get a 404, 403, 401 and any other status code which indicates that retrying will surely not fix the issue (but an admin has to intervene instead)
- We need to introduce a new notification type to communicate the unrecoverable failures to the user, ideally we want to reuse this notification type for multiple background tasks, not just zerotier related
#778 (comment) - Handled the scenario where an invalid hostname is provided in the ZeroTier /status call. - Improved user experience by overriding the clean() method, which now reports that the auth_token field is required when the user selects the ZeroTier VPN backend. - Moved the logic from the _update_zerotier_server() method to the update_vpn_server_configuration method for better code organization. - Removed redundant conditional checks and unnecessary log statement from the _update_zerotier_server method.
#778 (comment) - Handled the scenario where an invalid hostname is provided in the ZeroTier /status call. - Improved user experience by overriding the clean() method, which now reports that the auth_token field is required when the user selects the ZeroTier VPN backend. - Moved the logic from the _update_zerotier_server() method to the update_vpn_server_configuration method for better code organization. - Removed redundant conditional checks and unnecessary log statement from the _update_zerotier_server method.
9e8f310
to
abc6a11
Compare
#778 (comment) - Handled the scenario where an invalid hostname is provided in the ZeroTier /status call. - Improved user experience by overriding the clean() method, which now reports that the auth_token field is required when the user selects the ZeroTier VPN backend. - Moved the logic from the _update_zerotier_server() method to the update_vpn_server_configuration method for better code organization. - Removed redundant conditional checks and unnecessary log statement from the _update_zerotier_server method.
4fa87d4
to
87b552d
Compare
- Added a new notification type for background API tasks. - This notification type will be used in the zerotier backend to notify users about various events related to background API calls. - Currently, the new notification is used in the following scenarios: - Creating a new zerotier network. - Updating an existing zerotier network. - Modifying network member configurations such as authorization and IP assignments.
**ZerotierService class** - Added a timeout of '5' seconds to API request methods. **OpenwispApiTask class** - Changed the log level to `logger.warn` when the retry limit is not reached, and `logger.error` only when the retry limit is reached. - Modified the logic to send a recovery notification only when the API task encountered an error before. Otherwise, no recovery notification is generated. - Updated the cache key as suggested in the review. - Added a docstring and comments for the `handle_api_call` method. **vpn.py** - Disabled email notifications for background API tasks. - Utilized guard clause in the `_validate_host` method. - Moved Zerotier VPN `auth_token` validation back to `models.py`. - Simplified the subnet and IP values for Zerotier API tasks. - Updated translation string messages and used the `.format` method instead of f-strings. For more information, see: https://docs.djangoproject.com/en/4.2/topics/i18n/translation/#standard-translation
- Added TestZeroTier and TestZeroTierTransaction - Updated `_invalidate_peer_cache` method of the Vpn model to emit `vpn_peers_changed` signal for only for Wireguard or VXLAN VPN backends
…calls - Ensure that recovery notifications are triggered only when the VPN server has previously encountered an unrecoverable error (error notification)
- Added a test case for zerotier vpn deletion 404
…pn server - (req-changes) Moved time import at the top of the file. - Subnet should be type cast after ip assignment evaluation. - Updated zt vpn deletion tests.
- Fixed test vpn deletion notification mock. - Deleted ZT API tasks notification cache keys after vpn deletion.
- Added exception handling to get_node_status() method. - Fixed timeout issues, added global constant for timeout. - Refactored zerotier network ip_start and ip_end assignments. - Added comment for update_vpn_server_configuration() method. - Updated signature of update_network_member() method for reusability with zt members. - Changed locmem.keys mock to settings.backned.keys - Moved subnet delete reset outside of subtest.
69c3fd5
to
07b48f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
07b48f7
to
ffd4eb1
Compare
ffd4eb1
to
3b9e0f7
Compare
Depends on:
Additionally, basic support has been added for managing ZeroTier networks through OpenWISP using the ZeroTier Service API
Closes #604