#972 - [Nginx] - Shouldn't ssl_src_path be remplaced by nginx_ssl_dir in ssl.yml task - Disroot/Disroot-Project - Disroot Forgejo: Brace yourself, merge conflicts ahead.

meaz commented

2024-07-29 08:25:55 +02:00

Owner

On nginx role, shouldn't ssl_src_path be replaced by nginx_ssl_dir in ssl.yml task? If so, then we should remove all ssl_src_path from all role, hosts_vars, documentation, etc.

Also, I don't understand the use of

- name: "[NGINX] - Deploy SSL keys for {{ item.name }}"
 copy:
   src: "{{ ssl_src_path }}/{{ item.ssl_name }}/privkey.pem"
   dest: "{{ nginx_ssl_dir}}/{{ item.ssl_name }}/privkey.pem"
   mode: 0700
 with_items: "{{ nginx_vhosts }}"
 when: item.copy_ssl is defined
 notify: reload nginx

- name: "[NGINX] - Deploy SSL certs for {{ item.name }}"
 copy:
   src: "{{ ssl_src_path }}/{{ item.ssl_name }}/fullchain.pem"
   dest: "{{ nginx_ssl_dir}}/{{ item.ssl_name }}/fullchain.pem"
   mode: 0644
 with_items: "{{ nginx_vhosts }}"
 when: item.copy_ssl is defined
 notify: reload nginx

I especially don't understand coz for most (all?) CT, I see that ssl_src_path and nginx_ssl_dir is the same path...

On nginx role, shouldn't `ssl_src_path` be replaced by `nginx_ssl_dir` in `ssl.yml` task? If so, then we should remove all `ssl_src_path` from all role, hosts_vars, documentation, etc. Also, I don't understand the use of ``` - name: "[NGINX] - Deploy SSL keys for {{ item.name }}" copy: src: "{{ ssl_src_path }}/{{ item.ssl_name }}/privkey.pem" dest: "{{ nginx_ssl_dir}}/{{ item.ssl_name }}/privkey.pem" mode: 0700 with_items: "{{ nginx_vhosts }}" when: item.copy_ssl is defined notify: reload nginx - name: "[NGINX] - Deploy SSL certs for {{ item.name }}" copy: src: "{{ ssl_src_path }}/{{ item.ssl_name }}/fullchain.pem" dest: "{{ nginx_ssl_dir}}/{{ item.ssl_name }}/fullchain.pem" mode: 0644 with_items: "{{ nginx_vhosts }}" when: item.copy_ssl is defined notify: reload nginx ``` I especially don't understand coz for most (all?) CT, I see that `ssl_src_path` and `nginx_ssl_dir` is the same path...

muppeth commented

2024-07-29 08:42:44 +02:00

Owner

I need to look into it. The thing is that although the vars could apear to be the same path, they are pointing to different hosts. However I need to check this properly as it used to be used to copy from ansible container. But yeah, something isn't right here. Since I need to prepare letsencrypt update this week (in next three days in fact) I will look into it as well.

muppeth added this to the 24.07 - July milestone 2024-07-29 08:42:47 +02:00

muppeth added the

sysadmin

housekeeping

labels 2024-07-29 08:42:56 +02:00

muppeth commented

2024-08-01 11:16:31 +02:00

Owner

Ok. Took me a moment to realize what this task actually does lol. Those tasks were created for situation, were you do not use self signed cert or letsencrypt, but you bought a certificate elsewhere and you would like to distribute those. The reason the path seem the same is because src is located on ansible server and dest is on the remote host you play the role on. So although they look similar they dont have to be and this is the reason for two different vars.

We never had a use case for it from the moment letsencrypt has become a thing. Now the proposal for solution would be:

Remove those tasks and just not support certs other then selfsigned or letsencrypt
Rename the tasks to make it clear what it is about (in the future we should provide a proper documentation for the entire role).

Ok. Took me a moment to realize what this task actually does lol. Those tasks were created for situation, were you do not use self signed cert or letsencrypt, but you bought a certificate elsewhere and you would like to distribute those. The reason the path seem the same is because `src` is located on ansible server and `dest` is on the remote host you play the role on. So although they look similar they dont have to be and this is the reason for two different vars. We never had a use case for it from the moment letsencrypt has become a thing. Now the proposal for solution would be: 1. Remove those tasks and just not support certs other then selfsigned or letsencrypt 2. Rename the tasks to make it clear what it is about (in the future we should provide a proper documentation for the entire role).

meaz commented

2024-08-01 19:01:59 +02:00

Author

Owner

I would go for n°2. And update readme too.

muppeth referenced this issue from Disroot-Ansible/nginx

2024-08-01 22:11:16 +02:00

Some clarification about ssl.yml task #70

muppeth commented

2024-08-01 22:14:05 +02:00

Owner

PR done with some suggestion here
I did add some docs to readme file but it's not complete. I do want to write a more detailed readme covering everything around nginx role which is pretty big and very little documented. It's a start, and something we should keep on working when we have some time (matter of doing it little by little for all roles but systematically).

PR done with some suggestion [here](https://git.disroot.org/Disroot-Ansible/nginx/pulls/70) I did add some docs to readme file but it's not complete. I do want to write a more detailed readme covering everything around nginx role which is pretty big and very little documented. It's a start, and something we should keep on working when we have some time (matter of doing it little by little for all roles but systematically).

muppeth referenced this issue from a commit

2024-08-02 10:16:16 +02:00

Some clarification about ssl.yml task (#70)

meaz closed this issue

2024-08-04 15:16:21 +02:00

[Nginx] - Shouldn't ssl_src_path be remplaced by nginx_ssl_dir in ssl.yml task #972