Bug 1966711

@jjelen any ideas?

We have exactly this covered in the tests/tests_hostkeys.yml

Interestingly enough, it works if you use the "include_role" syntax:

- hosts: localhost
  tasks:
    - name: sshd
      include_role:
        name: redhat.rhel_system_roles.sshd
      vars:
        sshd_hostkey_group: root
        sshd_hostkey_mode: 0600

It is possible that I misunderstood the variable precedence in [1]. The vars/RedHat_8.yml is indeed loaded (and is supposed to get precedence 18 if I see right), but it is supposed to be possible to override with the variables from the playbook (it is with precedence 20), but it looks like plain vars in play have precedence only 12, which is probably the cause.

[1] https://docs.ansible.com/ansible/latest/user_guide/playbooks_variables.html#variable-precedence-where-should-i-put-a-variable

My bad - I should have seen this:

defaults/main.yml:sshd_hostkey_mode: "0600"
tasks/install.yml:        mode: "{{ sshd_hostkey_mode }}"
tests/tests_hostkeys.yml:        sshd_hostkey_mode: "0664"
vars/Fedora.yml:sshd_hostkey_mode: "0640"
vars/Fedora_31.yml:sshd_hostkey_mode: "0640"
vars/RedHat_7.yml:sshd_hostkey_mode: "0640"
vars/RedHat_8.yml:sshd_hostkey_mode: "0640"

This is bad.  You _cannot_ define variables in both defaults and vars files.  If you want to define the default values for variables that users should be able to provide and override.

The general pattern for this is that variables defined in vars files *must be role internal variables* and *must begin with two underscores* as in https://github.com/redhat-cop/automation-good-practices/tree/main/roles#naming-things

Then, in defaults/main.yml, you would set the default value to whatever is the default value for that platform, which was set in the vars/Platform.yml file.  So it would look like this:

defaults/main.yml:sshd_hostkey_mode: "{{ __ssh_hostkey_mode }}"
tasks/install.yml:        mode: "{{ sshd_hostkey_mode }}"
tests/tests_hostkeys.yml:        sshd_hostkey_mode: "0664"
vars/Fedora.yml:__sshd_hostkey_mode: "0640"
vars/Fedora_31.yml:__sshd_hostkey_mode: "0640"
vars/RedHat_7.yml:__sshd_hostkey_mode: "0640"
vars/RedHat_8.yml:__sshd_hostkey_mode: "0640"

Looks like we have the same problem with the ssh role:

defaults/main.yml:ssh_drop_in_name: null
tasks/main.yml:        {% if ssh_drop_in_name is not none and __ssh_supports_drop_in %}
tasks/main.yml:        {% if ssh_drop_in_name is not none and __ssh_supports_drop_in %}
tasks/main.yml:          {{ __ssh_drop_in_template | replace("{name}", ssh_drop_in_name) }}
tests/tests_global_drop_in.yml:        ssh_drop_in_name: 99-last
vars/Fedora.yml:ssh_drop_in_name: "00-ansible"
vars/RedHat_8.yml:ssh_drop_in_name: "00-ansible"
vars/RedHat_9.yml:ssh_drop_in_name: "00-ansible"

This should be 

defaults/main.yml:ssh_drop_in_name: "{{ __ssh_drop_in_name }}"
tasks/main.yml:        {% if ssh_drop_in_name is not none and __ssh_supports_drop_in %}
tasks/main.yml:        {% if ssh_drop_in_name is not none and __ssh_supports_drop_in %}
tasks/main.yml:          {{ __ssh_drop_in_template | replace("{name}", ssh_drop_in_name) }}
tests/tests_global_drop_in.yml:        ssh_drop_in_name: 99-last
vars/Fedora.yml:__ssh_drop_in_name: "00-ansible"
vars/RedHat_8.yml:__ssh_drop_in_name: "00-ansible"
vars/RedHat_9.yml:__ssh_drop_in_name: "00-ansible"
vars/main.yml:__ssh_drop_in_name: null

and in the sshd case you would have to define the platform defaults not specified elsewhere in vars/default.yml - so

vars/default.yml:__sshd_hostkey_mode: "0640"

because the sshd role uses a variant of the "first found" strategy like https://github.com/redhat-cop/automation-good-practices/tree/main/roles#platform-specific-tasks

whereas the ssh role and most of the other system roles use the "override" strategy like https://github.com/redhat-cop/automation-good-practices/tree/main/roles#platform-specific-variables

Thank you for checking. Looks like I misunderstood this somehow. I am afraid this will be in more places than only in case of hostkeys (certainly also sshd_config_*). But good that we have a reproducer.

I created the following changes for the sshd role including a testcases:

https://github.com/Jakuje/ansible-sshd/tree/precedence

Reading through the vars/ files, I see that sshd_packages is using similar schematics, but there is not much a good reason to use this variable as public. For now, it is mentioned in "Secondary role variables", where previously used to be the above variables too (but they should really be in public API).

Let me have a look to the ssh client role too.

(In reply to Jakub Jelen from comment #6)
> Thank you for checking. Looks like I misunderstood this somehow. I am afraid
> this will be in more places than only in case of hostkeys (certainly also
> sshd_config_*). But good that we have a reproducer.
> 
> I created the following changes for the sshd role including a testcases:
> 
> https://github.com/Jakuje/ansible-sshd/tree/precedence
> 
> Reading through the vars/ files, I see that sshd_packages is using similar
> schematics, but there is not much a good reason to use this variable as
> public. For now, it is mentioned in "Secondary role variables", where
> previously used to be the above variables too (but they should really be in
> public API).

Typically the packages and services for the role will be defined in vars as role internal variables https://github.com/linux-system-roles/kernel_settings/blob/master/vars/default.yml - usually it doesn't make sense for the user to set these.
In some rare cases, if you want the user to be able to provide additional packages, you could have a variable defined in defaults/main.yml like "sshd_additional_packages: []", but usually the optional packages to install depend on some optional feature, in which case the role should know which packages go with which feature.  The cockpit role defines different package sets depending on which features are required by the user - 
https://github.com/linux-system-roles/cockpit/blob/master/vars/default.yml


> Let me have a look to the ssh client role too.

Would be glad for reviews on the PRs whether they make sense or if you notice some similar issues elsewhere:

https://github.com/linux-system-roles/ssh/pull/15
https://github.com/willshersystems/ansible-sshd/pull/159

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (rhel-system-roles bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2021:4159