Fix OpenLDAP custom queries to use search_scope parameter #21385

puretension · 2025-09-17T18:13:59Z

What does this PR do?

Fixes the OpenLDAP integration to properly use the search_scope parameter in custom queries. The parameter was documented in the configuration but was being ignored during LDAP search operations.

Motivation

Users reported in #20685 that the search_scope parameter in OpenLDAP custom queries was not working as expected. The parameter is documented in conf.yaml.example but the actual search implementation was not using this parameter, making it impossible to control LDAP search scope (base, level, or subtree).

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

- Add support for search_scope parameter in custom queries - Default to 'subtree' when search_scope is not specified - Update existing tests to include search_scope parameter - Add comprehensive test for search_scope functionality Fixes DataDog#20685

LDAP3 library requires constants (ldap3.BASE, ldap3.LEVEL, ldap3.SUBTREE) instead of string values for search_scope parameter.

codecov · 2025-09-17T18:44:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.00%. Comparing base (9063f29) to head (039403f).
⚠️ Report is 21 commits behind head on master.

Additional details and impacted files

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

AAraKKe

Thanks a lot for the PR @puretension ! I just left a couple of small requests, once they are handled I am happy to go ahead with the merge.

My Feedback Legend

Here's a quick guide to the prefixes I use in my comments:

praise: no action needed, just celebrate!
question: I need clarification or I'm seeking to understand your approach.
suggestion: I'm proposing an improvement. This is optional but recommended.
nit: A minor, non-blocking issue (e.g., style, typo). Feel free to ignore.
request: A change I believe is necessary before this can be merged.

The only blocking comments are request, any other type of comment can be applied at discretion of the developer.

AAraKKe · 2025-10-20T12:46:13Z

openldap/datadog_checks/openldap/openldap.py

                self.log.error("`search_filter` field is required for custom query #%s", name)
                continue
            attrs = query.get("attributes")
+            search_scope_str = query.get("search_scope", "subtree")


request: thanks for defining the default in our side! Since the documentation clearly states the default it is good that we are not allowing this default to be chosen by the library implementation.

Can you please keep this default as an ldap3 constant as a class constant instead of hidden here? Something like DEFAULT_SEARCH_SCOPE = ldqp3.SUBREE would be great.

openldap/datadog_checks/openldap/openldap.py

openldap/tests/test_unit.py

- Add SEARCH_SCOPE_MAPPING class constant for efficient lookups - Change invalid scope handling from warning+fallback to error+skip - Add comprehensive parameterized tests for all valid values - Add tests for default behavior and invalid value handling Signed-off-by: puretension <[email protected]>

puretension · 2025-10-22T07:33:52Z

Hi @AAraKKe,

Thank you for the detailed feedback! I've addressed all your requests in the latest commit. I added the DEFAULT_SEARCH_SCOPE class constant using ldap3.SUBTREE instead of the hardcoded string, implemented the SEARCH_SCOPE_MAPPING dictionary for efficient lookups, and changed the error handling to skip invalid queries with proper logging instead of silently falling back to defaults.
For the tests, I created comprehensive parameterized tests covering all valid values in both cases, added a test for default behavior when the option is not supplied, and included a test that verifies invalid values log errors and skip the search entirely.
I noticed your suggested code used DEFAULT_SEARCH_SCOPE as the default value, but since it's an ldap3 constant, I kept the string subtree as the default to avoid issues with the .lower() call. Please let me know if there's anything else you'd like me to adjust!

AAraKKe

Thanks a lot for the follow up @puretension !! I just have a question and a very small request on the updated tests. Would you mind clarifying why the constant is not used? The intention of defining it is to use it there but I am not seeing why the lower method would be an issue.

Once clarified I am happy to merge it.

Thanks!!

My Feedback Legend

Here's a quick guide to the prefixes I use in my comments:

praise: no action needed, just celebrate!
note: just a comment/information, no need to take any action.
question: I need clarification or I'm seeking to understand your approach.
nit: A minor, non-blocking issue (e.g., style, typo). Feel free to ignore.
suggestion: I'm proposing an improvement. This is optional but recommended.
request: A change I believe is necessary before this can be merged.

The only blocking comments are request, any other type of comment can be applied at discretion of the developer.

openldap/datadog_checks/openldap/openldap.py

openldap/tests/test_unit.py

- Replace hardcoded 'subtree' with DEFAULT_SEARCH_SCOPE in main code - Replace ldap3.SUBTREE with DEFAULT_SEARCH_SCOPE in all tests - Ensures centralized management of default search scope value Signed-off-by: puretension <[email protected]>

