Skip to content

Conversation

@ekohl
Copy link
Member

@ekohl ekohl commented Mar 11, 2025

@ehelms as you requested, an example of using systemd socket activation an unix sockets.

It changes the connections to go through Apache, which may or may not be right.

@ehelms
Copy link
Member

ehelms commented Mar 11, 2025

Instead of enforcing Apache to be present and an ordering, could you use the unix_socket parameter of the uri module to do the service up test?

@ekohl
Copy link
Member Author

ekohl commented Mar 11, 2025

Instead of enforcing Apache to be present and an ordering, could you use the unix_socket parameter of the uri module to do the service up test?

I had that for a moment, but the type of the result changes to a dict and didn't want to deal with it. Also didn't know if the smart_proxy module from FAM supports unix sockets. Then I took the easy route and relied on Apache though I kind of agree with you that it'd be nicer if it was more standalone.

On the other hand, perhaps we should rely more on health checks for ready detection. Then the Smart Proxy registration also feels like it doesn't belong in the Foreman role. Perhaps we need to swap the order of Foreman and Foreman Proxy. That stuff is easier in Puppet IMHO.

@ehelms
Copy link
Member

ehelms commented Mar 12, 2025

Then the Smart Proxy registration also feels like it doesn't belong in the Foreman role.

Not that you say that, I am surprised it's in that role as well due to what I would have thought are order of operations. I suppose it's there as the last action with the way the playbook is organized. I think in past iterations where we played with Ansible we made registration it's own role to control it better. Ahh yes, back in the day, we had a spark of an idea.

Then you have #81 which changes things and negates this being here all together I think.

ansible.builtin.uri:
url: 'http://{{ ansible_hostname }}:3000/api/v2/ping'
url: '{{ foreman_url }}/api/v2/ping'
validate_certs: false # TODO
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
validate_certs: false # TODO
ca_path: '{{ ca_certificate }}'

Copy link
Member Author

Choose a reason for hiding this comment

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

When I try this I get:

The conditional check 'foreman_status.status == 200' failed. The error was: error while evaluating conditional (foreman_status.status == 200): 'dict object' has no attribute 'status'. 'dict object' has no attribute 'status'

Copy link
Member

Choose a reason for hiding this comment

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

Weird. In Line 126 you mean?

url: "https://{{ ansible_fqdn }}:9090"
server_url: "http://{{ ansible_fqdn }}:3000"
server_url: "{{ foreman_url }}"
validate_certs: false # TODO
Copy link
Member

Choose a reason for hiding this comment

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

We don't support ca_path today in FAM. Should we?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was looking for it and now I'm wondering if validate_certs: '{{ ca_certificate }}' works. https://docs.python-requests.org/en/latest/user/advanced/#ssl-cert-verification says verify='/path/to/certfile' is supported and looking at https://github.com/theforeman/foreman-ansible-modules/blob/2343e66adf65aac42f2d26a076d555102320bcc6/plugins/inventory/foreman.py#L266 I'd think that works. Not sure about https://github.com/theforeman/foreman-ansible-modules/blob/2343e66adf65aac42f2d26a076d555102320bcc6/plugins/module_utils/foreman_helper.py#L617 but if we go that route, it'd make sense to me to copy requests' API because it saves a parameter.

Copy link
Member

Choose a reason for hiding this comment

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

It saves one, yes, but diverges from ansible.builtin.uri API, which is the one Ansible users are more familiar with.

The problem (today) is https://github.com/theforeman/foreman-ansible-modules/blob/2343e66adf65aac42f2d26a076d555102320bcc6/plugins/module_utils/foreman_helper.py#L373 which makes it a boolean, not a string.

Comment on lines 72 to 74
[Unit]
Requires=foreman.socket
Copy link
Member

Choose a reason for hiding this comment

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

This is the only thing needed for it to do socket activation? 🤯

Copy link
Member

Choose a reason for hiding this comment

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

Mhh, but the tests are failing with error 500, so maybe it isn't?

Copy link
Member

Choose a reason for hiding this comment

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

Does the socket "just" need to be started too?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only thing needed for it to do socket activation? 🤯

