-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add a module for nsldc support. #28
Conversation
Hi, how are we going to use nslcd for SSSD testing? |
This is the one and only setup that used for nslcd setup for current test framework: https://github.com/SSSD/sssd/blob/master/src/tests/multihost/alltests/conftest.py#L754 |
I went over the use cases and it seems that we only use it to test the proxy provider. For this, I think it would be better create a generic function that would configure the proxy provider to SSSDCommonConfiguration. Something like: def proxy(self, domain=None, proxy=$proxy, provider=$provider, role: MultihostRole = None):
"""
... $proxy can be ldap or files
... $provider can be one of sssd providers (id, auth, chpass, ...)
"""
if domain is None:
domain = self.default_domain
match proxy:
case "ldap":
# configure and start nslcd, create pam stack for pam_ldap
# uri is role.host.hostname
# basedn is not needed, nslcd can figure it on its own
case "files":
# create pam stack for pam_unix The usage would be like: @pytest.mark.topology(KnownTopology.LDAP)
@pytest.mark.parametrize("proxy", ["files", "ldap"])
def test_proxy__id(client: Client, ldap: LDAP, proxy: str):
... create users either local or ldap based on proxy value
client.sssd.common.proxy(proxy=proxy, provider="id", role=ldap)
client.sssd.start()
... test |
4b73b8d
to
f77714f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Anuj, thank you for implementing the idea, but there are lots of issues in your code.
https://tests.sssd.io/en/latest/concepts.html#core-concepts-and-coding-style
Please, also fix CI issues. It would be good if you create a habit to automatically check your code before submitting it (testing it since current code will yield error about host.host attribute missing) and checking the coding style - you can just run tox
command to do it for you. See https://tests.sssd.io/en/latest/concepts.html#core-concepts-and-coding-style on what tools you should run.
9f24b70
to
b993031
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also:
- rename commit to "sssd: add common configuration for proxy domain" and change the commit message since it is not really about nslcd
- run black, isort, flake8 and mypy to automatically reformat and validate your code to pass the PR CI and follow the coding style
3f30c5b
to
082faf4
Compare
f40de93
to
e40be03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you Anuj. From my perspective, the logic is there, we just need to move few lines after the if statement and fix types.
a8d1ebc
to
1d34454
Compare
03ee9ac
to
fa55644
Compare
Hi, we were going in a in circle - one thing is fixed, another issue created. So I took the liberty to fix documentation and parameter handling to move on with this pull request. Sample tests: @pytest.mark.topology(KnownTopology.Client)
def test_proxy__files(client: Client):
client.local.user("tuser").add()
client.sssd.common.proxy()
client.sssd.start()
result = client.tools.id(f"tuser@{client.sssd.default_domain}")
assert result is not None
assert result.user.name == "tuser"
@pytest.mark.topology(KnownTopology.LDAP)
def test_proxy__ldap(client: Client, ldap: LDAP):
ldap.user("tuser").add()
client.sssd.common.proxy("ldap", server_hostname=ldap.host.hostname)
client.sssd.start()
result = client.tools.id(f"tuser@{client.sssd.default_domain}")
assert result is not None
assert result.user.name == "tuser" Now we need to add nss-pam-ldapd to containers before pushing this. @sidecontrol can you take care of that? |
@pbrezina , I don't know how to do it yet, but yes, I will figure it out and take care of it. |
4c4f0f8
to
c028570
Compare
Please review the unresolved conversations, if they are already fixed then resolve them. Apart from that rebase on top of master. |
I built nslcd in copr for downstream rhel9 testing and pushed Dan's container patch again. Once it is verified that we didn't break downstream CI again, we can push this. |
Create a module that will configure proxy in client machine.
@sidecontrol do you agree with latest version? |
@pbrezina Yes. @aborah-sudo Thank you. |
Create a module that will configure nslcd in client machine.