refactor: Support with-tlog authselect feature #120

justin-stephenson · 2024-01-25T20:08:03Z

authselect removes with-files-domain feature in F40+/RHEL10+, it is replaced by 'with-tlog' feature because the SSSD files domain is removed entirely there. This will not be backported to RHEL9 therefore we still need to execute 'authselect select sssd with-files-domain' on RHEL9 and older Fedora versions.

justin-stephenson · 2024-01-25T20:10:51Z

Hi @richm Could you help me with this implementation of this PR? I imagine there is a better way than what I have proposed here. The role needs to execute 'authselect select sssd with-tlog' in F40+/RHEL10+ only.

richm · 2024-01-27T00:10:59Z

tasks/main.yml

+  command: authselect select sssd with-tlog --force
+  when:
+    - not ansible_check_mode
+    - tlog_use_sssd


Suggested change

- tlog_use_sssd

- tlog_use_sssd | bool

richm · 2024-01-27T00:16:42Z

Hi @richm Could you help me with this implementation of this PR? I imagine there is a better way than what I have proposed here. The role needs to execute 'authselect select sssd with-tlog' in F40+/RHEL10+ only.

I think this is pretty good - I think you are using "duck typing" to see if authselect has the with-tlog feature instead of using a condition like ansible_facts["distribution_major_version"] | int >= 10.

The only thing I would suggest is to first do the

- name: Check appropriate authselect features exists
  command: authselect list-features sssd
  register: __tlog_authselect_features
  changed_when: false

then have a block which does the stuff that's only applicable when: '"with-tlog" in __tlog_authselect_features.stdout'

justin-stephenson · 2024-01-29T14:25:28Z

Hi @richm Could you help me with this implementation of this PR? I imagine there is a better way than what I have proposed here. The role needs to execute 'authselect select sssd with-tlog' in F40+/RHEL10+ only.

I think this is pretty good - I think you are using "duck typing" to see if authselect has the with-tlog feature instead of using a condition like ansible_facts["distribution_major_version"] | int >= 10.

The only thing I would suggest is to first do the
- name: Check appropriate authselect features exists
  command: authselect list-features sssd
  register: __tlog_authselect_features
  changed_when: false
then have a block which does the stuff that's only applicable when: '"with-tlog" in __tlog_authselect_features.stdout'

Thank you, please check the updated version.

justin-stephenson · 2024-01-29T14:27:38Z

@jakub-vavra-cz For your information.

spetrosi · 2024-01-30T09:17:01Z

tasks/main.yml

+    - tlog_use_sssd | bool
+    - '"with-tlog" in __tlog_authselect_features.stdout'
+
+- name: Check which authselect features are currently enabled
  command: authselect current
  register: __tlog_authselect_current
  changed_when: false
  failed_when: __tlog_authselect_current.rc not in [0, 2]



Suggested change

extra blank line

spetrosi · 2024-01-30T09:19:47Z

tests/check_sssd_with_tlog.yml

@@ -0,0 +1,28 @@
+---


Test should be a playbook, and you have a list of tasks. Check https://github.com/linux-system-roles/tlog/blob/main/tests/tests_cockpit.yml

Test playbook filenames must match named tests_*.yml
I thought perhaps that check_sssd_with_tlog.yml was going to be a task file called from other playbooks because it does not call the tlog role, and you just had not yet changed the other test playbooks to use check_sssd_with_tlog.yml

Ahh okay, I see now. I fixed it so that check_sssd_with_tlog is called from tests/tests_sssd.yml

spetrosi · 2024-01-30T09:20:50Z

tests/check_sssd_with_tlog.yml

+    that:
+      - __nsswitch_contents | regex_search('^passwd:\\s+sss', multiline=True)
+      - '"with-tlog" in __tlog_authselect_current.stdout'
+  when:


Suggested change

when:

when: '"with-tlog" in __tlog_authselect_features.stdout'

spetrosi · 2024-01-30T09:20:59Z

tests/check_sssd_with_tlog.yml

+      - __nsswitch_contents | regex_search('^passwd:\\s+sss', multiline=True)
+      - '"with-tlog" in __tlog_authselect_current.stdout'
+  when:
+    - '"with-tlog" in __tlog_authselect_features.stdout'


Suggested change

- '"with-tlog" in __tlog_authselect_features.stdout'

spetrosi · 2024-01-30T09:21:39Z

tests/check_sssd_with_tlog.yml

+  set_fact:
+    __nsswitch_contents: "{{ __nsswitch_slurp['content'] | b64decode }}"
+
+- name: Check if files domain enabled and nsswitch set correctly


Should there be an opposite check for systems where "with-tlog" not in __tlog_authselect_features.stdout?

If with-tlog feature is not available then we fallback to with-files-domain feature, which we already have a test for tests/check_sssd_files_provider.yml

richm · 2024-01-30T14:36:39Z

tests/check_sssd_with_tlog.yml

+    that:
+      - __nsswitch_contents | regex_search('^passwd:\\s+sss', multiline=True)
+      - '"with-tlog" in __tlog_authselect_current.stdout'
+  when: - '"with-tlog" in __tlog_authselect_features.stdout'


Suggested change