github-actions · 2025-10-22T12:51:21Z

⚠️ The qa/skip-qa label has been added with shippable changes

The following files, which will be shipped with the agent, were modified in this PR and
the qa/skip-qa label has been added.

You can ignore this if you are sure the changes in this PR do not require QA. Otherwise, consider removing the label.

List of modified files that will be shipped with the agent

openldap/changelog.d/21385.fixed
openldap/datadog_checks/openldap/openldap.py

puretension · 2025-10-22T12:53:53Z

@AAraKKe Thanks for catching that! You're absolutely right! I missed using the constant even though it was already defined.

I've updated both the main code and tests to use DEFAULT_SEARCH_SCOPE instead of hardcoded values. Changed query.get("search_scope", "subtree") to query.get("search_scope", self.DEFAULT_SEARCH_SCOPE) in the main code and replaced all ldap3.SUBTREE with DEFAULT_SEARCH_SCOPE in the test assertions.

As you mentioned, ldap3.SUBTREE is indeed a string so .lower() works perfectly fine. The constant is now centralized and easily recognizable as intended. Thanks for the guidance!

AAraKKe

Thanks for the update @puretension! I left a small nit, will leave to you to decide if you want to change it but it is approved. Will check later and merge if nothing else has changed.

openldap/tests/test_unit.py

Review from AAraKKe is dismissed. Related teams and files:

agent-integrations
- openldap/tests/test_unit.py

AAraKKe · 2025-10-24T15:55:37Z

@puretension I was about to merge and noticed that there is a linting error, can you fix it? I cannot update the branch through the gh cli for some reason.

Signed-off-by: puretension <[email protected]>

Organize imports in test files to comply with ruff linting rules: - tests/conftest.py: Sort imports alphabetically - tests/test_check.py: Sort imports alphabetically - tests/test_unit.py: Sort imports alphabetically Signed-off-by: puretension <[email protected]>

AAraKKe · 2025-10-27T10:14:58Z

Can you please make sure to update your branch with the latest in master to make sure you have the latest version to run the tests? It seems that you fix the issue and force push after I have updated it from the PR which downgrades it again.

Upgrade your branch with the latest from master
Run ddev test --fmt openldap
Commit the current state and push

Thanks!

Signed-off-by: puretension <[email protected]>

puretension · 2025-10-27T14:58:09Z

Hi @AAraKKe,

Thank you for your patience! I've completed all the requested steps.
I updated the branch with the latest master, ran ddev test --fmt openldap (after fixing the ddev configuration),
and pushed the changes.
The linting issues have been resolved and the PR should be ready for merge now.

I really appreciate you teaching me about the ddev test --fmt command. I wasn't familiar with it before.
Sorry for the multiple failed attempts, and thanks for your guidance throughout this process! 🙏

AAraKKe

Thanks @puretension ! All good from my side 🚀

* Fix OpenLDAP custom queries to use search_scope parameter - Add support for search_scope parameter in custom queries - Default to 'subtree' when search_scope is not specified - Update existing tests to include search_scope parameter - Add comprehensive test for search_scope functionality Fixes #20685 * Add changelog entry for search_scope parameter fix * Simplify search_scope test to avoid CI issues * Fix code formatting for linting compliance * Fix search_scope to use ldap3 constants instead of strings LDAP3 library requires constants (ldap3.BASE, ldap3.LEVEL, ldap3.SUBTREE) instead of string values for search_scope parameter. * Fix trailing whitespace for linting compliance * Address reviewer feedback: use mapping dict and improve error handling - Add SEARCH_SCOPE_MAPPING class constant for efficient lookups - Change invalid scope handling from warning+fallback to error+skip - Add comprehensive parameterized tests for all valid values - Add tests for default behavior and invalid value handling Signed-off-by: puretension <[email protected]> * Use DEFAULT_SEARCH_SCOPE constant instead of hardcoded values - Replace hardcoded 'subtree' with DEFAULT_SEARCH_SCOPE in main code - Replace ldap3.SUBTREE with DEFAULT_SEARCH_SCOPE in all tests - Ensures centralized management of default search scope value Signed-off-by: puretension <[email protected]> * Fix code formatting for linting compliance Signed-off-by: puretension <[email protected]> * Fix import sorting in OpenLDAP test files Organize imports in test files to comply with ruff linting rules: - tests/conftest.py: Sort imports alphabetically - tests/test_check.py: Sort imports alphabetically - tests/test_unit.py: Sort imports alphabetically Signed-off-by: puretension <[email protected]> * Fix import formatting for linting compliance Signed-off-by: puretension <[email protected]> * Run ddev test --fmt openldap to fix linting issues Signed-off-by: puretension <[email protected]> --------- Signed-off-by: puretension <[email protected]> Co-authored-by: Juanpe Araque <[email protected]>

