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

Large Architecture overhaul. #26

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

Conversation

alex-davis-bird
Copy link

  • Move to nginx+gunicorn for web stack.
  • Implement redis for sharing pipes across multiple threads / instances
  • server/initialize.py now creates a penguindomeSVC account on the server machine
  • Addeds file permission checks / sets during server/initialize.py to fix any permission errors.
  • integrated the nginx / redis setup into server/initialize.py. will auto create the needed config files, remove default sites, create systemd unit files

I created this fork a while back and din't think the project was active... I saw all the new changes and it looks like we both fixed the pgp keychain issue. I also fixed the issue, which i included in the PR since it was less work to merge back up to master than the other way around. Let me know if theres any issues there. The way i write it you explicitly set which keychain to use in server.py / client.py

One other change that had to be made was that by having multiple server workers (multiple threads or multiple instances), there was an issue with encryptors being used during the client shell. So some work had to be done to move where the encryptor was created in order to store it in redis. Since the encryptor is only used to validate the data and transit security is being provided by tls/ssl to the server

…ces readability for arbitrary reason. I fixed all the length issues to

bring the code in line to the repo, however due to a long line in ./server/initialize.py:570 i still had to add E501 to the tox ignore list.
@jikamens
Copy link
Collaborator

Thanks for the contribution! This is great stuff, so thanks for sharing it.

I unfortunately do not have time to review the entire PR in detail right now, but I do see some initial issues that I would like to see addressed before it is merged:

  1. Please update your PR to exclude your pgp keychain issue fix, since there's already a working fix on master. There are no full-time maintainers for this project, so putting extra work into a PR makes it less likely that we will have the time to review and merge it. As such, it's not really a good idea to include a fix in a PR when there is already a different, working fix for the same issue on master.

  2. We can't require redis or nginx, so code that adds support for those needs to make it optional.

  3. I like the idea of the server running in a service account, but it needs to be able to listen on privileged ports even if the installer opts not to use nginx. Therefore if the server isn't running under nginx then it needs to start as root and bind to the necessary ports before changing its UID to the service account.

  4. Please take E501 out of tox.ini and fix your code to conform if it doesn't. We are not removing the line length requirement from the project.

Thanks!

@alex-davis-bird
Copy link
Author

Yeah, no need to rush through the review - i know a lot changed :)

  1. will update this in the upcoming days / weekend, ill probably be working on this in my personal time so this might stay open for a bit.

  2. same as above. Ill set a flag on install and then reference that when setting pipes and encryptors. this adds some complexity in running the service as someone not familiar with the architecture... if you don't use redis, then running with multiple web workers will cause errors when trying to use the client shell. I can get around that by checking on if redis is being used, and only use 1 worker for nginx if thats the case.

You can also run this without nginx by invoking the server directly. I actually do this for debugging during dev, and make sure that invoking the server without nginx still starts up correctly. When making the changes to the guided install, if someone answers no to nginx/redis, would you expect a systemd unit file to be created to run the debug server on boot, or let the user handle their own startup file? I wouldn't recommend running this in production on the debug server.

  1. You should be able to start the server as root. I think the answer here depends on the second paragraph above... do we want a systemd file created for the no nginx use case? if so we can just use that to start as root, as is the current case.

I do see a potential problem where running as root could lead to files being created in var with root:root ownership, which could then break the install when running as non root. re-running the ./server-setup.sh and pressing y on the fix permissions option will fix that.

besides that, i dont think theres a graceful way to start as root in order to bind to 80, then drop down to the service account that doesnt involve significant effort to implement

  1. All of the lines in the file are <=79 characters except for one. the only line that is longer is a systemd file which can't be broken up as a multiline string. I can ditch the multiline string but then it won't cleanly represent the file being written. The line in question is server/initialize.py:570 - let me know if you still want me to change it to be multiple concatenated strings so i can wrap the line.

@jikamens
Copy link
Collaborator

  1. same as above. Ill set a flag on install and then reference that when setting pipes and encryptors. this adds some complexity in running the service as someone not familiar with the architecture... if you don't use redis, then running with multiple web workers will cause errors when trying to use the client shell. I can get around that by checking on if redis is being used, and only use 1 worker for nginx if thats the case.

Yes, I agree that it's reasonable to only allow for one worker if redis isn't being used.

You can also run this without nginx by invoking the server directly. I actually do this for debugging during dev, and make sure that invoking the server without nginx still starts up correctly. When making the changes to the guided install, if someone answers no to nginx/redis, would you expect a systemd unit file to be created to run the debug server on boot, or let the user handle their own startup file? I wouldn't recommend running this in production on the debug server.

Yes, if not using nginx I would expect the current behavior of creating a systemd file to be offered.

We had no trouble at Quantopian running it in production using the debug server. It really depends on how many clients you're supporting. Nginx, redis, and multiple workers are overkill for a small installation, which is why they should be optional.

besides that, i dont think theres a graceful way to start as root in order to bind to 80, then drop down to the service account that doesnt involve significant effort to implement

It is not clear to me what problem you're anticipating here that makes this a difficult problem.

  1. All of the lines in the file are <=79 characters except for one. the only line that is longer is a systemd file which can't be broken up as a multiline string. I can ditch the multiline string but then it won't cleanly represent the file being written. The line in question is server/initialize.py:570 - let me know if you still want me to change it to be multiple concatenated strings so i can wrap the line.

E501 isn't enforced on multi-line strings, so if that's the only problem then it's not clear to me why you added E501 to the exclude list in the first place.

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