-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
LDAP state module improvements #62791
Conversation
Hi @rhansen, thanks for your contribution! It looks like there are some failing tests here. And for the integration tests, perhaps use a docker container for openldap? Something like https://hub.docker.com/r/bitnami/openldap/ should work, and there are some examples around the test suite where we use pytest-salt-factories to spin up docker containers in our test suite! |
Here is an example of using saltfactories for the docker containers -
|
Thanks for the pointer @MKLeb. I'm having a bit of trouble figuring out how to set up tests. The |
Apologies, I should have looked a bit further here. It seems OpenLDAP is already installed into the CI machines (see here). From what I can tell, your integration tests are being skipped because it can't find the ldapadd command. Perhaps we can use the ldap3 module itself (and its add method) to add the entries instead of ldapadd from slapdtest? It all depends what underlying tools it uses to do that. I get that it's kind of meta to use the ldap3 mod to test it, but I think that's our best and quickest route here as long as it doesn't rely on ldapadd under the hood. |
a92293d
to
af7e7c7
Compare
It looks like
It's probably easy to add that package to the golden images, but I think it would be nicer if the tests tested against a Docker container rather than requiring OpenLDAP to be installed on the CI machines. Getting that to work will take some time and effort; I'll start working on that. If you'd prefer, we can do the easy thing first (install |
I agree, docker would be the nice way to do it, and likely the correct way as well. There is an example of how we run a minion in a docker container here. It uses an image built in https://github.com/saltstack/salt-ci-containers. We can't do quite what those do in that repo though, because we likely want to test against a minion running the changes on the current branch, as is the case here for instance. You can probably just pip install the local salt, all the ldap dependencies, and pip install python-ldap. I would imagine you would either create an inline dockerfile and build it each time or create a minimal dockerfile that runs the minion on the local salt and then use that minion to then install the ldap dependencies and python-ldap. Not sure which is better honestly, but for no reason at all I'm leaning towards the second option :) Here is the docs on how the container minion works. Let me know if you need any help! |
af7e7c7
to
b1dad6e
Compare
OK, I think I got it working. It depends on saltstack/salt-ci-containers#3 so the tests will fail until that is merged and an image is published. |
I'll rebase onto latest master and fix some of the lint errors. |
3d5372b
to
5f93900
Compare
Looks like the new |
5f93900
to
772e3e7
Compare
Hey @rhansen, thanks for diving deep into this! From a cursory glance, I'm not sure what is going on there. Maybe I can dig around here tomorrow and look for a fix. I'll let you know as soon as I do. |
772e3e7
to
d48cee0
Compare
Aha! I got it to work. I switched them to module scopes, and also added a line to delete the minion's key after we are done with it (that is what was messing up the netapi tests). Here is my diff, and you'll notice I switched from using salt_cilent to salt_cli just to be a little more explicit and consisten across the test suite.
|
d48cee0
to
4cdf713
Compare
Awesome, thank you! I rebased and pushed the fixes; hopefully the tests will all pass.
Oops, I conflated "package" with "module" again. I meant to do module, but thought it was spelled "package". 🤦
Ah, makes sense! I had expected the master to be cleaned up after module scope (due to confusing package with module) so I didn't think I needed to clean up the key.
I intentionally used Thanks for the help! |
4cdf713
to
55d7132
Compare
20d9858
to
3c09fc9
Compare
Oops, I missed that comment. If you wanted to make that a unit test that should be fine! Also, is there a reason you deleted the old ldap module unit test file? If they were incompatible with your changes, could you add a few more simple ones to make sure we have some coverage there as well? Thanks! Let me know if you need any more help. |
Future commits will modify this class to fix some bugs.
Move all encoding and decoding logic to the `AttributeValueSet` class and consistently use it when reading from or writing to LDAP. This fixes some encoding/decoding corner cases, and improves the API's usability. Technically this is a backwards-incompatible change: `ldap3.search` and `ldap.managed` now return decoded strings when possible. However, it appears that the Salt master (or maybe the master/minion protocol?) automatically decodes returned `bytes` objects to `str` when possible, so this change only affects direct function calls.
3a94258
to
89b405e
Compare
Figured it out: saltstack/pytest-salt-factories#139 I still want to add some more unit tests, and #62932 needs to be merged. Once those are done, this should be ready to go. |
Sorry for not getting back to you quickly, but glad to hear you figured it out.
Going to look at #62932 today and suggest changes (if any). We don't need to solve that salt factories issue first, correct? |
Correct. I have a workaround in place. |
# Bind mount the checked-out source code into the Docker | ||
# container so that a minion daemon started inside the container | ||
# will run the code to be tested. | ||
RUNTIME_VARS.CODE_DIR: { |
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.
Don't use RUNTIME_VARS
, that's something we inherited from the old test suite.
from tests.conftest import CODE_DIR
v = importlib_metadata.version("pytest-salt-factories") | ||
for cmd in [ | ||
("install_packages", "python3-pip"), | ||
("pip", "install", f"pytest-salt-factories=={v}"), |
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.
Salt factories is bind mounted on the minion container
pytest_plugins = [ | ||
"tests.support.pytest.ldap", | ||
] |
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.
pytest_plugins = [ | |
"tests.support.pytest.ldap", | |
] | |
from tests.support.pytest.ldap import openldap_minion_run |
Import the fixture instead.
Closing this due to inactivity. Anyone should feel free to re-open it if they want to see it through to the end in one release cycle. |
What does this PR do?
Various improvements to the
ldap
state module andldap3
execution module:ldap.managed
.python-ldap
generates modification lists.There are now integration tests, which requires OpenLDAP to be installed. The tests use a custom Docker image to get OpenLDAP (see saltstack/salt-ci-containers#3).
What issues does this PR fix or reference?
None.
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
No
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.