puretension requested a review from a team as a code owner September 17, 2025 18:14

temporal-github-worker-1 bot added agent/review-requested ecosystems/review-requested product/review-requested labels Sep 17, 2025

datadog-agent-integrations-bot bot added integration/openldap team/agent-integrations labels Sep 17, 2025

puretension added 5 commits September 18, 2025 03:17

Add changelog entry for search_scope parameter fix

f54168b

Simplify search_scope test to avoid CI issues

a39e31a

Fix code formatting for linting compliance

5f048a2

Fix search_scope to use ldap3 constants instead of strings

4b5c6bd

LDAP3 library requires constants (ldap3.BASE, ldap3.LEVEL, ldap3.SUBTREE) instead of string values for search_scope parameter.

Fix trailing whitespace for linting compliance

366139b

AAraKKe self-assigned this Oct 20, 2025

AAraKKe requested changes Oct 20, 2025

View reviewed changes

temporal-github-worker-1 bot added agent/requested-changes and removed agent/review-requested labels Oct 20, 2025

AAraKKe requested changes Oct 22, 2025

View reviewed changes

openldap/datadog_checks/openldap/openldap.py Outdated Show resolved Hide resolved

openldap/tests/test_unit.py Show resolved Hide resolved

AAraKKe added the qa/skip-qa Automatically skip this PR for the next QA label Oct 22, 2025

AAraKKe previously approved these changes Oct 24, 2025

View reviewed changes

openldap/tests/test_unit.py Show resolved Hide resolved

temporal-github-worker-1 bot added agent/approved and removed agent/requested-changes labels Oct 24, 2025

temporal-github-worker-1 bot added agent/review-requested and removed agent/approved labels Oct 24, 2025

Fix code formatting for linting compliance

18f8403

Signed-off-by: puretension <[email protected]>

puretension force-pushed the fix/openldap-search-scope-parameter-20685 branch 2 times, most recently from f4aa331 to f1f785c Compare October 24, 2025 16:12

puretension force-pushed the fix/openldap-search-scope-parameter-20685 branch from f1f785c to f1139b2 Compare October 24, 2025 16:20

Merge branch 'master' into fix/openldap-search-scope-parameter-20685

8191df1

Fix import formatting for linting compliance

f1db660

Signed-off-by: puretension <[email protected]>

puretension force-pushed the fix/openldap-search-scope-parameter-20685 branch from 4a70bc0 to f1db660 Compare October 27, 2025 14:37

Run ddev test --fmt openldap to fix linting issues

039403f

Signed-off-by: puretension <[email protected]>

AAraKKe approved these changes Oct 29, 2025

View reviewed changes

temporal-github-worker-1 bot added agent/approved and removed agent/review-requested labels Oct 29, 2025

AAraKKe added this pull request to the merge queue Oct 29, 2025

Merged via the queue into DataDog:master with commit f6edc0a Oct 29, 2025
34 of 35 checks passed

AAraKKe mentioned this pull request Oct 29, 2025

OpenLDAP integration custom_queries search_scope option is ignored #20685

Closed

Fix OpenLDAP custom queries to use search_scope parameter #21385

Fix OpenLDAP custom queries to use search_scope parameter #21385

Conversation

puretension commented Sep 17, 2025

What does this PR do?

Motivation

Review checklist (to be filled by reviewers)

Uh oh!

codecov bot commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AAraKKe left a comment

Choose a reason for hiding this comment

Uh oh!

AAraKKe Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

puretension commented Oct 22, 2025

Uh oh!

AAraKKe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

puretension commented Oct 22, 2025

Uh oh!

AAraKKe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AAraKKe commented Oct 24, 2025

Uh oh!

AAraKKe commented Oct 27, 2025

Uh oh!

puretension commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AAraKKe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Sep 17, 2025 •

edited

Loading

AAraKKe left a comment •

edited

Loading

github-actions bot commented Oct 22, 2025 •

edited

Loading

puretension commented Oct 27, 2025 •

edited

Loading