Code review and hardening #73
Comments
If you have any resources for Python development you'd like to share I'd be eager to give them a read! 99% of my Python experience is just back-end / server automation type scripts. Mostly super simple stuff. Thank you! Let me know what you need from me! |
|
All right, so I'll spend the next two days or so making the code more production-ready. I'll let you know about the progress 🙂 |
|
Thank you! |
|
Here is a quick update from my side. I'm in the middle of a relatively big refactor, so it will take a few more days to complete and properly test it. I already published part of my work in a separate package (https://pypi.org/project/headscale-api/). It's a Headscale API abstraction, which could be useful also for other projects connecting to Headscale – I already use it in the refactor and am pretty happy about the current state of it. In the meantime, please hold any development so that there are no conflicts for me to solve once the PR is ready later this week or in the beginning of the next week. |
|
This is already wayyy more advanced than my capabilities. I will be watching closely and learning! |
|
A quick update from my side: I'm mostly done. I tried to test everything and after some bughuting everything seems to work. Now, I'll be cleaning up the code, adding some CI and preparing the PR with full description of changes. I'll let you know once I push some code. Unfortunately, due to the fact that the refactor is rather big and touches many parts of the app, it will be one big commit with many changes, so it will be a little harder to see all the specific aspects of the refactor, but I hope the description will fill the gap. I hope to finish it by the end of the week. |
|
Sounds good! I've been slightly busier than usual so I may not have a lot of time to work on this. I'll try and check daily. |
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>
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>
|
Hm, that seems to have broken my Jenkins build. |
|
Both BUILD_DATE and AUTH_TYPE should be set unless they're being overwritten somewhere? |
|
Chiming in here, but I was setting up this app for the first time and it looks like latest is broken like you said. I was setting AUTH_TYPE but the app was failing to load same as the error above. Using v0.6.1 works fine Edit: Definitely an issue with the way you're doing the new config.py. I can get AUTH_TYPE to set by using lower "basic", I wasn't able to get BUILD_DATE to take correctly, but the default definitely isn't being used |
|
I'm troubleshooting, give me a bit :) |
|
To be honest, I didn't expect you to already release the code 😛 But I guess this way, we will get more bug reports quicker. I think that I know what the problem might be. I'll troubleshoot in a moment. |
|
ha, I'm super new to all this -- The only way I can get Jenkins to build is either by branching from my repo (my mirror doesn't pick up on pull requests) or by committing to main. So YOLO, I committed to main! but I realize I should be more careful -- There ARE at least a few active users of this project. |
|
FYI Jenkins is running. Since it's multiarch it takes around an hour to build. |
|
I lied. Jenkins doesn't seem to like that new string -- No logs output at all |
|
I'll try to make a GH-based Docker build sometime next week. |
|
Hmm, ok, so I'll revert it to general string. Not that it's needed to be in some concrete format anyways. |
|
Done in #90. |
|
Now I'm getting other errors: I'm going to see what troubleshooting I can do myself. Some of this code is above my head but I'll do my best :) |
|
So when I initially tested I just cloned your pull request and did a local docker build. I'm really unsure of what the difference would be |
|
Figured that part out, now we're onto an issue with the scheduler: Not 100% sure about this one. if you'd like to take a look it's in the 7.0.1-testing branch I'm off to bed for now |
|
I can't really do much more -- My system runs out of RAM when trying to install packages via poetry. All 64GB's.. Hm. |
|
Hi, sorry for a long break. I had to catch up at work. I noticed that you reverted the PR. Could you summarize what was problematic in the last revision so that we can go back to work on merging it to main? |
|
The issue you mentioned has a rather misleading message. The thing that is the failing point is Side note: Please, don't push commits with such messages to a place where collaboration happens. I suggest reading https://cbea.ms/git-commit/ to make other developers happy. Also, |
|
Based on the git history, there are the new features currently available on
|
|
Ok, so all changes guaranteeing feature parity should be pushed to 0.7.0-dev. Merging it will be tricky, since you reverted the previous PRs in 2033f80. I'll figure out a way to make it mergable. |
|
All right. It will be messy, but possible. Opened #107 to have it pass CI. Further discussion there. |
|
My apologies -- I know you didn't sign up for teaching anyone :) I'll read through the docs and everything later tonight. Thank you! |
|
Are there news on the new version? |
Hi, first of all, thank you for the great work. I consider using headscale in my company, and having a GUI is crucial for efficient operation. Since VPN is crucial in terms of security for the company, I want to thoroughly review the code of the GUI as it has direct access to all administration tasks (via API key) and is a significant attack surface both from outside and from inside. I have some questions regarding the current code style because it seems to me that some important things are implemented in a poor style, and many low-hanging fruits are left out. I have a few years of experience in Python programming, and honestly, some constructs look to me a little unsettling and make me not want to use this package for production.
The text was updated successfully, but these errors were encountered: