Add molecule configuration for nginx role #58

p-j-smith · 2024-01-16T10:57:21Z

Fixes #50
FIxes #51
Fixes #52

add option to use own template for the nginx configuration (the template to be used can be selected using the nginx_conf_template variable). Define one template:
1. a template for configuring nginx as a reverse proxy for XNAT with an optional additional default server (nginx_reverse_proxy.j2). A default server is created if nginx_add_default_server is true
use flat variable naming for the nginx role rather than nested
update mirsg.infrastructure.install_xnat group vars with new variable names. At the inventory level if nginx_use_ssl: true, we will need to set:
- nginx_server_cert_cache: ssl.server_cert
- nginx_server_key_cache: ssl.server_key
add a molecule configuration for mirsg.infrastructure.nginx. This configures nginx as a reverse proxy for a Flask app running on a Gunicorn server. Include a verify playbook to check we can connect to the server from the localhost via nginx
fix the molecule configuration for mirsg.infrastructure.install_xnat - the tomcat port was being published rather than the one for http connections, so nginx wasn't being used to connect from localhost even though it was configured. Add a verify playbook to check we can connect to the server from the localhost via nginx
add a workflow to run molecule test on the nginx role
add a role README

Also expose and publish ports for nginx

… molecule setup

set correct path to gunicorn don't install unnecessary packages

p-j-smith · 2024-01-16T20:31:16Z

roles/nginx/templates/nginx_xnat.j2

+        server localhost:{{ nginx_upstream_port }};
    }

    server {
-        listen {{ dicom_port }};
+        listen {{ nginx_upstream_listen_port }};


I wasn't sure what to call these variables, perhaps there's a better name?

Variable names look fine to me. I wonder whether this role should XNAT agnostic though - maybe the template should live in the xnat role and here we provide a basic template of a reverse proxy conf (with TLS termination). This is how I would override for OMERO (rather than adding another conf here).

yeah good point, I've tried to change it to make it more general now

drmatthews

Looks great! Just some comments about mentioning XNAT and tomcat - I think we can make this role ignorant of both.

drmatthews · 2024-01-17T08:54:24Z

.github/workflows/molecule-install-xnat.yml

@@ -6,6 +6,7 @@ on:
      - "roles/xnat_container_service/**"
      - "playbooks/install_xnat.yml"
      - "playbooks/install_container_service.yml"


Suggested change

- "playbooks/install_container_service.yml"

- "playbooks/install_container_service.yml"

- "playbooks/molecule/*_xnat"

Do we need the above to catch changes to the molecule configuration?

I think the molecule config changes should be included with the line below (- "playbooks/molecule/**/xnat/**")

drmatthews · 2024-01-17T08:58:07Z

playbooks/molecule/resources/xnat/inventory/group_vars/all/all.yml

 # Support for ipv6
 ipv6_enabled: false

+# nginx config


Should this be in the web.yml group vars?

drmatthews · 2024-01-17T08:59:58Z

roles/nginx/README.md

+| `nginx_group`         | The OS group that will have ownership of the nginx service file and directory. Defaults to `root`                                                                                               |
+| `nginx_log_folder`    | The path to the nginx logs. Defaults to `/var/log/nginx`                                                                                                                                        |
+| `nginx_http_port`     | The port to listen on for HTTP connections. Defaults to `80`                                                                                                                                    |
+| `nginx_https_port`    | The port to listen on for HTTPS connections. Defaults to `80`                                                                                                                                   |


drmatthews · 2024-01-17T09:01:11Z

roles/firewalld/molecule/resources/inventory/group_vars/all.yml

@@ -12,6 +12,7 @@ firewalld_work_zone_open_services:
  - http
  - https
 firewalld_public_zone_ports:
+  - "80"
  - "8080"


Do we need to open 8080?

nope, I've removed it

drmatthews · 2024-01-17T09:03:44Z

roles/nginx/README.md

+| `nginx_http_port`     | The port to listen on for HTTP connections. Defaults to `80`                                                                                                                                    |
+| `nginx_https_port`    | The port to listen on for HTTPS connections. Defaults to `80`                                                                                                                                   |
+| `nginx_proxy_port`    | The port to forward requests to. Defaults to `8080` (tomcat)                                                                                                                                    |
+| `nginx_root`          | The path to search for static files. Defaults to `/usr/share/tomcat/webapps/ROOT`                                                                                                               |


I know this is for convenience but I'm not sure about setting defaults for tomcat (or any specific web framework).

yeah definitely, agreed

drmatthews · 2024-01-17T09:05:56Z

roles/nginx/README.md

+| `nginx_https_port`    | The port to listen on for HTTPS connections. Defaults to `80`                                                                                                                                   |
+| `nginx_proxy_port`    | The port to forward requests to. Defaults to `8080` (tomcat)                                                                                                                                    |
+| `nginx_root`          | The path to search for static files. Defaults to `/usr/share/tomcat/webapps/ROOT`                                                                                                               |
+| `nginx_conf_template` | The template to use for generating the NGINX config. See currently available [templates](templates/). Defaults to `nginx_xnat.j2`, which is used to configure NGINX as a reverse proxy for XNAT |


Again I'm not sure about having an XNAT default. Can we have a basic reverse proxy config?

drmatthews · 2024-01-17T09:15:41Z

roles/nginx/templates/nginx_xnat.j2

+        server localhost:{{ nginx_upstream_port }};
    }

    server {
-        listen {{ dicom_port }};
+        listen {{ nginx_upstream_listen_port }};


Variable names look fine to me. I wonder whether this role should XNAT agnostic though - maybe the template should live in the xnat role and here we provide a basic template of a reverse proxy conf (with TLS termination). This is how I would override for OMERO (rather than adding another conf here).

and move nginx vars into web group

drmatthews

Minor changes, looks good otherwise. I just had one additional question about the provisioner block in the base config files. If a role doesn't need to set any group vars or doesn't have a prepare.yml playbook we'll get an error. Should we move the provisioner block to the role/playbook molecule configs (although this would mean repetition since many of our roles do require group vars and a prepare playbook; doesn't have to be this PR).