when: - '"with-tlog" in __tlog_authselect_features.stdout'

when: '"with-tlog" in __tlog_authselect_features.stdout'

richm · 2024-01-30T14:51:14Z

tasks/main.yml

  command: authselect current
  register: __tlog_authselect_current
  changed_when: false
  failed_when: __tlog_authselect_current.rc not in [0, 2]
+  notify: Handler tlog_handler restart sssd
+  changed_when: true


You have both changed_when: false and changed_when: true - if this command doesn't change anything, you should have changed_when: false and no notify

tests/check_sssd_with_tlog.yml

tasks/main.yml

richm · 2024-01-30T15:04:01Z

tests/check_sssd_with_tlog.yml

+    src: /etc/nsswitch.conf
+  register: __nsswitch_slurp
+
+- name: Decode nsswitch content


since you are using vars below, you can get rid of this task

richm · 2024-01-30T15:05:38Z

@justin-stephenson how should we document this change? Is this something users should know about, or is just something that the role takes care of for the user that the user doesn't have to know about? This doesn't seem like a bug fix or a new feature - could it be considered a refactoring?

authselect removes with-files-domain feature in F40+/RHEL10+, it is replace with the 'with-tlog' feature.

justin-stephenson · 2024-01-30T15:08:34Z

@justin-stephenson how should we document this change? Is this something users should know about, or is just something that the role takes care of for the user that the user doesn't have to know about? This doesn't seem like a bug fix or a new feature - could it be considered a refactoring?

It might be worthwhile to mention this change briefly, but I don't think it is important to users of this role to know about. It is mostly changing internals here(how session recording is configured to work with SSSD + tlog).

richm · 2024-01-30T15:10:39Z

[citest]

richm reviewed Jan 27, 2024

View reviewed changes

justin-stephenson force-pushed the authselect_with_tlog branch from 890ad1f to 05ec765 Compare January 29, 2024 14:24

spetrosi reviewed Jan 30, 2024

View reviewed changes

justin-stephenson force-pushed the authselect_with_tlog branch 3 times, most recently from c73d8de to 9003a4e Compare January 30, 2024 14:30

richm reviewed Jan 30, 2024

View reviewed changes

justin-stephenson force-pushed the authselect_with_tlog branch from 9003a4e to 9499118 Compare January 30, 2024 14:42

richm reviewed Jan 30, 2024

View reviewed changes

tests/check_sssd_with_tlog.yml Show resolved Hide resolved

justin-stephenson force-pushed the authselect_with_tlog branch from 9499118 to 069262e Compare January 30, 2024 15:00

richm reviewed Jan 30, 2024

View reviewed changes

tasks/main.yml Show resolved Hide resolved

richm reviewed Jan 30, 2024

View reviewed changes

Support with-tlog authselect feature

11ad0c8

authselect removes with-files-domain feature in F40+/RHEL10+, it is replace with the 'with-tlog' feature.

justin-stephenson force-pushed the authselect_with_tlog branch from f5c6f6e to 11ad0c8 Compare January 30, 2024 15:07

richm changed the title ~~Support with-tlog authselect feature~~ refactor: Support with-tlog authselect feature Jan 30, 2024

richm merged commit 7ef1794 into linux-system-roles:main Jan 31, 2024
20 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Support with-tlog authselect feature #120

refactor: Support with-tlog authselect feature #120

justin-stephenson commented Jan 25, 2024

justin-stephenson commented Jan 25, 2024

richm Jan 27, 2024

richm commented Jan 27, 2024

justin-stephenson commented Jan 29, 2024

justin-stephenson commented Jan 29, 2024

spetrosi Jan 30, 2024

justin-stephenson Jan 30, 2024

spetrosi Jan 30, 2024

justin-stephenson Jan 30, 2024

richm Jan 30, 2024 •

edited

Loading

justin-stephenson Jan 30, 2024

spetrosi Jan 30, 2024

justin-stephenson Jan 30, 2024

spetrosi Jan 30, 2024

justin-stephenson Jan 30, 2024

spetrosi Jan 30, 2024

justin-stephenson Jan 30, 2024

richm Jan 30, 2024

justin-stephenson Jan 30, 2024

richm Jan 30, 2024

justin-stephenson Jan 30, 2024

richm Jan 30, 2024

justin-stephenson Jan 30, 2024

richm commented Jan 30, 2024

justin-stephenson commented Jan 30, 2024

richm commented Jan 30, 2024

	when:
	when: '"with-tlog" in __tlog_authselect_features.stdout'

	when: - '"with-tlog" in __tlog_authselect_features.stdout'
	when: '"with-tlog" in __tlog_authselect_features.stdout'

refactor: Support with-tlog authselect feature #120

refactor: Support with-tlog authselect feature #120

Conversation

justin-stephenson commented Jan 25, 2024

justin-stephenson commented Jan 25, 2024

Choose a reason for hiding this comment

richm commented Jan 27, 2024

justin-stephenson commented Jan 29, 2024

justin-stephenson commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richm Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richm commented Jan 30, 2024

justin-stephenson commented Jan 30, 2024

richm commented Jan 30, 2024

richm Jan 30, 2024 •

edited

Loading