fix: Document and streamline the sshd_main_config_file

[Jakuje]

The option was introduced in f6ae209 without documentation and intended use. The recent change f6ae209 propagated this option to the generated service files, which is resulting in unexpected results, when a user decided to set only sshd_config_file for the second sshd service such as:

• the service file points to the system-wide configuration file
• the system-wide configuration file will get include directive pointing to the directory of the second configuration file

This is an attempt to fix this by introducing some heuristics to guess if the user wants to set up second drop-in directory (ending with .d) or create a standalone configuration file.

Issue Tracker Tickets (Jira or BZ if any): https://issues.redhat.com/browse/RHEL-29309 #280

[nkakouros]

I think I understand the issue now. The problem is that for a distro, the role uses a default sshd_main_config_file, but the user may not want to use that. The user has to explicitly set sshd_main_config_file to None to prevent the new sshd service from using the OS's default. Is this right?

In this case, I wonder if the role could do:

- set_fact:
    sshd_main_config_file: None
  when: sshd_config_file == sshd_main_config_file ~ '.d'

and the rest of the code stays as is already. The above snippet could go into variables.yml.

[Jakuje]

Thank you for having a look into this!

- set_fact:
    sshd_main_config_file: None
  when: sshd_config_file | dirname == sshd_main_config_file ~ '.d'

Yes, this could be the other option, maybe nicer as it would be checked only in one place, unlike what I suggested.

But still I am not sure about the condition sshd_config_file | dirname != sshd_main_config_file | dirname. I would probably keep it in the way I suggested as sshd_config_file | dirname == sshd_main_config_file ~ '.d', what do you think?

I will try to put together some tests coverage for the change here too.

[Jakuje]

But still I am not sure about the condition sshd_config_file | dirname != sshd_main_config_file | dirname. I would probably keep it in the way I suggested as sshd_config_file | dirname == sshd_main_config_file ~ '.d', what do you think?

My bad. All these exist in condition with and sshd_main_config_file is not none so they could be removed altogether if I see right when we will make sure the sshd_main_config_file is None.

[Jakuje]

The problem with the set_fact is that the set fact and variable is propagated to subsequent role invocation, for example if we run the tests like we do now with single ansible-playbook, the set_fact in first invocation is kept around for next test, and later, causing unexpected failures.

We had issues with others variables that were force set as facts in the past. I think the solution was to set only internal variables or to avoid set_fact altogether.

So unless there will be some better ideas, I will revert the changes to the previous version, even though it requires modification on more places. Testing and feedback welcomed.

[nkakouros]

The problem with the set_fact is that the set fact and variable is propagated to subsequent role invocation, for example if we run the tests like we do now with single ansible-playbook, the set_fact in first invocation is kept around for next test, and later, causing unexpected failures.

To work around this you can do:

# as the last task of the role
- set_fact:
    sshd_main_config_file: __sshd_sentinel__

# in variables.yml
- set_fact:
    sshd_main_config_file: "{{ __sshd_main_config_file }}"
  when: sshd_main_config_file == '__sshd_sentinel__'

- set_fact: 
    sshd_main_config_file: None
  when: sshd_config_file | dirname == sshd_main_config_file ~ '.d'

I am not saying this is less cognitive load for a person that reads the role's code, but it keeps things centralized.

[Jakuje]

To work around this you can do:

# as the last task of the role
- set_fact:
    sshd_main_config_file: __sshd_sentinel__

# in variables.yml
- set_fact:
    sshd_main_config_file: "{{ __sshd_main_config_file }}"
  when: sshd_main_config_file == '__sshd_sentinel__'

- set_fact: 
    sshd_main_config_file: None
  when: sshd_config_file | dirname == sshd_main_config_file ~ '.d'

I am not saying this is less cognitive load for a person that reads the role's code, but it keeps things centralized.

I am still undecided if having fact set to placeholder variable is nicer or more fragile. @richm @mattwillsher any thoughts or strong opinions?

[richm]

In general it is a bad idea for the role runtime code to do

set_fact:
  sshd_role_variable: value

for role variables declared in defaults/main.yml - it changes the scope from whatever was the original scope to global, and may cause weird, unexpected behavior if the role is used more than once in a play.

[richm]

any update?

[Jakuje]

I think this is good to go if there are no other concerns

…

On Thu, Apr 4, 2024, 21:29 Richard Megginson ***@***.***> wrote: any update? — Reply to this email directly, view it on GitHub <#281 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUR2WO54GKQ7ZLAQIFLV2DY3WS3NAVCNFSM6AAAAABE4HVNASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZYGA2TIMJQGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[Jakuje]

Thank you for the reviews and comments. Merging then.