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

SSH is not automatically reloaded when included via include_role due to Ansible limitations #301

Closed
micolous opened this issue Oct 29, 2024 · 6 comments · Fixed by #303
Closed

Comments

@micolous
Copy link

When using this module via include_role on an Ubuntu 20.04.6 target, SSH is not automatically reloaded on config change.

This is because of an Ansible limitation: ansible/ansible#26537 ansible/proposals#136

My task definition (below) is itself included via include_tasks, because I'm using another pre-canned playbook for the system:

- name: Move sshd to port 222
  include_role:
    name: willshersystems.sshd
  vars:
    sshd_Port: 222
    sshd_manage_service: true
    sshd_allow_reload: true

When running the playbook, I can see it's updated the config successfully (in /etc/ssh/sshd_config.d/00-ansible_system_role.conf), but ss -tnl still shows sshd listening on [::]:22.

The Ansible debugging output seems to indicate that it's tried to validate the new config, and then start sshd if it wasn't already running (which is a no-op), but config changes don't seem to trigger Reload_sshd at all.

I've worked around this in my task definition by adding another step to manually reload sshd:

- name: Reload SSH config
  ansible.builtin.service:
    name: "ssh"
    state: reloaded

Expected behaviour

  • The docs mention this limitation for include_role
  • This module has some other way to trigger a service reload which doesn't depend on notifications

Versions

  • Ansible: 2.17.5
  • willshersystems.sshd: 0.25.0
@Jakuje
Copy link
Collaborator

Jakuje commented Dec 16, 2024

Thank you for the report!

I do not think there is any other simple way for us to trigger the reload effectively without handlers. We could theoretically "emulate" handlers by setting some facts/variables through the play, but this does not sound like very elegant solution.

Would calling the meta: flush_handlers at the end of the role play solve the issue? Or is there some other proposal how this could be improved instead of mentioning this as a limitation?

@richm any thoughts?

@richm
Copy link
Collaborator

richm commented Dec 16, 2024

Thank you for the report!

I do not think there is any other simple way for us to trigger the reload effectively without handlers. We could theoretically "emulate" handlers by setting some facts/variables through the play, but this does not sound like very elegant solution.

Would calling the meta: flush_handlers at the end of the role play solve the issue? Or is there some other proposal how this could be improved instead of mentioning this as a limitation?

@micolous our recommendation would be to use meta: flush_handlers instead of doing the reload

@richm any thoughts?

@Jakuje
Copy link
Collaborator

Jakuje commented Dec 16, 2024

Would calling the meta: flush_handlers at the end of the role play solve the issue? Or is there some other proposal how this could be improved instead of mentioning this as a limitation?

@micolous our recommendation would be to use meta: flush_handlers instead of doing the reload

Would calling it from inside of the role before returning cause some issues (except for making sure the handlers are invoked?).

@richm
Copy link
Collaborator

richm commented Dec 16, 2024

Would calling the meta: flush_handlers at the end of the role play solve the issue? Or is there some other proposal how this could be improved instead of mentioning this as a limitation?

@micolous our recommendation would be to use meta: flush_handlers instead of doing the reload

Would calling it from inside of the role before returning cause some issues (except for making sure the handlers are invoked?).

That's generally not recommended in my experience with Ansible.

There are two cases:

  • The role needs to restart a service based on some sort of configuration change - in this case, the role knows it needs to restart because the user set some special configuration parameter, or the role cannot continue with restarting the service - for example, the fapolicyd role https://github.com/linux-system-roles/fapolicyd/blob/main/tasks/main.yml#L88

  • The user needs to restart a service before the play can continue - the role has told the handler that the service needs to be restarted, but doesn't know when that should happen, but the user knows when - in this case, the user can flush the handlers after calling the role

The drawback with flush_handlers is that ALL of the handlers in the current play will be run - so if you have called many roles, you may have a situation where a bunch of services are being restarted, and perhaps you only wanted one or a few of those handlers to be called.

If we really do need for the user to be able to say "I want the sshd role to restart the service if something has changed immediately after making that change" then we need to introduce some additional logic like this:

- name: Make some config change
  template: ...
  register: __role_config

- name: Notify handler
  debug:
    msg: Handler notified
  notify: handler
  when:
    - __role_config is changed
    - not role_restart_service

- name: Restart service
  service:
    name: servit
    state: restarted
  when:
    - __role_config is changed
    - role_restart_service | bool

@Jakuje
Copy link
Collaborator

Jakuje commented Dec 17, 2024

So this really sounds to me like something that should be mentioned in documentation. Even though this might be clear to experienced ansible users and it might not affect users using just a single role in most common cases, some might get burnt by this.

@mattwillsher
Copy link
Member

It's covered in the Ansible docs under playbook handlers. Going from this design, the author of the playbook should start a seperate play if sshd needs to be restarted before some other tasks happen - the play isn't complete until it ends.

It's worth a mention in the docs with a link to the upstream for clarity.

I don't think we should try to restart sshd in the role beyond the user of handlers.

Jakuje added a commit to Jakuje/ansible-sshd that referenced this issue Dec 18, 2024
@mattwillsher mattwillsher linked a pull request Dec 19, 2024 that will close this issue
@Jakuje Jakuje closed this as completed in 14bf764 Dec 19, 2024
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 a pull request may close this issue.

4 participants