feat(policy): Add the ability to do substring search#3551
Draft
c-r33d wants to merge 10 commits into
Draft
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
comp:policy
Contributor
Summary of ChangesHello, 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 introduces a standardized substring search mechanism across the policy service API. By adding a reusable Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe 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
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 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. A search term added to the list, To find the data that we missed. With LIKE or ILIKE in the base, We find the items in their place. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new Search message containing a term field and integrates it as an optional search parameter across multiple list request protobuf definitions in the policy service. It also includes updated documentation and a unit test for search term validation. The reviewer recommends enforcing a minimum length of 1 character on the search term to prevent inefficient database queries with empty strings, along with adding a corresponding test case to verify this validation.
| } | ||
|
|
||
| message Search { | ||
| string term = 1 [(buf.validate.field).string.max_len = 253]; |
Contributor
There was a problem hiding this comment.
To prevent empty or whitespace-only search queries from being sent to the database (which can cause inefficient queries like LIKE '%%'), we should enforce a minimum length of 1 character for the search term when the Search message is provided.
Suggested change
| string term = 1 [(buf.validate.field).string.max_len = 253]; | |
| string term = 1 [(buf.validate.field).string = { | |
| min_len: 1 | |
| max_len: 253 | |
| }]; |
Comment on lines
+12
to
+25
| func TestSearchTermValidation(t *testing.T) { | ||
| v, err := protovalidate.New() | ||
| require.NoError(t, err) | ||
|
|
||
| require.NoError(t, v.Validate(&policy.Search{ | ||
| Term: strings.Repeat("a", 253), | ||
| })) | ||
|
|
||
| err = v.Validate(&policy.Search{ | ||
| Term: strings.Repeat("a", 254), | ||
| }) | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "string.max_len") | ||
| } |
Contributor
There was a problem hiding this comment.
Since we are adding a min_len: 1 constraint to the search term, we should also add a test case to verify that an empty search term fails validation.
Suggested change
| func TestSearchTermValidation(t *testing.T) { | |
| v, err := protovalidate.New() | |
| require.NoError(t, err) | |
| require.NoError(t, v.Validate(&policy.Search{ | |
| Term: strings.Repeat("a", 253), | |
| })) | |
| err = v.Validate(&policy.Search{ | |
| Term: strings.Repeat("a", 254), | |
| }) | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(), "string.max_len") | |
| } | |
| func TestSearchTermValidation(t *testing.T) { | |
| v, err := protovalidate.New() | |
| require.NoError(t, err) | |
| require.NoError(t, v.Validate(&policy.Search{ | |
| Term: strings.Repeat("a", 253), | |
| })) | |
| err = v.Validate(&policy.Search{ | |
| Term: strings.Repeat("a", 254), | |
| }) | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(), "string.max_len") | |
| err = v.Validate(&policy.Search{ | |
| Term: "", | |
| }) | |
| require.Error(t, err) | |
| require.Contains(t, err.Error(), "string.min_len") | |
| } |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
### Proposed Changes 1.) Add substring searching to ListNamespaces, `fqn` field. 2.) Escape characters used by `LIKE\ILIKE` from incoming input 3.) Specify `\` as the escape character ### Checklist - [ ] I have added or updated unit tests - [ ] I have added or updated integration tests (if appropriate) - [ ] I have added or updated documentation ### Testing Instructions <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added search functionality to namespace listing with support for name and FQN matching * Search is case-insensitive and supports prefix matching * Search integrates with namespace state filtering (ACTIVE/INACTIVE) * Special wildcard characters are properly escaped to prevent unexpected matches * **Tests** * Added comprehensive test coverage for search functionality, pagination, and edge cases <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
## Summary - Adds ListAttributes RPC search support by wiring request search into the policy DB list query. - Applies escaped, case-insensitive matching in the attributes SQL path and adds integration coverage for search behavior, wildcard literals, empty search, and pagination after filtering. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added search functionality for attributes, including filtering by fully qualified name with wildcard escaping and combined namespace/state filters. * **Tests** * Added comprehensive integration tests for attribute search operations, including pagination and filter combinations. * **Chores** * Updated sqlc tool to version 1.31.1. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chris Reed <creed@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
## Summary - Adds ListRegisteredResources RPC search support by wiring request search into the policy DB list query. - Applies escaped, case-insensitive matching in the registered resources SQL path and adds integration coverage for search behavior, wildcard literals, namespace filters, empty/whitespace search, and pagination after filtering. --------- Signed-off-by: Chris Reed <creed@virtru.com>
## Summary - Adds ListKeyAccessServers RPC search support by wiring request search into the policy DB list query. - Applies escaped, case-insensitive matching in the KAS registry SQL path and adds integration coverage for search behavior, wildcard literals, empty search, and pagination after filtering. --------- Signed-off-by: Chris Reed <creed@virtru.com>
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
## Summary - Adds ListObligations RPC search support by wiring request search into the policy DB list query. - Applies escaped, case-insensitive matching in the obligations SQL path and adds integration coverage for search behavior, wildcard literals, empty search, and pagination after filtering. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added search capability to obligation listings, supporting search by name and fully qualified name with case-insensitive matching. * Search integrates seamlessly with namespace filters and pagination. * **Tests** * Added comprehensive integration tests for search functionality, including combined filtering, whitespace handling, and pagination behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chris Reed <creed@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
## Summary - Adds ListSubjectConditionSets RPC search support by wiring request search into the policy DB list query. - Applies escaped, case-insensitive matching in the subject condition set SQL path and adds integration coverage for search behavior, wildcard literals, empty search, and pagination after filtering. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added search functionality for subject condition sets filtered by metadata labels. * Search supports pagination, sorting, and combines with namespace filtering. * Search properly handles whitespace and special character escaping. * **Tests** * Expanded integration test coverage for search behavior with metadata labels. * Updated existing sorting tests with improved cleanup utilities. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chris Reed <creed@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
## Summary - Adds ListSubjectMappings RPC search support by wiring request search into the policy DB list query. - Applies escaped, case-insensitive matching in the subject mappings SQL path and adds integration coverage for search behavior, wildcard literals, empty search, and pagination after filtering. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Subject mappings now support search filtering by attribute values and metadata labels * Search results can be combined with namespace filters for refined queries * Automatic whitespace trimming and special character handling ensure accurate searches * Result pagination works correctly with all filters applied <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chris Reed <creed@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
## Summary - Adds ListKeys RPC search support by wiring request search into the policy DB list query. - Applies escaped, case-insensitive matching in the KAS key SQL path and adds integration coverage for search behavior, wildcard literals, empty search, whitespace handling, and pagination after filtering. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Search capability for KAS registry keys by key ID. * Search input automatically trims leading and trailing whitespace. * Wildcard characters in search queries are properly escaped. * Search results work seamlessly with existing filters and pagination. * **Tests** * Added comprehensive test coverage for search functionality. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Chris Reed <creed@virtru.com>
Contributor
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
General goal
Search for policy objects with a
Listreq by specifying a Searchterm. The searchtermis simply that, a word or phrase. We do this by using theLIKEorILIKEcommand depending on the specific RPC.The following sanitization is done for each query:
LIKE\ILIKEsuch as%and_are escaped before being queried.Note
Currently there is no goal to optimize this strategy, this is to serve as a starting point. In the case optimizations are needed
we can look into adding GIN indexes and
pg_trgmfor better fuzziness matching.Implementations
ListNamespacesattribute_fqns.fqn)ListAttributesattribute_fqns.fqn)ListKeyAccessServerskey_access_servers.name), KAS URI (key_access_servers.uri)ListKeyskas_keys.key_id)ListObligations<namespace fqn>/obl/<obligation name>)ListRegisteredResourcesregistered_resources.name)ListSubjectMappingsattribute_fqns.fqn), metadata label values (metadata.labels.*)ListSubjectConditionSetsmetadata.labels.*)