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.

Sign up for GitHub

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

feat: Allow vhosts to listen on own hosts #256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: Allow vhosts to listen on own hosts #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ apache_global_vhost_settings: |

apache_vhosts:
# Additional properties:
# 'serveradmin, serveralias, allow_override, options, extra_parameters'.
# 'serveradmin, serveralias, allow_override, options, extra_parameters, listen_ip, listen_port'.
- servername: "local.dev"
documentroot: "/var/www/html"

Expand Down
4 changes: 4 additions & 0 deletions molecule/default/converge.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
apache_vhosts:
- servername: "example.com"
documentroot: "/var/www/vhosts/example_com"
- servername: "local.example.com"
documentroot: "/var/www/vhosts/example_com"
listen_ip: 127.0.0.1
listen_port: 8080

pre_tasks:
- name: Update apt cache.
Expand Down
21 changes: 19 additions & 2 deletions templates/vhosts.conf.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,25 @@
{{ apache_global_vhost_settings }}

{# Set up VirtualHosts #}
{% set _extra_listen_list = [] %}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of extra logic inside a template to try to build a list of listen IP/ports, it seems like it will add a lot of overhead to maintaining this role if I were to merge it. Is there any other way to achieve this complexity without adding a ton of hard-to-test logic?

Copy link
Author

@noonedeadpunk noonedeadpunk Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, let me probably explain the usecase better.

Eventually we have series of roles for services that need apache (as one of scenarios when mod_oidc is required for auth). These services by default are running on different ports and listen to specific IPs.

They may (or may not) run on same hosts.

So idea is to include the apache role from these individual service roles when needed and configure series of vhosts on potentially same server.

We already do setup apache in these roles independently, but obviously makes sense to leverage some role instead. Sample of such role vhost:
https://opendev.org/openstack/openstack-ansible-os_keystone/src/commit/d163d9173050a53eb52f3889e8a1cdcb3ce1bfdd/templates/keystone-httpd.conf.j2

So what we currently do there, is just wipe out ports.conf and have all configuration inside vhosts, which makes things flexible and less complex, as you don't need to have Listen defined separately. As the way how ports.conf are configured now makes it kind of very hard to have multiple Listen statements and keep compatibility.
Moving apache_ports_configuration_items from ports.conf to vhosts is possible and will also simplify logic there, but it feels as potentially breaking change in the logic.
Another way around might be replacing lineinfile with blockinfile and placing all options needed inside a block without need to regexp each statement (since you can repeat Listen multiple times - regexp not great). Or do a copy with content at all...

Another complication that comes in picture is Apache 2.2 support. Which I kind of wonder if makes sense at this point of time, given no tested OS does not come with it. And I wonder if any OS that comes with 2.2 has not reached EOL at all this point. And this is second way to reduce complexity I see.

But having listen per vhost and making it configurable makes total sense to ensure the role is reusable, as if it's gonna be included multiple times, you can pass different apache_vhosts_filename making the role so more flexible...

{% for vhost in (apache_vhosts | selectattr('listen_ip', 'defined') + apache_vhosts | selectattr('listen_port', 'defined')) | unique %}
{% set _ = _extra_listen_list.append({
'ip': vhost.listen_ip | default(apache_listen_ip),
'port': vhost.listen_port | default(apache_listen_port)
})
%}
{% endfor %}
{% for listen in _extra_listen_list | unique %}
{% if apache_vhosts_version == '2.2' %}
Listen {{ listen['port'] }}
NameVirtualHost {{ listen['ip'] }}:{{ listen['port'] }}
{% elif apache_vhosts_version == '2.4' %}
Listen {{ (listen['ip'] == '*') | ternary('', listen['ip'] + ':') }}{{ listen['port'] }}
{% endif %}
{% endfor %}

{% for vhost in apache_vhosts %}
<VirtualHost {{ apache_listen_ip }}:{{ apache_listen_port }}>
<VirtualHost {{ vhost.listen_ip | default(apache_listen_ip) }}:{{ vhost.listen_port | default(apache_listen_port) }}>
ServerName {{ vhost.servername }}
{% if vhost.serveralias is defined %}
ServerAlias {{ vhost.serveralias }}
Expand Down Expand Up @@ -36,7 +53,7 @@
{# Set up SSL VirtualHosts #}
{% for vhost in apache_vhosts_ssl %}
{% if apache_ignore_missing_ssl_certificate or apache_ssl_certificates.results[loop.index0].stat.exists %}
<VirtualHost {{ apache_listen_ip }}:{{ apache_listen_port_ssl }}>
<VirtualHost {{ vhost.listen_ip | default(apache_listen_ip) }}:{{ vhost.listen_port_ssl | default(apache_listen_port_ssl) }}>
ServerName {{ vhost.servername }}
{% if vhost.serveralias is defined %}
ServerAlias {{ vhost.serveralias }}
Expand Down