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.

Sign up for GitHub

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

Jump to bottom

A boring version of #7607 #7638

Closed
wants to merge 8 commits into from
Closed

A boring version of #7607 #7638

wants to merge 8 commits into from

Conversation

alexey-tikhonov
Copy link
Member

Fixes #5013 + few minor improvements.

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Oct 9, 2024
@alexey-tikhonov alexey-tikhonov force-pushed the check-socket-activated-2 branch 2 times, most recently from aef1c57 to 78544ac Compare October 9, 2024 18:08
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 9, 2024
View reviewed changes
src/monitor/monitor.c Fixed Show fixed Hide fixed
@alexey-tikhonov alexey-tikhonov force-pushed the check-socket-activated-2 branch from 78544ac to 89fb341 Compare October 9, 2024 18:23
@alexey-tikhonov alexey-tikhonov marked this pull request as ready for review October 9, 2024 20:47
aplopez
aplopez reviewed Oct 10, 2024
View reviewed changes
src/confdb/confdb.c Show resolved Hide resolved
src/confdb/confdb.c Show resolved Hide resolved
src/confdb/confdb.c Show resolved Hide resolved
src/confdb/confdb.h Show resolved Hide resolved
src/monitor/monitor.c Outdated Show resolved Hide resolved
@sumit-bose
Copy link
Contributor

Hi,

thank you for the patch. I wonder if it would be easier to add an SBUS method to the monitor which returns all running services. This will include running socket activated services as well since they register to the monitor, too.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

alexey-tikhonov commented Oct 10, 2024 •
edited
Loading

I wonder if it would be easier to add an SBUS method to the monitor which returns all running services.

To use this method in 'ifp' responder or in 'sssd_check_socket_activated_responders' or both?

For 'sssd_check_socket_activated_responders' this would mean the need to pull in all sbus code, to register on the bus, etc...

@sumit-bose
Copy link
Contributor

I wonder if it would be easier to add an SBUS method to the monitor which returns all running services.

To use this method in 'ifp' responder or in 'sssd_check_socket_activated_responders' or both?

For 'sssd_check_socket_activated_responders' this would mean the need to pull in all sbus code, to register on the bus, etc...

Hi,

yes, I was thinking about sssd_check_socket_activated_responders but I was under the assumption that only client code is needed to send a request to the monitor and receive the reply (basically what dbus-send does plus checking the reply), so no registration on the bus.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

I'd prefer to keep current approach (reading config.ldb) if there are no objections.

@alexey-tikhonov alexey-tikhonov force-pushed the check-socket-activated-2 branch from 89fb341 to 6c1291c Compare October 10, 2024 17:37
@alexey-tikhonov
Copy link
Member Author

Manual testing with:

  • explicitly configured 'pam':
Oct 10 19:59:24 fedora sssd_check_socket_activated_responders[5641]: Misconfiguration found for the 'pam' responder.
                                                                     It has been configured to be socket-activated but it's still menti>
                                                                     Please consider either adjusting services' line or disabling the s>
                                                                     "systemctl disable sssd-pam.socket"
  • implicitly configured 'pac':
Oct 10 19:59:35 fedora sssd_check_socket_activated_responders[5642]: Misconfiguration found for the 'pac' responder.
                                                                     It has been configured to be socket-activated but it's still menti>
                                                                     Please consider either adjusting services' line or disabling the s>
                                                                     "systemctl disable sssd-pac.socket"
  • non-configured 'autofs':
/usr/libexec/sssd/sssd_check_socket_activated_responders -r autofs
echo $?
0

aplopez
aplopez approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@aplopez aplopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

sumit-bose
sumit-bose reviewed Oct 11, 2024
View reviewed changes
src/monitor/monitor.c Outdated Show resolved Hide resolved
@alexey-tikhonov
Move 'nscd' helper functions out of 'utils'
2e32cb5 
as it's not used anywhere outside 'monitor'.
@alexey-tikhonov
CONFDB: introduce helper to read a full list of configured services,
207fe4e 
including implicitly configured
@alexey-tikhonov
IFP: use new helper to retrieve services list
057d801 
This still won't handle socket activated services, but should
take care of implicitly configured services at least.
@alexey-tikhonov
socket_activated_responders: check confdb
c1a1edc 
(instead of sssd.conf) using new helper to take into
account implictly configured services.

Resolves: SSSD#5013
@alexey-tikhonov
socket_activated_responders: log to syslog instead of stdout
4173b6a 
Otherwise logs of 'ExecStartPre' command are lost.
@alexey-tikhonov
TESTS:INTG: 'implicit files domain' not supported
2cffd82 
since 501e05f
@alexey-tikhonov
CONFDB: don't hard fail in add_implicit_services()
a0176b3 
if no explicitly configured domains found.

There are might be 'enable_files_domain = true' or app domains that
are expanded later.
@alexey-tikhonov
CONFDB: mistype fix
0eb64b6
@alexey-tikhonov alexey-tikhonov force-pushed the check-socket-activated-2 branch from 39ed772 to 0eb64b6 Compare October 11, 2024 11:38
sumit-bose
sumit-bose approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@sumit-bose sumit-bose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

thank you for the updates, ACK.

bye,
Sumit

@alexey-tikhonov
Copy link
Member Author

Pushed PR: #7638

  • master
    • 9bb7b92 - CONFDB: mistype fix
    • dbf4763 - CONFDB: don't hard fail in add_implicit_services()
    • 272ee81 - TESTS:INTG: 'implicit files domain' not supported
    • 32e7616 - socket_activated_responders: log to syslog instead of stdout
    • 59c48f7 - socket_activated_responders: check confdb
    • 28bb146 - IFP: use new helper to retrieve services list
    • 7f0f5a6 - CONFDB: introduce helper to read a full list of configured services,
    • c9026bf - Move 'nscd' helper functions out of 'utils'

@alexey-tikhonov alexey-tikhonov mentioned this pull request Oct 14, 2024
Closed
@alexey-tikhonov alexey-tikhonov removed the request for review from pbrezina October 16, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aplopez aplopez aplopez approved these changes

@sumit-bose sumit-bose sumit-bose approved these changes

Assignees

@sumit-bose sumit-bose

@aplopez aplopez

Labels
no-backport This should go to target branch only. Pushed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Duplicate sssd_pac process when using IPA backend
4 participants
@alexey-tikhonov @sumit-bose @aplopez @pbrezina