iFargle / headscale-webui

A simple Headscale web UI for small-scale deployments.
Other
627 stars 57 forks source link

Improve code style #86

Closed MarekPikula closed 1 year ago

MarekPikula commented 1 year ago

Unfortunately, it wasn't possible to split it into multiple smaller commits since the changes touched the entire application substantially. Here is a short list of significant changes:

  1. Create a separate library (headscale-api), which is used as a convenient abstraction layer providing a Pythonic interface with Pydantic. Headscale API is a fully asynchronous library, benefitting from improved concurrency for backend requests, thus increasing page load speed, e.g., on the "Machines" page.
  2. Create a common, validated (with flask-pydantic) API passthrough layer from GUI to the backend.
  3. Move authentication to a separate module (auth.py), consolidating the functionality in a single place (with a better place for expansion in the future).
  4. Move configuration management to a separate module (config.py). Use Pydantic's BaseSettings for reading values from the environment, with extensive validation and error reporting.
  5. Reduce the number/frequency of health checks increasing the overall performance/decreasing load latency:
    • Now, most checks (e.g., filesystem checks) are performed during server initialization. If any test fails, the server is started in tainted mode, with only the error page exposed (thus reducing the surface of the attack in an invalid state).
    • Key checks are implicit in the requests to the backend and guarded by @headscale.key_check_guard decorator.
    • Key renewal is moved to the server-side scheduler.
  6. Introduce type hints to the level satisfactory for mypy static analysis. Also, enable some other linters in CI and add optional pre-commit hooks.
  7. Properly handle some error states. Instead of returning success and handling different responses, if something fails, there is an HTTP error code and standard response for it.
  8. General formatting, minor rewrites for clarity and more idiomatic Python constructs.
  9. Add basic devcontainer setup (if somebody wants to develop this in VS Code).

From my perspective, some more work still needs to be done, but at this point, most of the significant changes are complete, and I would like to (finally) push this forward to have it merged sooner than later so that other people can base on this (or the evolution of it). I think that some issues might be solved with this rewrite (#84 #74), but testing from the user's side is required for this. I did my best to go through all the parts of the application manually, but for sure, there is an option I missed something (or didn't retest after some polishing). The most important thing is for you @iFargle, to review the code and check if everything works as before (or better). From my side, I will check if the documentation needs any updates and retest everything again (tomorrow).

One issue I already found is some intermittent error with aiohttp used for async requests when running by gunicorn in a multi-worker set-up. I would say that for now, it's a low priority as, either way, the project from the start didn't support it. In the long run, it would be really nice to have support for multiple workers, especially if this frontend would be used by bigger users (e.g., companies) with more machines/users/admins to serve.

Work on #73

iFargle commented 1 year ago

I'll work on this as soon as I can! 2-3 days at most. Thank you for your help on this -- This is great!

iFargle commented 1 year ago

My (very) basic tests seem to all work fine! #80 still exists but that's expected.