playbooks/group_vars/web.yml

playbooks/molecule/resources/xnat/inventory/group_vars/web.yml

playbooks/molecule/resources/xnat/verify.yml

roles/nginx/README.md

roles/nginx/defaults/main.yml

roles/nginx/molecule/resources/inventory/group_vars/all.yml

roles/nginx/molecule/resources/inventory/group_vars/centos7.yml

roles/nginx/molecule/resources/inventory/group_vars/rocky9.yml

Co-authored-by: Daniel Matthews <[email protected]>

p-j-smith · 2024-01-18T13:05:56Z

. I just had one additional question about the provisioner block in the base config files. If a role doesn't need to set any group vars or doesn't have a prepare.yml playbook we'll get an error. Should we move the provisioner block to the role/playbook molecule configs (although this would mean repetition since many of our roles do require group vars and a prepare playbook; doesn't have to be this PR).

I think if there's no prepare playbook then molecule just silently skips that stage rather than failing. Not sure about the group vars though - I can check that and open a separate PR if that is an issue

drmatthews · 2024-01-18T14:17:21Z

I think if there's no prepare playbook then molecule just silently skips that stage rather than failing. Not sure about the group vars though

Having a role without group vars definitely causes an error. I haven't tried having group vars and no prepare but would it skip if we've set a path but the file doesn't exist?

Anyway, let's look at it separately.

drmatthews

Apologies to keep dragging this out, I spotted something I didn't ask last time. Also, I think we should just have the one template for the nginx conf. It looks like the only difference is the server block that sets the default_server in nginx_reverse_proxy_as_default.j2. This is the template that is used in the tests but it isn't the default. Perhaps it should be the default, renamed to nginx_reverse_proxy.j2?

drmatthews · 2024-01-18T14:22:01Z

playbooks/molecule/resources/xnat/inventory/group_vars/web.yml

+
+# nginx config
+nginx_diffie_helman_size_bits: 2048
+nginx_conf_template: nginx_reverse_proxy_as_default.j2


Sorry for confusion, and for not asking this in the last review, but why are we using nginx_reverse_proxy_as_default.j2 in the tests rather than nginx_reverse_proxy.j2?

drmatthews · 2024-01-18T16:02:45Z

roles/nginx/molecule/resources/inventory/group_vars/all.yml

