[PM-17562] Add integration filter support #5971
Conversation
Co-authored-by: Matt Bishop <mbishop@bitwarden.com>
…or-event-integrations
…or of single bacticks for JSON
…or-event-integrations
…or-event-integrations
…or-event-integrations
|
New Issues (5)Checkmarx found the following issues in this Pull Request
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5971 +/- ##
==========================================
+ Coverage 47.68% 51.11% +3.42%
==========================================
Files 1693 1697 +4
Lines 75080 75243 +163
Branches 6759 6781 +22
==========================================
+ Hits 35801 38457 +2656
+ Misses 37823 35261 -2562
- Partials 1456 1525 +69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
The unit tests for the filtering help a lot because I don't necessarily feel qualified to review all that expression matching logic! I left a few comments and couldn't help myself with some documentation ⛏️s but I think this is a workable direction. Performance is gonna be crucial so I don't know if we try to figure out a cache first or after this.
Lemme know about desired next steps like doing the database work.
src/Core/AdminConsole/Models/Data/EventIntegrations/IntegrationFilterOperation.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Models/Data/Organizations/OrganizationIntegrationConfigurationDetails.cs
Show resolved
Hide resolved
| if (configuration.Filters is string filterJson) | ||
| { | ||
| // Evaluate filters - if false, then discard and do not process | ||
| var filters = JsonSerializer.Deserialize<IntegrationFilterGroup>(filterJson) | ||
| ?? throw new InvalidOperationException($"Failed to deserialize Filters to FilterGroup"); | ||
| if (!integrationFilterService.EvaluateFilterGroup(filters, eventMessage)) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎨 Any and all optimizations for speed here will be important. I would think we could abstract away the validation when asking for filters so that it happens once on load and be done. Does all this happen when we introduce a cache?
There was a problem hiding this comment.
This is just making sure we parsed the filters correct from the DB (i.e. we didn't store a bad filter). The filters themselves are parsed and cached when the IntegrationFilterService starts.
This filters column would be part of the DB cache. I wonder if there's a way for us to cache the filters in addition where we run a filter before even looking to the DB cache? If we think a lot of operations might get filtered out, that might be a solid optimization.
I guess that also raises another question of if there should be maybe a larger scope for filters? Right now, I'm allowing for filters at the event type level which is the most flexible. But it is also one of the more granular (i.e. if I have the same filter on every event type, I'd have to process and store that on all of them with no optimization across the whole integration / organization)
src/Core/AdminConsole/Services/Implementations/EventIntegrations/EventIntegrationHandler.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/EventIntegrations/README.md
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/EventIntegrations/README.md
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/EventIntegrations/README.md
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/EventIntegrations/README.md
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/EventIntegrations/README.md
Show resolved
Hide resolved
| Equals, | ||
| NotEquals, | ||
| In, | ||
| NotIn |
There was a problem hiding this comment.
⛏️ We should assign the numerical value to these enum members
| private delegate bool CompiledFilter(EventMessage message, object? value); | ||
|
|
||
| private readonly Dictionary<string, CompiledFilter> _equalsFilters = new(); | ||
| private readonly Dictionary<string, CompiledFilter> _inFilters = new(); | ||
|
|
||
| public IntegrationFilterService() | ||
| { | ||
| BuildFilters(); | ||
| } |
There was a problem hiding this comment.
This feels like it would fit better encapsulated in its own class, including AddEqualityFilter and AddInFilter
There was a problem hiding this comment.
In the latest commit, I pulled out the creation of expressions into IntegrationFilterFactory. So now the factory is responsible for creating the expressions and the service is responsible for groups and rules (and simply executing the expressions, not building them).
Yep, and we do. None of this is on anywhere but dev and it's not running in our cloud instance since no Service Bus is configured. |
* [PM-17562] Add database support for integration filters * Respond to PR review - fix database scripts * Further database updates; fix Filters to be last in views, stored procs, etc * Fix for missing nulls in stored procedures in main migration script * Reorder Filters to the bottom of OrganizationIntegrationConfiguration
|
I may still be several days away from getting to this, so I've requested a review from @r-tome and @jrmccannon in case they have the capacity to look at it before me. I definitely think it's worth the additional review though. At a glance, is there a UI for configuring integrations (including the new filter capability), or are you just editing the db directly at this stage? |
| var properties = new[] | ||
| { | ||
| "UserId", | ||
| "InstallationId", | ||
| "ProviderId", | ||
| "CipherId", | ||
| "CollectionId", | ||
| "GroupId", | ||
| "PolicyId", | ||
| "OrganizationUserId", | ||
| "ProviderUserId", | ||
| "ProviderOrganizationId", | ||
| "ActingUserId", | ||
| "SecretId", | ||
| "ServiceAccountId" | ||
| }; |
There was a problem hiding this comment.
These properties could be moved to a static readonly field
Sorry I missed this comment earlier. There is no UI for adding these yet, but there is an API - |
|
🎟️ Tracking
PM-17562
📔 Objective
This is a POC of an idea for adding arbitrary filters to our existing event integrations. The idea would be to give organization admins the ability to add a filter to a configuration that would filter out events (e.g. I want to listen for these key events, but only when it happens in a specific set of high priority collections). I'm new to expression trees and this is a first draft POC, so feel free to provide as much feedback as you want.
Here's the basic structure of the idea:
OrganizationIntegrationConfigurationtable - alongside where we store Template. (Note: I haven't added this piece yet since I wanted to get feedback first on this approach. The property I pull out isstring?so for now it's fine and I'm just doing a bunch in unit tests)alloranyin evaluating them. Rules are just the property, the operation and the value. So you can say "CollectionId, In, a list of values". Or "CollectionId, Equals, exact value". Right now I have in, equals and before/after for dates.EventIntegrationHandlerpulls the DB configuration, this would just be another column in the object that comes back (and would get cached as well when we get to that). So no extra load on the DB or new call.EventIntegrationHandlerserializes the Filters property and hands it to the filter service. The filter service has a set of compiled expression trees that it can use to quickly run the filter calculation. Since we have a limited set of properties onEventMessagewe can build a handful of useful expressions to evaluate. If we get a better idea of what admins might like to be filtering, we could probably adjust that as well.EventIntegrationHandler- if it's false, we simply move on and never publish the message that would kick off the integration. If it's true, we proceed as we would have previously.So net-net, this should give us somewhat arbitrary filters without really any performance change to what we're doing now. We don't add any more load on the DB and we hopefully don't add much performance load to the handler due to the expression trees.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes