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

Improve code style #86

Merged
merged 14 commits into from
Apr 21, 2023
Merged

Improve code style #86

merged 14 commits into from
Apr 21, 2023

Conversation

MarekPikula
Copy link
Collaborator

@MarekPikula MarekPikula commented Apr 21, 2023

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

Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
- black
- isort
- ruff
- pre-comit
- mypy
- pydocstyle

Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
Major part of iFargle#73

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

1. Create a separate library (headscale-api), which is used as a
   convenient abstraction layer providing Pythonic interface with
   Pydantic. Headscale API is fully asynchronous library, benefitting
   from improved concurrency for backend requests thus increasing page
   load speed, e.g., on "Machines" page.
2. Create a common common, validated with flask-pydantic API passthrough
   layer from GUI to the backend.
3. Move authentication to a separate (auth.py), consolidating the
   functionality in a single place (with 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 environment, with
   extensive validation and error reporting.
5. Reduce the number of health checks.
    - Now, most 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 attack in 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 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 HTTP error
   code and standard response for it.
8. General formatting, small rewrites for clarity and more idiomatic
   Python constructs.

Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
@MarekPikula MarekPikula self-assigned this Apr 21, 2023
Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
@iFargle
Copy link
Owner

iFargle commented Apr 21, 2023

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!

Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
To be revisited in future.

Signed-off-by: Marek Pikuła <marek.pikula@embevity.com>
@iFargle iFargle merged commit 5097807 into iFargle:main Apr 21, 2023
@iFargle
Copy link
Owner

iFargle commented Apr 21, 2023

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

@MarekPikula MarekPikula deleted the improve_code_style branch April 21, 2023 16:48
MarekPikula added a commit that referenced this pull request Jun 4, 2023
This reverts commit 2033f80.

All the changes were introduced in #86. Reference it for full history.
@MarekPikula MarekPikula mentioned this pull request Jun 4, 2023
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.

2 participants