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

Fix and test IdM related parsers and combiners #4178

Merged
merged 0 commits into from
Aug 8, 2024

Conversation

pbrezina
Copy link
Contributor

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

This pull request fix multiple issues in ipa, sssd and identity_domain. Each commit has its own description about the fix. It also adds extensive tests to verify its functionality.

@jenkins-qa-bot
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@xiangce xiangce left a comment

Choose a reason for hiding this comment

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

@pbrezina - Since the insights-core is a public open-source project, other than the internal projects, there might be other unknown project depends on it, we'd make sure the new changes won't break their usage.

BTW, May I know the context about why you raise this PR, e.g., a Jira card or a customer case?

insights/specs/__init__.py Outdated Show resolved Hide resolved
@@ -727,7 +727,8 @@ class Specs(SpecSet):
sshd_config_d = RegistryPoint(multi_output=True, filterable=True)
sshd_config_perms = RegistryPoint(no_obfuscate=['hostname', 'ip'])
sshd_test_mode = RegistryPoint(filterable=True)
sssd_config = RegistryPoint()
sssd_conf = RegistryPoint()
sssd_conf_d = RegistryPoint(multi_output=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

To add a new spec, please file a RHINENG Jira card with "Component=Insights Spec" to get it approved first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

insights/parsers/sssd_conf.py Outdated Show resolved Hide resolved
@pbrezina
Copy link
Contributor Author

@pbrezina - Since the insights-core is a public open-source project, other than the internal projects, there might be other unknown project depends on it, we'd make sure the new changes won't break their usage.

Sure, I'll look into it.

BTW, May I know the context about why you raise this PR, e.g., a Jira card or a customer case?

I'll send the ticket to you privately, as it is not publicly accessible.

@pbrezina pbrezina force-pushed the idm branch 3 times, most recently from 0c629bd to 94a199e Compare August 5, 2024 12:21
@pbrezina
Copy link
Contributor Author

pbrezina commented Aug 5, 2024

@xiangce Please, see updated patches.

insights/parsers/sssd_conf.py Outdated Show resolved Hide resolved
insights/combiners/sssd_conf.py Show resolved Hide resolved
insights/parsers/sssd_conf.py Outdated Show resolved Hide resolved
@pbrezina
Copy link
Contributor Author

pbrezina commented Aug 6, 2024

Fixed docs.

@JoySnow
Copy link
Collaborator

JoySnow commented Aug 6, 2024

@pbrezina Just a reminder... the PR CI code-tests runs into the flake8 errors, eg. ./insights/combiners/sssd_conf.py:6:46: W605 invalid escape sequence '\*'

@pbrezina
Copy link
Contributor Author

pbrezina commented Aug 6, 2024

@pbrezina Just a reminder... the PR CI code-tests runs into the flake8 errors, eg. ./insights/combiners/sssd_conf.py:6:46: W605 invalid escape sequence '\*'

I removed the asterisk completely. Without escaping it, sphinx will complain. If I escape it, flake will complain.

@pbrezina pbrezina force-pushed the idm branch 2 times, most recently from 65a3bd4 to 0a2550d Compare August 7, 2024 09:51
@xiangce xiangce merged commit 66c6081 into RedHatInsights:master Aug 8, 2024
8 of 9 checks passed
xiangce pushed a commit that referenced this pull request Aug 8, 2024
* fix(parsr): do not auto-create magic methods in __getattr__

The previous code would just provide any attribute, including
the magic methods. Especially __deepcopy__ method that breaks
deepcopy(parser_object).

Signed-off-by: Pavel Březina <[email protected]>

* fix(identity_domain): use correct SSSD realm option

SSSD does not have krb5_domain, but krb5_realm.

Signed-off-by: Pavel Březina <[email protected]>

* fix(identity_domain): do not add realm twice

This realm could have been already added as SSSD domain.

Signed-off-by: Pavel Březina <[email protected]>

* fix(sssd): refactor the code, fix issues and add SSSDConfAll

Existing implementation of SSSD configuration parser did not consinder
SSSD's include directory and other new functionality.

* Add support for sssd.conf and conf.d include folder
* Add SSSDConfAll combiner that merge all configuration files together
* Add support for [domain/$dom]/enabled in addition to [sssd]/domain
* Refactor the code to follow naming convention of other
  parsers/combiners

Signed-off-by: Pavel Březina <[email protected]>

* fix(ipa): simplify code and logic

The current IPA combiner logic was quite complex, difficult to test
and prone to errors.

* It is sufficient to only test the SSSD configuration in order
  to get the server mode, other checks are redundant.
* It is sufficient if there is any SSSD domain with ipa provider
  to get the client mode
* SSSD domain name may differ from the IPA domain name, even though
  this is not the default setup, it can be changed.
* Release information was removed, it make testing unnecessarily
  more complex.
* It is possible to configure SSSD as IPA client without the
  freeipa-client package if one does not use ipa-client-install
  or realm command. Checking if SSSD package is there and IPA
  domain is configured is sufficient.

Signed-off-by: Pavel Březina <[email protected]>

* fix(identity_domain): write extensive tests

...and make krb5 configuration deterministic.

Signed-off-by: Pavel Březina <[email protected]>

---------

Signed-off-by: Pavel Březina <[email protected]>
(cherry picked from commit 66c6081)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants