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.

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

KCM uses its own configuration #6948

Closed
wants to merge 5 commits into from
Closed

Conversation

aplopez
Copy link
Contributor

@aplopez aplopez commented Sep 21, 2023

  • CONFDB: Fixed some missing dependencies in a header file
    This header file was missing some #includes it needs.

  • CONFDB: Allow loading an empty configuration
    This is done with a parameter. The old behavior is kept for the other components than KCM.

  • UTILS: Add the db file name to server_setup()'s parameters
    Helper for the last commit.

  • UTILS: Create a macro for the --config option
    Helper for the last commit.

  • KCM handles its own configuration
    KCM now uses the ${SSS_STATEDIR}/db/config_kcm.ldb database to store its configuration.
    config.ldb is no longer used by KCM.

Resolves: #6926

@aplopez aplopez force-pushed the kcm-config branch 2 times, most recently from 7b02332 to 82b0512 Compare September 22, 2023 11:13
@aplopez aplopez changed the title KCM uses its own configuration and other fixes KCM uses its own configuration Sep 22, 2023
@alexey-tikhonov
Copy link
Member

Please rebase on the top of latest 'master'. Some CI failures should be fixed.

Makefile.am Outdated Show resolved Hide resolved
src/responder/kcm/kcm.c Fixed Show fixed Hide fixed
src/responder/kcm/kcm.c Fixed Show fixed Hide fixed
@alexey-tikhonov
Copy link
Member

What bothers me is that 'config_kcm.ldb' will have entire 'sssd.conf' content while it only needs '[kcm]' section.
Would it be possible to save only [kcm] to ldb?

@aplopez
Copy link
Contributor Author

aplopez commented Sep 27, 2023

What bothers me is that 'config_kcm.ldb' will have entire 'sssd.conf' content while it only needs '[kcm]' section. Would it be possible to save only [kcm] to ldb?

It only has the [kcm] section. load_configuration()'s third parameter is only_section and the value kcm is passed.

@alexey-tikhonov
Copy link
Member

What bothers me is that 'config_kcm.ldb' will have entire 'sssd.conf' content while it only needs '[kcm]' section. Would it be possible to save only [kcm] to ldb?

It only has the [kcm] section. load_configuration()'s third parameter is only_section and the value kcm is passed.

I'm removing it in #6908...
Ok, then I'll have to deal with it.

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Sep 27, 2023
@alexey-tikhonov
Copy link
Member

Now when #6924 was merged, could you please rebase again?

@aplopez
Copy link
Contributor Author

aplopez commented Sep 28, 2023

Now when #6924 was merged, could you please rebase again?

Done

@aplopez aplopez marked this pull request as ready for review September 29, 2023 21:00
@aplopez
Copy link
Contributor Author

aplopez commented Oct 2, 2023

All failing test are doing so because of external reasons.

@justin-stephenson
Copy link
Contributor

KCM configuration is still stored in config.ldb in addition to the new config_kcm.ldb - is it intended?

# ldbsearch -H /var/lib/sss/db/config.ldb -b 'cn=kcm,cn=config'
# record 1
dn: cn=kcm,cn=config
cn: kcm
debug_level: 9
distinguishedName: cn=kcm,cn=config

Will removing --genconf-section/--genconf entirely be done in another PR later?

@alexey-tikhonov
Copy link
Member

Will removing --genconf-section/--genconf entirely be done in another PR later?

Here - #6908

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Oct 6, 2023

KCM configuration is still stored in config.ldb in addition to the new config_kcm.ldb - is it intended?

Ideally 'sssd_kcm' should have its own config file (and there are not many reason to name it 'sssd_' actually).

But transition to new config file should be gradual, we should keep supporting sssd.conf for kcm server - to avoid breaking existing setups.

I'm not sure if it's worth the effort to make sure [kcm] section doesn't get unto sssd's config.ldb...

@aplopez
Copy link
Contributor Author

aplopez commented Oct 6, 2023

KCM configuration is still stored in config.ldb in addition to the new config_kcm.ldb - is it intended?

Ideally 'sssd_kcm' should have its own config file (and there are not many reason to name it 'sssd_' actually).

But transition to new config file should be gradual, we should keep supporting sssd.conf for kcm server - to avoid breaking existing setups.

I'm not sure if it's worth the effort to make sure [kcm] section doesn't get unto sssd's config.ldb...

If we are going that way, I think we should create a separate ticket and do all those changes separately and properly instead of using this PR.

@alexey-tikhonov
Copy link
Member

KCM now uses the /var/lib/sss/db/config_kcm.ldb database ...

This is actually %{sssdstatedir}/db, might be configured differently on other platforms.

@aplopez
Copy link
Contributor Author

aplopez commented Oct 6, 2023

KCM now uses the /var/lib/sss/db/config_kcm.ldb database ...

This is actually %{sssdstatedir}/db, might be configured differently on other platforms.

True. I will correct the description.

@alexey-tikhonov
Copy link
Member

A couple of minor comments, otherwise looks good to me.

Copy link
Contributor

@justin-stephenson justin-stephenson left a comment

Choose a reason for hiding this comment

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

Ack, thank you.

@alexey-tikhonov
Copy link
Member

KCM now uses the /var/lib/sss/db/config_kcm.ldb database ...

This is actually %{sssdstatedir}/db, might be configured differently on other platforms.

True. I will correct the description.

Latest version still references /var/lib/sss/db/config_kcm.ldb'

@aplopez
Copy link
Contributor Author

aplopez commented Oct 9, 2023

KCM now uses the /var/lib/sss/db/config_kcm.ldb database ...

This is actually %{sssdstatedir}/db, might be configured differently on other platforms.

True. I will correct the description.

Latest version still references /var/lib/sss/db/config_kcm.ldb'

Can you please be more specific? Where is /var/lib/sss/db/config_kcm.ldb referenced?

@alexey-tikhonov
Copy link
Member

KCM now uses the /var/lib/sss/db/config_kcm.ldb database ...

This is actually %{sssdstatedir}/db, might be configured differently on other platforms.

True. I will correct the description.

Latest version still references /var/lib/sss/db/config_kcm.ldb'

Can you please be more specific? Where is /var/lib/sss/db/config_kcm.ldb referenced?

In the commit message of KCM: Handle its own configuration patch.

KCM now uses the ${SSS_STATEDIR}/db/config_kcm.ldb database to store its
configuration. config.ldb is no longer used by KCM.

The configuration text file remains the same.

Resolves: SSSD#6926
@aplopez
Copy link
Contributor Author

aplopez commented Oct 9, 2023

In the commit message of KCM: Handle its own configuration patch.

Fixed.

@alexey-tikhonov
Copy link
Member

Thanks, ACK.

@alexey-tikhonov
Copy link
Member

Pushed PR: #6948

  • master
    • 0485342 - KCM: Handle its own configuration
    • e6c1d3a - CONFDB: Fixed some missing dependencies in a header file
    • 7cc28f3 - CONFDB: Allow loading an empty configuration
    • 049edef - UTILS: Add the db file name to server_setup()'s parameters
    • 22f8eee - UTILS: Create a macro for the --config option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport This should go to target branch only. Pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KCM should handle its own configuration itself
3 participants