Fixes un-overrideable public api variables

[nkakouros]

This reverts commit f32003f.

That commit hardcoded many variables to the values under vars/, making it impossible for the user to parameterize things like the systemd service name. The assumption was that the __sshd_* variables were useless in an effort to blindly adhere to best practices, but they were crucial in allowing flexibility to the user.

The way the test suite is set up is a bit strange for me, but I tried to make it pass with minimum changes.

[nkakouros]

I cannot fix the failing ansible-lint test. The master branch has the same error, so I guess this PR is ready to review.

[richm]

This breaks some integration tests - the handler that starts/restarts sshd fails because sshd_service is undefined.

RUNNING HANDLER [ansible-sshd : Reload the SSH service] ************************
task path: /home/rmeggins/ansible-sshd/handlers/main.yml:3
Tuesday 16 August 2022  10:51:57 -0600 (0:00:00.000)       0:00:24.908 ********
fatal: [/home/rmeggins/.cache/linux-system-roles/centos-9.qcow2]: FAILED! => {
    "changed": false
}

MSG:

missing parameter(s) required by 'state': name

This is because the test resets sshd_service:

TASK [ansible-sshd : Unset variables to allow running the role more than once] ***
task path: /home/rmeggins/ansible-sshd/tasks/sshd.yml:11
Tuesday 16 August 2022  10:51:57 -0600 (0:00:00.030)       0:00:24.788 ********
...
ok: [/home/rmeggins/.cache/linux-system-roles/centos-9.qcow2] => (item=sshd_service) => {
    "ansible_facts": {
        "sshd_service": null
    },
    "ansible_loop_var": "item",
    "changed": false,
    "item": "sshd_service"
}

This is because https://github.com/willshersystems/ansible-sshd/pull/199/files#diff-3ca6b148feb962b6f4e0c2a016f5f5f22d3b881aa387a29ef466be43fdd537f5R2

- name: Always reset overridden OS defaults
  ansible.builtin.set_fact:
    sshd_clear_role_vars: true

[Jakuje]

I cannot fix the failing ansible-lint test. The master branch has the same error, so I guess this PR is ready to review.

These are fixed by #198 already (known ansible(-lint) bug if I see right).

[Jakuje]

Can you provide a some examples of a playbook that did not work for you, ideally as part of test case here, which could be used as a regression test to make sure this keeps working in the future?

[nkakouros]

This breaks some integration tests - the handler that starts/restarts sshd fails because sshd_service is undefined.

How can I run these tests?

[richm]

This breaks some integration tests - the handler that starts/restarts sshd fails because sshd_service is undefined.

How can I run these tests?

I'm not sure what the ansible-sshd authors would recommend. The linux system roles team is based on Fedora as the development platform. We use tox and tox-lsr to run the tests using qemu/kvm. https://linux-system-roles.github.io/contribute.html
You'll also need a tox.ini like this since ansible-sshd doesn't have one:

[lsr_config]
lsr_enable = true

[lsr_ansible-lint]
configfile = {toxinidir}/.ansible-lint

With this, I can run the tests like this:

tox -e qemu-ansible-core-2.13 -- --image-name centos-9 tests/tests_precedence.yml

To run the test playbook tests/tests_precedence.yml using ansible-core 2.13 against a centos-9 local VM image.

[nkakouros]

I made some changes to address the error with the undefined name. Now, the unsetting of variables is the last thing that happens in the role. It also happens only when it is absolutely needed. I will try to come up with some test case.

[richm]

I still had some failures with 438e010 so I tried to fix it: https://github.com/richm/ansible-sshd/tree/revert-richm
With this code, all of the tests pass.
One thing I don't understand - why does the role take such great pains to reset the public API variables? e.g. https://github.com/richm/ansible-sshd/blob/revert-richm/handlers/main.yml#L33 - what is the use case for this? Something like

include_role:
  name: sshd
vars:
  sshd_config_file: /path/to/1
...
include_role:
  name: sshd
vars:
  sshd_service: my_custom_sshd_service

Where the value sshd_config_file: /path/to/1 will still be in effect for the second run of the role?

[nkakouros]

The reason I reset the public API variables is because in my implementation they become facts. According to Ansible's variable precedence rules, facts take precedence over playbook vars and others. This means that sth like the below snippet will fail:

- hosts: all
  vars:
    sshd_config_file: /custom/config1
  roles:
    - ansible-sshd

- hosts: all
  roles:
    - ansible-sshd 
    # user expects to run with OS default for sshd_config_file,
    # but /custom/config1 is used instead. Here is where resetting
    # the variables to None is useful.

- hosts: all
  vars:
    sshd_config_file: /custom/config2  # this has no effect
  roles:
    - ansible-sshd

I still had some failures with 438e010 so I tried to fix it: https://github.com/richm/ansible-sshd/tree/revert-richm

You are right, using intermediate variables or boolean expressions is the proper solution as it would cover the third case above as well and not need unsetting the variables at all. I started with the assumption that using intermediate variables or logic expression would make the code hard to read and maintain, but not using them breaks user expectations. I will rewrite the PR using intermediate variables.

[nkakouros]

I completely rewrote the PR. The issue was actually much simpler, some defaults (e.g. sshd_service) were not set correctly, making it impossible for the user to override them. I fixes those and I also add it a test to check for multiple runs of the role. I also moved internal variables out of defaults/main.yml.

[nkakouros]

BTW, should __sshd_drop_in_dir and __sshd_main_config_file also be overrideable by the user so that they can create drop in files as they need?

[richm]

lgtm - works locally using qemu/kvm tests - other than the ansible-lint issues, most of which are fixed by #198, I think this is good to go

[Jakuje]

BTW, should __sshd_drop_in_dir and __sshd_main_config_file also be overrideable by the user so that they can create drop in files as they need?

Probably yes, but I think we did not get a request for that yet.

[richm]

tests pass locally with qemu + ansible-core 2.13 + centos-9 vm

[Jakuje]

hopefully last comment regarding the test backup. The rest looks good.

[Jakuje]

I think we are good here. @mattwillsher what do you think? Ready to merge?

[mattwillsher]

I'm happy if you all are - I can follow the reasoning for the change.

Please can someone produce a changelog for this please and I'll get it up to Galaxy.

Slightly off-topic but I've been considering a rewrite of the module to try and consolidate the many changes that have occurred to this code base over the years. I'm getting a little concerned about general stability and side effects of changes.

[Jakuje]

Please can someone produce a changelog for this please and I'll get it up to Galaxy.

The only change since last release was the #201 fixing the auto-tag so I would wait for at least #200 to finalize before doing a new release. For a oneliner, this basically fixes #170 and similar cases, so something like

• Allow overriding less common role variables sshd_packages, sshd_binary, sshd_service and sshd_sftp_server by role user.

Slightly off-topic but I've been considering a rewrite of the module to try and consolidate the many changes that have occurred to this code base over the years. I'm getting a little concerned about general stability and side effects of changes.

I am not sure if a module would help here much, but I dont have much practical experience with modules. For the stability and side effects of the changes, we have tests coverage, but indeed there might slip some issues such as this one.

[mattwillsher]

I mean role (too much Terraform for me these days!).

Perhaps things are ok, I just need to spend more time with the code