Pretty much yes. It ends up setting the right env vars and Puma picks that up because we tell it to (https://github.com/theforeman/foreman/blob/75dc40095546966477a927911e715263c42d1bea/config/puma/production.rb#L35-L38).

Mhh, but the tests are failing with error 500, so maybe it isn't?

I think that's because of async. Looking at the Pulp failures I get the impression the service is started before migrations ran. That's why I'll split this off into 2 PRs.

Does the socket "just" need to be started too?

Starting foreman.service (as we do) will activate the socket so it shouldn't be needed to enable it, but we could. In theory we could only start the socket and let foreman.service start on demand but I'm not sure we want to go that route.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, async was a different patch. Apache fails with:

[Thu Mar 13 18:02:39.887738 2025] [proxy:warn] [pid 707:tid 891] [client 192.168.122.14:52218] AH01144: No protocol handler was valid for the URL / (scheme 'unix'). If you are using a DSO version of mod_proxy, make sure the proxy submodules are included in the configuration using LoadModule.

Turns out the template had a trailing slash and then it couldn't find the right socket anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Starting foreman.service (as we do) will activate the socket so it shouldn't be needed to enable it, but we could. In theory we could only start the socket and let foreman.service start on demand but I'm not sure we want to go that route.

There is a good reason to start from Ansible: we should restart it if a change is made.

@evgeni
Copy link
Member

evgeni commented Mar 13, 2025

Also didn't know if the smart_proxy module from FAM supports unix sockets.

It does not, because requests can't. There is https://github.com/msabramo/requests-unixsocket but I never tried it.

@ekohl
Copy link
Member Author

ekohl commented Mar 13, 2025

Next issue:

type=AVC msg=audit(1741889614.482:20207): avc:  denied  { write } for  pid=58362 comm="httpd" name="foreman.sock" dev="tmpfs" ino=2284 scontext=system_u:system_r:httpd_t:s0 tcontext=system_u:object_r:var_run_t:s0 tclass=sock_file permissive=0

We handled this in our foreman-selinux package but that's no longer installed on the host.

@ekohl ekohl force-pushed the socket-activation branch from 3c95a0d to 19a28df Compare March 13, 2025 18:15
@evgeni
Copy link
Member

evgeni commented Mar 14, 2025

Next issue:

type=AVC msg=audit(1741889614.482:20207): avc:  denied  { write } for  pid=58362 comm="httpd" name="foreman.sock" dev="tmpfs" ino=2284 scontext=system_u:system_r:httpd_t:s0 tcontext=system_u:object_r:var_run_t:s0 tclass=sock_file permissive=0

We handled this in our foreman-selinux package but that's no longer installed on the host.

Nothing prevents us from installing it again.

Or we extend/adjust
https://github.com/theforeman/foreman-quadlet/blob/3c0ec9ca701dc196c693f812a265f558c1e3d511/roles/httpd/tasks/main.yml#L9-L13

audit2allow says daemons_enable_cluster_mode could help? -- Edit: tested. nope it does not

@ekohl ekohl force-pushed the socket-activation branch from 19a28df to 995fb64 Compare March 14, 2025 22:05
@ekohl
Copy link
Member Author

ekohl commented Mar 14, 2025

audit2allow says daemons_enable_cluster_mode could help? -- Edit: tested. nope it does not

If you use /run/httpd.* it gets the httpd_var_run_t type and then the boolean does work. Still doesn't feel right, but if it helps move this forward it can be a decent hack.

@ehelms
Copy link
Member

ehelms commented May 14, 2025

@ekohl Would you mind rebasing when you get a moment?

ekohl added 4 commits May 15, 2025 09:47
This makes it easier to change where the service listens on.
This means certificates are properly verified, both server side and
client side.
@ekohl ekohl force-pushed the socket-activation branch from 995fb64 to 0885411 Compare May 15, 2025 07:51
foreman_oauth_consumer_secret: abcdefghijklmnopqrstuvwxyz123456
foreman_listen_stream: /run/httpd.foreman.sock
foreman_url: "https://{{ ansible_fqdn }}"
httpd_foreman_backend: "unix://{{ foreman_listen_stream }}|http://%{HTTP_HOST}/"
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised all three of these are not the role defaults.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because they're connected: my thought was that the roles should be dependent and only on the playbook level you can relate them.

---
foreman_container_image: "quay.io/evgeni/foreman-rpm"
foreman_container_tag: "nightly"
foreman_listen_stream: localhost:3000
Copy link
Member

Choose a reason for hiding this comment

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

I think what I don't know enough about is why we wouldn't default this to /run/httpd.foreman.sock if our intent is to use systemd sock activation.

@ekohl ekohl mentioned this pull request Sep 5, 2025
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.

3 participants