Skip to content

Support LDAP Nested Query - #321

Open
Abhidas747 wants to merge 8 commits into
redhat-data-and-ai:mainfrom
Abhidas747:DATA-6657_nested_ldap_query
Open

Support LDAP Nested Query#321
Abhidas747 wants to merge 8 commits into
redhat-data-and-ai:mainfrom
Abhidas747:DATA-6657_nested_ldap_query

Conversation

@Abhidas747

Copy link
Copy Markdown

Changes

📝 Description

What changed?

The ldap_query in Group CRD previously supported only a flat list of filters under a single boolean operator. This PR introduces a queries field that allows nesting sub-queries with their own operators, enabling compound boolean logic (e.g., OR across title patterns AND-ed with global exclusions). The nesting is implemented as a fixed-depth typed hierarchy (4 levels: LDAPQueryLDAPSubQueryLDAPLeafQueryLDAPLeafSubQuery) with full CRD schema validation at each level.

Why is this change needed?

Real-world LDAP-driven consumer group policies require nested OR + AND combinations (e.g., "(engineers OR developers) AND (manager=X OR manager=Y) AND NOT external"). The previous flat filter model could not express these compound conditions.

Dependencies

  • N/A

🧪 Testing

Test Coverage

  • Unit tests added in pkg/clients/ldap/query_test.go for all nesting scenarios (4-level nesting, queries-only, mixed filters+queries, invalid nested operators, empty leaf filters).
  • Unit tests added in internal/controller/group_controller_manager_query_test.go for manager filter detection, UID extraction, filter replacement, and deduplication across nested structures.
  • All existing controller integration tests (9 specs) continue to pass.
  • Local E2E tested with a nested query CR against live LDAP (Red Hat corp LDAP).

Performance Impact

  • N/A (single LDAP search call unchanged; only the filter string construction is more expressive)

🚀 Deployment

Deploy Steps

  1. Apply updated CRD (make install or deploy updated CRD YAML)
  2. Deploy new controller image

Prerequisites

  • Updated CRD must be applied before any CR using queries field is created

Post-Deployment Monitoring

  • Monitor controller logs for LDAP query build errors on groups using nested queries

Rollback Plan

  • Revert CRD and controller image; existing CRs using only flat filters remain unaffected

⚠️ Breaking Changes

  • This PR contains breaking changes
  • Migration guide provided (if applicable)

Details:

  • N/A (fully backward compatible; existing CRs with only filters work unchanged)

⚙️ Configuration Changes

  • N/A

✅ Developer Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added positive and negative tests that prove my fix is effective or that my feature works
  • Relevant documentation (README, tech specs, etc.) has been added or updated
  • All CI/CD checks are passing

Signed-off-by: Abhidas747 <abhishek.das747@gmail.com>
@gemini-code-assist

Copy link
Copy Markdown

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the LDAP query capabilities of the Group CRD by enabling nested sub-queries. This allows users to define more expressive, compound boolean conditions (e.g., combining multiple OR and AND operations) that were previously limited to a flat filter structure. The changes include updates to the API schema, controller logic for handling recursive query structures, and extensive testing to ensure reliability and backward compatibility.

Highlights

  • Nested LDAP Query Support: Introduced a hierarchical queries field in the Group CRD, allowing for complex, nested boolean logic (AND/OR) in LDAP filters.
  • Schema Validation: Implemented CRD-level validation using x-kubernetes-validations to ensure at least one of filters or queries is defined at every level of the nesting hierarchy.
  • Controller Logic Updates: Refactored the Group controller to recursively traverse nested query structures for manager filter detection and UID extraction, ensuring backward compatibility with existing flat filters.
  • Test Coverage: Added comprehensive unit tests in pkg/clients/ldap/query_test.go and internal/controller/group_controller_manager_query_test.go covering various nesting depths and edge cases.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.


LDAP filters, once so flat, Now grow branches, fancy that! Nested logic, deep and wide, With nowhere for the groups to hide.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for nested LDAP queries in the Usernaut operator. It defines new nested query structures (LDAPSubQuery, LDAPLeafQuery, and LDAPLeafSubQuery) in the API, updates the CRD schemas, and refactors the controller and LDAP client to recursively parse, build, and handle manager filters within these nested query trees. Comprehensive unit tests and sample configurations are also added. The review feedback focuses on strengthening the CRD validation rules to prevent empty filters or queries from bypassing validation, and suggests minor code improvements for consistency and better serialization in the controller's helper functions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread api/v1alpha1/group_types.go Outdated
Comment thread api/v1alpha1/group_types.go Outdated
Comment thread api/v1alpha1/group_types.go Outdated
Comment thread api/v1alpha1/group_types.go Outdated
Comment thread internal/controller/group_controller.go
…f filters or queries is specified and non-empty.

Signed-off-by: Abhidas747 <abhishek.das747@gmail.com>
@vinamra28

Copy link
Copy Markdown
Member

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for nested LDAP queries in the group controller by defining new nested query types (LDAPSubQuery, LDAPLeafQuery, LDAPLeafSubQuery) and updating the CRD schema, deepcopy functions, and query builder to recursively process and validate these nested structures. It also refactors the controller's manager filter detection and replacement logic to support recursive traversal, backed by new unit tests. The review feedback highlights several violations of the repository style guide where functions return nil instead of empty slices when handling empty results, and suggests returning empty slices/maps instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread internal/controller/group_controller.go
Comment thread internal/controller/group_controller.go Outdated
Comment thread internal/controller/group_controller.go Outdated
Comment thread internal/controller/group_controller.go Outdated
Comment thread internal/controller/group_controller.go Outdated
Signed-off-by: Abhidas747 <abhishek.das747@gmail.com>
Signed-off-by: Abhidas747 <abhishek.das747@gmail.com>
Signed-off-by: Abhidas747 <abhishek.das747@gmail.com>
// +optional
// +kubebuilder:validation:Schemaless
// +kubebuilder:pruning:PreserveUnknownFields
LDAPQuery *LDAPQuery `json:"ldap_query,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it possible to have some validation here that checks that if key/criteria etc are provided then ldap_query is not supported? and vice versa?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added validation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Abhidas747 can we add test for that vaalidation?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added test for this validation.

…teria/value and nested LDAP queries.

Signed-off-by: Abhidas747 <abhishek.das747@gmail.com>
…riteria/value and ldap_query

Signed-off-by: Abhidas747 <abhishek.das747@gmail.com>
@Abhidas747
Abhidas747 force-pushed the DATA-6657_nested_ldap_query branch from 2b9d80f to f9bf67a Compare July 13, 2026 13:12
Signed-off-by: Abhidas747 <abhishek.das747@gmail.com>
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.

3 participants