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

feat: Allow vhosts to listen on own hosts #256

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

Conversation

noonedeadpunk
Copy link

Current vhosts defenition limit them to listen only on single IP/port.
In the meanwhile, there are usecases where vhosts on the same server
might wanna listen on different quite independent set of IPs/ports,
which is not currently possible.

This patch adds such functionality, and an operator might define
extra properties for apache_vhosts to make them listening on required
ports/interfaces.

@noonedeadpunk noonedeadpunk force-pushed the feature/vhost_listen branch 2 times, most recently from 203206f to 2cc936c Compare July 24, 2024 17:37
Current vhosts defenition limit them to listen only on single IP/port.
In the meanwhile, there are usecases where vhosts on the same server
might wanna listen on different quite independent set of IPs/ports,
which is not currently possible.

This patch adds such functionality, and an operator might define
extra properties for apache_vhosts to make them listening on required
ports/interfaces.
@@ -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

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...

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