-
Notifications
You must be signed in to change notification settings - Fork 40
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
HTTPS via letsencrypt #103
Conversation
@bgruening @martenson @jmchilton Two questions:
|
@@ -34,13 +34,21 @@ http { | |||
|
|||
server { | |||
listen 80 default_server; | |||
server_name localhost; | |||
server_name {{ galaxy_hostname }}; ## default == localhost => fing out where to put this |
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.
that would be in defaults/main.yml
I think that's OK, we have other scripts in here as well.
Well, it's a bit of overhead, that would be OK for me, but if you're not modifying the script you could also download it when needed, right? Have you tried this role https://github.com/thefinn93/ansible-letsencrypt? It looks pretty decent. |
@mvdbeek see thefinn93/ansible-letsencrypt#38 We have unique constraints with this https/letsencrypt problem, because since I'm using @bgruening's docker container, it can't be a regular ansible-role because Bjoern calls the roles at If you go back through the commit history here a little, you see that I was originally using ansible calling |
you could launch ansible at runtime though, that's what we're doing here:
ah yes ... need to read more carefully. Yes, it all has pro's and cons, I agree. |
|
||
{% if nginx_use_passwords %} | ||
auth_basic "devbox"; | ||
auth_basic_user_file /etc/nginx/htpasswd; | ||
{% endif %} | ||
|
||
{% if galaxy_extras_config_letsencrypt %} |
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.
I think that would be generally useful, maybe call the variable galaxy_extras_config_ssl ?
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.
I'm sorry -- you want me to change the variable name, yes? Why? Because someone might use something else than letsencrypt? The only small issue with this is that right now, galaxy_extras_config_letsencrypt
is the only flag you need to set to get https via letsencrypt -- splitting it into two flags would be kind of annoying...
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.
Because someone might use something else than letsencrypt?
exactly, see https://github.com/galaxyproject/ansible-galaxy-extras/pull/47/files
galaxy_extras_config_letsencrypt is the only flag you need to set to get https via letsencrypt -- splitting it into two flags would be kind of annoying...
so what you can do is {% if galaxy_extras_config_letsencrypt or galaxy_extras_config_ssl %}
or
in the defaults.yml set galaxy_extras_config_ssl: "{{ galaxy_extras_config_letsencrypt }}".
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.
Very clever!
@@ -13,3 +13,12 @@ | |||
- { src: 'nginx_uwsgi.conf.j2', dest: '{{ nginx_conf_directory }}/uwsgi.conf' } | |||
- { src: 'nginx.conf.j2', dest: '{{ nginx_conf_path }}' } | |||
notify: Restart Nginx | |||
|
|||
|
|||
- name: Configure HTTPS via Letsencrypt |
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.
Can we change the name to 'Copy nginx acme location include' ?
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.
Do you think I can fold this into the above task? Or would making it conditional be difficult?
Thanks for the close review @mvdbeek! To everyone else: this is currently in testing -- hopefully soon will work and we can merge! |
What's your plan for providing the user details? I guess email and domain name have to go somewhere ... maybe as variables in letsencrypt.conf.j2 ? |
@mvdbeek what do you mean by providing the user details? I don't think we need email, but we do need domain name -- which needs to be passed in as an |
I've never used letsencrypt, I was assuming you would need some kind of account. |
You mean setting a variable in ansible to "" is enough to suggest that it I couldn't figure out how to do this earlier so I asked: On Thu, Aug 18, 2016 at 5:39 PM, Marius van den Beek <
Alex Lenail |
No, but usually we document all variables in defaults/main.yml, so that people are aware of all variables. So if there is no good default, Anyway, role default variables have the lowest precedence of all variables, so whatever you pass in as And that's also nicer for other projects that use this role, where you do know in advance the hostname. |
supervisorctl restart nginx: | ||
fi | ||
|
||
/bin/bash letsencrypt.sh --cron -d ${GALAXY_CONFIG_DOMAIN} --config letsencrypt.conf |
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.
Does this need cron? I don't think this is available in the docker container
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.
@mvdbeek that command does not require cron, it's meant to be executed from cron. See here:
https://github.com/galaxyproject/ansible-galaxy-extras/pull/103/files/f9d01e2ae4cd57606a0e7b2f6c87a6d4a6095c9d#diff-205c98235541487412061d12953dc4c0R94
@mvdbeek @bgruening This now works locally and remotely. However, it still uses ansible, which is a problem for people like me using Bjoern's image. So I'm going to try to refactor it to move much of the nginx.yml modifications into startup.sh. I need to ponder this briefly, but I'm hoping to merge this tonight or tomorrow before Bjoern goes on vacation! |
@mvdbeek @bgruening That is now fixed. This is ready for merge. |
when: galaxy_extras_config_letsencrypt | ||
|
||
- name: get the letsencrypt script from github repository | ||
get_url: |
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.
Nice!
This looks very good, I will do some testing and change the supervisor template and then merge this over the weekend. It looks really nice! Thanks a lot @zfrenchee |
@zfrenchee thanks for integrating all my comments and annoying requests - this is really looking great now. I fear that I can only get this in the Docker image when I'm back, in 1-2 weeks. As I already told you this should not put you back, just use your local own build Galaxy image in the meantime. Moreover, for the Docker integration it would be nice if you can reactivate the self-signed certificate, we need this in the Docker-travis file to test the setup. I think the following steps needs to be done:
Thanks again @zfrenchee! |
I don't think you will have incoming connections on travis, so I guess that won't work. |
@bgruening As @mvdbeek mentions, I still don't think we can test this in travis. I think I could add back a self-signed certificate and we could test that with travis, but I think that would make this PR more complicated and it wouldn't actually be testing what we want to test. So it sounds like the only modification I need to make is wrap the cron task in supervisor template in an if block. |
@mvdbeek I cleaned it up a little, including wrapping cron in an if block (please check to make sure I did it right). Looking forward to the merge this weekend if all goes smoothly. It seems there are some merge conflicts. Would you like to take care of those or should I try? |
variable called `GALAXY_CONFIG_DOMAIN` which is set to the domain name you would like to certify. | ||
|
||
If you're using [Docker-Galaxy-Stable](https://github.com/bgruening/docker-galaxy-stable/) or some | ||
derivative image, you can specify `GALAXY_CONFIG_DOMAIN` at runtime by running the image |
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.
Can you please rename it. GALAXY_CONFIG
is a preserved namespace for galaxy.ini changes. Maybe just GALAXY_DOMAIN
? You want to have the FQDN (Fully Qualified Domain Name), this could probably also go into the readme.
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.
Oh, sorry! I did not realize GALAXY_CONFIG_*
was a reserved namespace! I was hoping to change the name anyways! (It doesn't need to be a fqdn, e.g. for me "oom.io" was all I needed)
@@ -31,10 +31,17 @@ http { | |||
server localhost:8080; | |||
} | |||
{% endif %} | |||
server { | |||
listen 80; | |||
{% if galaxy_extras_config_ssl %} |
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.
indentation
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.
We need to cast this to bool isn't it? galaxy_extras_config_ssl|bool
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.
IIRC this is only needed when combined with logic operators in tasks.
Otherwise the tests wouldn't pass.
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.
indentation
I don't see it, what exactly should be indented?
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.
I'm not sure when it is really needed, but it is also mentioned here in when
without a logic operator. http://docs.ansible.com/ansible/playbooks_filters.html
- debug: msg=test
when: some_string_value | bool
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.
I wouldn't bet my life on it, but I think this is only needed in certain ansible tasks, not for jinja2 templates.
After all, the tests that test port 80 would be redirected to port 443, which would fail. Also we have other if statements in this file that are not using the boolean filter.
I think we should use this only when necessary.
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.
I would err on the side of caution and just add the cast if you're not 100% sure, or, alternatively (more work), test that the false case works as intended without the cast.
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.
I would err on the side of caution and just add the cast if you're not 100% sure, or, alternatively (more work), test that the false case works as intended without the cast.
I am 100% sure, because the tests take the default, which is False, and test that access to galaxy on port 80 works, without exposing port 443. There is no way that in this instance here it is necessary. I am just not sure when exactly it is needed.
Looks great, thanks especially for integrating the self-signed test again! |
@mvdbeek @bgruening what's the latest on this? |
ready to go from my side |
rebased to resolve merge conflict. |
keys in startup.sh. Remove cron handler (needs to not have autostart enabled, start in startup.sh script). Tag relevant part with https so only subset of tasks runs.
$GALAXY_CONFIG_GALAXY_INFRASTRUCTURE_URL.
OK, last test for the boolean issue is here. I changed false to no, just to be sure that this works as well. |
Great work @zfrenchee and @mvdbeek! Thanks a lot! |
Just tracking changes I’ve made through the docker-galaxy-stable repo
to add HTTPS. Opening a PR purely for visualization purposes right now.