@@ -0,0 +1,7 @@
+---
+nginx_conf_template: nginx_reverse_proxy_as_default.j2


Why not nginx_reverse_proxy.j2?

it's the same issue @paddyroddy was having in #44 in nginx_reverse_proxy.j2 - there is an additional default server, so any traffic to hosts that do not match nginx_server_name are routed to this default server. So in the verify task for this role:

ansible-collection-infra/roles/nginx/molecule/resources/verify.yml

Lines 3 to 11 in b8f4dd1

hosts: localhost

tasks:

- name: Get server status

ansible.builtin.uri:

url: http://localhost:8080

method: GET

validate_certs: false

return_content: true

register: response

the host passed to nginx will be localhost, and as this doesn't match nginx_server_name the request will be sent to the default server (which returns nothing). In the test we could set the correct host through a header (-H 'Host: {{.nginx_server_name }}'), and that would work fine. However, I don't think it's possible to do this in a web browser, so if we run the converge step locally we wouldn't be able to access the web UI. To get around this, @paddyroddy is currently setting the hostname for the webserver to be localhost

roles/nginx/templates/nginx_reverse_proxy_as_default.j2

Co-authored-by: Daniel Matthews <[email protected]>

p-j-smith · 2024-01-19T10:39:32Z

Apologies to keep dragging this out, I spotted something I didn't ask last time.

no worries! Sorry I should have explained what was going on earlier. Hopefully my comment above sheds some light on why the additional default server isn't used in the tests

Also, I think we should just have the one template for the nginx conf. It looks like the only difference is the server block that sets the default_server in nginx_reverse_proxy_as_default.j2. This is the template that is used in the tests but it isn't the default. Perhaps it should be the default, renamed to nginx_reverse_proxy.j2?

Yeah that is probably a better idea. We could have an nginx_add_default_server boolean variable, and if true then the default server is added. I think it should default to true as that's the current behaviour, but the tests will need to set it to false

Remove nginx config template that doesn't define an additional default server This is now handled in one template witht he nginx_add_default_server variable

drmatthews

Looks good - nice work!

paddyroddy · 2024-01-19T11:55:38Z

Can we do a release (soon)?

p-j-smith added 12 commits January 15, 2024 19:24

Add molecule setup for nginx

be5d84e

Fix path to verify playbooks

0089d13

Also expose and publish ports for nginx

Use flat naming rather than nested for nignx role

19df1e5

Use flat naming rather than nested for nignx role

1a1f1bb

use flat variable names in nginx config template

10f502d

Use flat variable names in xnat playbook molecule config

36b0f4b

publish port 80 in xnat molecule config

9e0a55e

add molecule config for nginx role

d75ae33

run gunicorn on localhost

d58fa8b

Update nginx role variables

f4fe7ea

Make ansible lint happy

a5e9ce7

Set permission mode for flask app and gunicorn config

76b5442

p-j-smith marked this pull request as draft January 16, 2024 10:57

p-j-smith added 17 commits January 16, 2024 13:26

Publish nginx port in xnat playbook molecule configuration

ed7337c

use flat variable names for nginx template

51c0c4d

remove stream from reverse proxy config

a18bfac

use tomcat as default server for nginx reverse proxy in xnat playbook…

a8d2f8b

… molecule setup

set defaults for nginx role to be consistent with current values

600c0a4

set nginx variables for xnat playbook molecule setup

f354329

Add verify playbook for nginx role

e0f9576

Fix typo in task description

91e87d1

Add verify file for xnat molecule config

d370b5e

Fix vars for nginx role in xnat playbook

dee847f

Move gunicorn nginx template to testing inventory

2aef7a1

Don't use ssl for nginx in testing the role

9c86fe5

Add molecule setup for testing nginx role on rocky 9

94539a6

Add workflow for testing nginx role with molecule

863b812

Move gunicorn service file to templates

01672b1

set correct path to gunicorn don't install unnecessary packages

Run xnat tests when molecule config changes

fc39135

remove jinja tempalting from verify assertion

0861270

p-j-smith commented Jan 16, 2024

View reviewed changes

drmatthews requested changes Jan 17, 2024

View reviewed changes

p-j-smith added 11 commits January 17, 2024 09:43

