refactor: improve clarity of filter matching logic #12
Conversation
Signed-off-by: NxPKG <iconmamundentist@gmail.com>
Reviewer's Guide by SourceryThis PR refactors the equality struct and its related code to improve naming clarity. The changes focus on renaming two fields in the equality struct to better reflect their purpose, with corresponding updates to comments and usage throughout the code. Updated class diagram for the equality structclassDiagram
class equality {
uint64 equalsInPolicies
uint64 keyUsedInPolicies
}
note for equality "Renamed fields for clarity"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe changes involve renaming and updating methods and test cases across several files to improve clarity and align with new naming conventions. In Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @NxPKG - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please fill out the PR description template with details about the motivation for these naming changes and how to verify them. This will help reviewers and future maintainers understand the context of the changes.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: NxPKG <iconmamundentist@gmail.com>
Signed-off-by: NxPKG <iconmamundentist@gmail.com>
Signed-off-by: NxPKG <iconmamundentist@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
pkg/filters/uint.go (1)
Line range hint
215-220: Approve name change and suggest adding documentation.The rename from
FilterOuttoMatchIfKeyMissingis a good improvement in clarity. However, the method would benefit from documentation explaining its purpose and return value semantics.Add documentation above the method:
+// MatchIfKeyMissing determines if the filter should match when a key is missing. +// Returns false if the filter only has equality conditions (no ranges or inequality conditions), +// indicating that only specific values should match. +// Returns true if the filter has any non-equality conditions, indicating a broader match scope. func (f *UIntFilter[T]) MatchIfKeyMissing() bool {pkg/filters/string_test.go (1)
177-177: LGTM! Comprehensive test coverage for different operator combinations.The test cases thoroughly verify the
MatchIfKeyMissingbehavior with different combinations of equality and inequality operators. All important scenarios are covered:
- Mixed (=, !=) operators
- Mixed (!=, =) operators
- All inequality (!=) operators
Consider adding a table-driven test for these cases to make the test more maintainable and easier to extend. Here's a suggested structure:
testCases := []struct { name string expressions []string expected bool }{ { name: "all equality operators", expressions: []string{"=some", "=word", "=here"}, expected: false, }, // ... add other cases }Also applies to: 188-188, 199-199
pkg/policy/ebpf.go (1)
629-708: Refactor to reduce code duplication incomputePoliciesConfigfunctionThe
computePoliciesConfigfunction contains repetitive code blocks for setting flags inPoliciesConfig. Consider refactoring this code by iterating over a collection of filter configurations or using helper functions to improve maintainability and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
pkg/filters/string_test.go(5 hunks)pkg/filters/uint.go(1 hunks)pkg/policy/ebpf.go(10 hunks)pkg/policy/equality.go(2 hunks)
🔇 Additional comments (6)
pkg/policy/equality.go (3)
13-14: LGTM! Field names are now more descriptive.
The renamed fields better convey their purpose:
equalsInPolicies: Clearly indicates equality matches in policieskeyUsedInPolicies: Better describes tracking of keys used in policies
18-18: LGTM! Comment accurately reflects renamed fields.
The constant value and its documentation are consistent with the struct definition.
48-49: LGTM! Verify usage of renamed fields across the codebase.
The function implementations correctly use the renamed fields while maintaining the original logic.
Let's verify the usage of these renamed fields across the codebase:
Also applies to: 55-56
✅ Verification successful
Field renaming has been consistently applied across the codebase
The verification shows that:
- No instances of old field names (
equalInPolicies,equalitySetInPolicies) were found - New field names (
equalsInPolicies,keyUsedInPolicies) are consistently used in bothpkg/policy/equality.goandpkg/policy/ebpf.go - All usages align with the intended changes, including struct definitions, comments, and binary operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old field names
echo "Checking for old field names..."
rg "equalInPolicies|equalitySetInPolicies"
# Search for usage of new field names to ensure consistent adoption
echo "Checking usage of new field names..."
rg "equalsInPolicies|keyUsedInPolicies"
Length of output: 1382
pkg/filters/string_test.go (1)
Line range hint 154-166: LGTM! Clear and descriptive test function name.
The renaming from TestStringFilterFilterOut to TestStringFilterMatchIfKeyMissing better describes the test's purpose. The first test case correctly verifies that when all expressions use equality operators, MatchIfKeyMissing() returns false.
pkg/policy/ebpf.go (2)
574-601: Verify that the field ordering matches the corresponding C struct
The PoliciesConfig struct mirrors the C struct policies_config_t, and the order of fields is crucial for correct operation. Please ensure that the field ordering in the Go struct matches the C struct to prevent any potential mismatches in data mapping.
161-161: Ensure all function calls are updated to match the new function signature
The function createNewEventsMapVersion now accepts rules map[events.ID]*eventFlags instead of eventsState map[events.ID]events.EventState. Please verify that all calls to this function are updated to match the new signature to prevent any compilation errors.
Run the following script to find all calls to createNewEventsMapVersion and check their arguments:
✅ Verification successful
Based on the rg search results, I can see that there is only one call to createNewEventsMapVersion in the codebase, and it's using the correct signature with rules map[events.ID]*eventFlags as the second argument. The function definition and its only usage are both in the same file (pkg/policy/ebpf.go), and the arguments match perfectly.
No signature mismatch found - function call is already updated
The only call to createNewEventsMapVersion is already using the correct argument type (rules map[events.ID]*eventFlags), matching the new function signature. No updates are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to createNewEventsMapVersion and display their usage
# Search for function calls to createNewEventsMapVersion in the codebase
rg 'createNewEventsMapVersion\(' -A 3
Length of output: 483
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
pkg/filters/uint.go (2)
Line range hint
215-220: Add documentation for the renamed method.The rename from
FilterOuttoMatchIfKeyMissingimproves clarity, but the method would benefit from documentation explaining its purpose and return value semantics.Add documentation like this:
+// MatchIfKeyMissing determines if a filter should match when a key is not present. +// Returns false only when there are equality conditions but no other constraints, +// indicating that only specific values should match. +// Returns true in all other cases, allowing matches for missing keys when there +// are range conditions or inequality constraints. func (f *UIntFilter[T]) MatchIfKeyMissing() bool {
Line range hint
115-134: Consider adding range consistency validation.While the implementation of
LowerEqualandGreaterEqualis correct, consider adding validation to ensure that range boundaries remain consistent (min < max) after updates.Example implementation:
func (f *UIntFilter[T]) add(val uint64, operator Operator) error { if !f.validate(val) { return InvalidValue(fmt.Sprint(val)) } + // Store original values for validation + origMin, origMax := f.min, f.max + switch operator { case Equal: f.addEqual(val) case NotEqual: f.addNotEqual(val) case Lower: f.addLessThan(val) case Greater: f.addGreaterThan(val) case LowerEqual: f.addEqual(val) f.addLessThan(val) case GreaterEqual: f.addEqual(val) f.addGreaterThan(val) } + + // Validate range consistency if both bounds are set + if f.min != MinNotSetUInt && f.max != MaxNotSetUInt && f.min >= f.max { + // Restore original values + f.min, f.max = origMin, origMax + return fmt.Errorf("invalid range: min (%d) must be less than max (%d)", val, f.max) + } + return nil }pkg/filters/string_test.go (1)
Line range hint
154-199: Consider refactoring to a table-driven test for better maintainability.The current test structure has repetitive patterns that could be simplified using a table-driven test approach.
Here's a suggested refactor:
func TestStringFilterMatchIfKeyMissing(t *testing.T) { t.Parallel() + testCases := []struct { + name string + filters []struct { + op string + value string + } + expectMatch bool + }{ + { + name: "all equality filters", + filters: []struct { + op string + value string + }{ + {"=", "some"}, + {"=", "word"}, + {"=", "here"}, + }, + expectMatch: false, + }, + { + name: "mixed equality and inequality", + filters: []struct { + op string + value string + }{ + {"=", "some"}, + {"!=", "word"}, + {"=", "here"}, + }, + expectMatch: true, + }, + // Add remaining test cases... + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + sf := NewStringFilter(nil) + for _, f := range tc.filters { + err := sf.Parse(f.op + f.value) + require.NoError(t, err) + } + assert.Equal(t, tc.expectMatch, sf.MatchIfKeyMissing()) + }) + } - sf1 := NewStringFilter(nil) - // ... existing test code ... }Benefits:
- Each test case is self-documenting with a descriptive name
- Easier to add new test cases
- Pattern of test setup and assertions is consistent
- Reduced code duplication
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
pkg/filters/string_test.go(5 hunks)pkg/filters/uint.go(1 hunks)pkg/policy/ebpf.go(10 hunks)pkg/policy/equality.go(2 hunks)
🔇 Additional comments (19)
pkg/policy/equality.go (5)
13-14: LGTM! Field names are now more descriptive.
The renamed fields better convey their purpose:
equalsInPolicies: Clearly indicates tracking of equality matcheskeyUsedInPolicies: Better describes tracking of keys used in policies
18-18: LGTM! Comment accurately reflects renamed fields.
The size constant remains correct at 16 bytes, matching the struct's memory layout.
48-49: LGTM! Function correctly uses renamed fields.
The notEqualUpdate function maintains its original logic while using the new field names.
55-56: LGTM! Function correctly uses renamed fields.
The equalUpdate function maintains its original logic while using the new field names.
13-14: Verify all references to renamed fields are updated.
Let's ensure no references to the old field names remain in the codebase.
✅ Verification successful
Field renames are consistently applied across the codebase
The search results show that:
- No references to old field names (
equalInPoliciesorequalitySetInPolicies) exist in the codebase - The new field names (
equalsInPoliciesandkeyUsedInPolicies) are used consistently in bothpkg/policy/equality.goandpkg/policy/ebpf.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to old field names
echo "Checking for old field names..."
rg -i "equalInPolicies|equalitySetInPolicies" --type go
# Search for new field names to verify consistent usage
echo "Verifying new field names usage..."
rg -i "equalsInPolicies|keyUsedInPolicies" --type go
Length of output: 1404
pkg/filters/string_test.go (1)
Line range hint 154-199: LGTM! Test cases comprehensively cover MatchIfKeyMissing behavior.
The test cases effectively validate the MatchIfKeyMissing behavior across different filter combinations:
- All equality filters (sf1)
- Mixed equality and inequality filters (sf2, sf3)
- All inequality filters (sf4)
pkg/policy/ebpf.go (13)
11-11: Import statement updated correctly
The import of the bpf package from github.com/khulnasoft/libbpfgo is correctly updated and aligns with the new module path.
161-161: Function parameter updated to use rules
The function createNewEventsMapVersion now accepts rules map[events.ID]*eventFlags as a parameter, reflecting the updated approach to managing event states.
182-182: Iterating over rules for event configuration
The loop correctly iterates over rules to populate the new events map version with the appropriate event configurations.
186-186: Storing policiesSubmit in event configuration
The assignment binary.LittleEndian.PutUint64(eventConfigVal[0:8], ecfg.policiesSubmit) correctly stores the policiesSubmit bitmap into the event configuration value.
221-222: Updating equality values with renamed fields
The equality values are updated using v.equalsInPolicies and v.keyUsedInPolicies, ensuring consistency with the renamed fields in the equality struct.
254-255: Consistent update of string filter equality values
The string filter equality values are correctly set using the new field names v.equalsInPolicies and v.keyUsedInPolicies.
281-282: Process tree filter equality values updated
In the process tree filter BPF update, the equality values are appropriately assigned using v.equalsInPolicies and v.keyUsedInPolicies.
386-387: Binary filter equality values set with new fields
The binary filter equality values are correctly updated with v.equalsInPolicies and v.keyUsedInPolicies, maintaining consistency across filters.
453-453: Function updateBPF parameter updated
The updateBPF function's parameter has been updated to accept rules map[events.ID]*eventFlags, aligning with the changes in event state management.
460-460: Consistent invocation of createNewEventsMapVersion
The call to ps.createNewEventsMapVersion(bpfModule, rules, eventsParams) uses the updated parameters, ensuring consistency with the function's signature.
629-667: Correctly updating filter enabled bitmaps
The code accurately updates the filter enabled bitmaps in PoliciesConfig by setting the appropriate bits based on each policy's enabled filters. This ensures that the BPF program is aware of which filters are active.
670-705: Updating MatchIfKeyMissing bitmaps correctly
The addition of the MatchIfKeyMissing fields and their corresponding bitmap updates correctly reflect the new filter behavior in the policies. This allows the BPF program to properly handle cases where a filter key is missing.
574-601:
Ensure alignment with C struct in BPF code
The PoliciesConfig struct has been updated with new field names and additional fields for MatchIfKeyMissing. Since this struct mirrors the C struct policies_config_t in the BPF code, it's crucial that the order and types of the fields match exactly to prevent any mismatches between user space and kernel space representations.
Recommend verifying that the Go struct PoliciesConfig precisely matches the C struct policies_config_t in field order and types.
User description
1. Explain what the PR does
"Replace me with
make check-proutput"2. Explain how to test it
3. Other comments
PR Type
enhancement
Description
equalitystruct by renaming fieldsequalInPoliciestoequalsInPoliciesandequalitySetInPoliciestokeyUsedInPoliciesfor improved clarity.notEqualUpdateandequalUpdatefunctions to use the new field names, maintaining consistency across the codebase.Changes walkthrough 📝
equality.go
Refactor equality struct and functions for claritypkg/policy/equality.go
Summary by CodeRabbit
New Features
MatchIfKeyMissingmethod for better handling of missing keys.Bug Fixes
Documentation
Refactor