rename ipv6_enabled to nginx_ipv6_enabled

0c62327

and move nginx vars into web group

fix typo in nginx readme for https port default

9a56f97

Make nginx xnat configs more general

5dc91c3

make nginx_root optional in the config

108d39f

update install_xnat nginx vars

88351f5

use general nginx config for tests

2ad6fb6

more logical ordering of nginx variables

c077dda

Update description of variables

c7d855d

remove tomcat port from firewall

c69df49

fix name of nginx config tempalte to use for testing xnat playbook

ac8ca63

Merge branch 'main' into tests/nginx

8b0b21e

p-j-smith requested a review from drmatthews January 17, 2024 20:16

drmatthews requested changes Jan 18, 2024

View reviewed changes

remove unnecessary " from string variables

08ef842

Co-authored-by: Daniel Matthews <[email protected]>

p-j-smith self-assigned this Jan 18, 2024

p-j-smith requested a review from drmatthews January 18, 2024 13:19

drmatthews requested changes Jan 18, 2024

View reviewed changes

fix typo in name of proxy server in nginx config template

b8f4dd1

Co-authored-by: Daniel Matthews <[email protected]>

Add nginx_add_default_server variable

02ec02c

Remove nginx config template that doesn't define an additional default server This is now handled in one template witht he nginx_add_default_server variable

p-j-smith requested a review from drmatthews January 19, 2024 11:32

drmatthews approved these changes Jan 19, 2024

View reviewed changes

p-j-smith merged commit 2e26c03 into main Jan 19, 2024
7 checks passed

p-j-smith deleted the tests/nginx branch January 19, 2024 11:47

paddyroddy mentioned this pull request Jan 19, 2024

Allow for any server name #44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add molecule configuration for nginx role #58

Add molecule configuration for nginx role #58

p-j-smith commented Jan 16, 2024 •

edited

Loading

p-j-smith Jan 16, 2024

drmatthews Jan 17, 2024

p-j-smith Jan 17, 2024

drmatthews left a comment

drmatthews Jan 17, 2024

drmatthews Jan 17, 2024

p-j-smith Jan 17, 2024

drmatthews Jan 17, 2024

p-j-smith Jan 17, 2024

drmatthews Jan 17, 2024

drmatthews Jan 17, 2024

p-j-smith Jan 17, 2024

drmatthews Jan 17, 2024

p-j-smith Jan 17, 2024

drmatthews Jan 17, 2024

drmatthews Jan 17, 2024

drmatthews left a comment

p-j-smith commented Jan 18, 2024

drmatthews commented Jan 18, 2024

drmatthews left a comment

drmatthews Jan 18, 2024

drmatthews Jan 18, 2024

p-j-smith Jan 19, 2024 •

edited by paddyroddy

Loading

p-j-smith commented Jan 19, 2024

drmatthews left a comment

paddyroddy commented Jan 19, 2024

	- "playbooks/install_container_service.yml"
	- "playbooks/install_container_service.yml"
	- "playbooks/molecule/*_xnat"

	\| `nginx_https_port` \| The port to listen on for HTTPS connections. Defaults to `80` \|
	\| `nginx_https_port` \| The port to listen on for HTTPS connections. Defaults to `443` \|

		@@ -0,0 +1,7 @@
		---
		nginx_conf_template: nginx_reverse_proxy_as_default.j2

	hosts: localhost
	tasks:
	- name: Get server status
	ansible.builtin.uri:
	url: http://localhost:8080
	method: GET
	validate_certs: false
	return_content: true
	register: response

Add molecule configuration for nginx role #58

Add molecule configuration for nginx role #58

Conversation

p-j-smith commented Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drmatthews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drmatthews left a comment

Choose a reason for hiding this comment

p-j-smith commented Jan 18, 2024

drmatthews commented Jan 18, 2024

drmatthews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-j-smith Jan 19, 2024 • edited by paddyroddy Loading

Choose a reason for hiding this comment

p-j-smith commented Jan 19, 2024

drmatthews left a comment

Choose a reason for hiding this comment

paddyroddy commented Jan 19, 2024

p-j-smith commented Jan 16, 2024 •

edited

Loading

p-j-smith Jan 19, 2024 •

edited by paddyroddy

Loading