ROX-32239: Process indicator filtering should be configurable #18345
+864
−13
Conversation
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
|
Skipping CI for Draft Pull Request. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `pkg/env/integerarraysetting.go:44-46` </location>
<code_context>
+}
+
+// IntegerArraySetting returns the integer slice represented by the environment variable
+func (s *IntegerArraySetting) IntegerArraySetting() []int {
+ val := os.Getenv(s.envVar)
+ if val == "" {
+ if s.minLength == 0 {
+ // Empty string returns empty array (allows for 0-length arrays)
</code_context>
<issue_to_address>
**issue (bug_risk):** Empty-string handling makes it impossible to ever use the default value when the env var is unset (and breaks the documented default behavior for fan-out levels).
The `val == ""` branch uses `minLength == 0` as a proxy for "empty string is allowed" and returns an empty slice instead of `defaultValue`. That causes two issues:
1) For settings where `minLength` is 0 (including `ProcessFilterFanOutLevels`), an *unset* env var (e.g. `ROX_PROCESS_FILTER_FAN_OUT_LEVELS` not present) yields an empty slice instead of the configured default (`[]int{8, 6, 4, 2}`), diverging from the usual "no env var -> use default" behavior.
2) Since `os.Getenv` returns `""` for both unset and explicitly-empty values, the code cannot distinguish "use default" from "use empty slice", so the default is effectively unreachable whenever empty slices are allowed.
In practice this makes `ProcessFilterFanOutLevels` default to no arg tracking (empty slice) rather than the documented default. To support both a default and an explicit "empty" value, you’ll likely need a different encoding (e.g. a special value like `"none"` or a separate flag), or treat `""` as `defaultValue` and drop the special empty-slice behavior.
</issue_to_address>
### Comment 2
<location> `pkg/env/integerarraysetting_test.go:81-89` </location>
<code_context>
+ expectedPanic: false,
+ expectedValue: []int{5, 5, 5},
+ },
+ "Return default for partially invalid format": {
+ value: "1,2,",
+ defaultValue: []int{5, 5},
+ minValue: 0,
+ maxValue: 100,
+ minLength: 0,
+ maxLength: 100,
+ expectedPanic: false,
+ expectedValue: []int{1, 2},
+ },
+ "Validation with minimum value": {
</code_context>
<issue_to_address>
**nitpick (testing):** Test name suggests default is returned, but the expectations verify partial parsing instead
This case currently asserts that the parsed prefix `[1, 2]` is used despite the trailing comma, but the name/comment suggest the default should be returned for partially invalid input. Please either rename the test (e.g., "Ignore trailing comma" / "Partial parse with trailing comma") or change the expectation to use the default, depending on the intended behavior.
</issue_to_address>
### Comment 3
<location> `pkg/env/integerarraysetting_test.go:61-69` </location>
<code_context>
+ expectedPanic: false,
+ expectedValue: []int{1, 2, 3},
+ },
+ "Empty array for empty string": {
+ value: "",
+ defaultValue: []int{5, 5, 5},
+ minValue: 0,
+ maxValue: 100,
+ minLength: 0,
+ maxLength: 100,
+ expectedPanic: false,
+ expectedValue: []int{},
+ },
+ "Return default for invalid format": {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for empty/whitespace values when `minLength > 0` and unset env var cases
To more completely cover `IntegerArraySetting.IntegerArraySetting`, please add tests for:
1. An empty string when `minLength > 0` (e.g. `value: ""`, `minLength: 1`) to verify we fall back to `defaultValue` instead of returning an empty slice.
2. A whitespace-only string (e.g. `value: " "`) to confirm it’s handled consistently with the empty string for both `minLength == 0` and `minLength > 0`.
These will guard against regressions around how "empty" values interact with length constraints.
Suggested implementation:
```golang
"Whitespace around values": {
value: " 1 , 2 , 3 ",
defaultValue: []int{5, 5, 5},
minValue: 0,
maxValue: 100,
minLength: 0,
maxLength: 100,
expectedPanic: false,
expectedValue: []int{1, 2, 3},
},
"Empty array for empty string": {
value: "",
defaultValue: []int{5, 5, 5},
minValue: 0,
maxValue: 100,
minLength: 0,
maxLength: 100,
expectedPanic: false,
expectedValue: []int{},
},
"Default for empty string when minLength > 0": {
value: "",
defaultValue: []int{5, 5, 5},
minValue: 0,
maxValue: 100,
minLength: 1,
maxLength: 100,
expectedPanic: false,
expectedValue: []int{5, 5, 5},
},
"Whitespace-only string with minLength == 0": {
value: " ",
defaultValue: []int{5, 5, 5},
minValue: 0,
maxValue: 100,
minLength: 0,
maxLength: 100,
expectedPanic: false,
expectedValue: []int{},
},
"Whitespace-only string with minLength > 0": {
value: " ",
defaultValue: []int{5, 5, 5},
minValue: 0,
maxValue: 100,
minLength: 1,
maxLength: 100,
expectedPanic: false,
expectedValue: []int{5, 5, 5},
},
"Return default for invalid format": {
value: "1,abc,3",
defaultValue: []int{5, 5, 5},
minValue: 0,
maxValue: 100,
minLength: 0,
maxLength: 100,
expectedPanic: false,
expectedValue: []int{5, 5, 5},
},
"Return default for partially invalid format": {
```
To also cover the "unset env var" case as mentioned in your review comment, the test table and test harness likely need a way to distinguish between:
- "env var is unset", and
- "env var is set to the empty string".
A typical approach (if not already present) is:
1. Extend the test case struct with a boolean like `setEnv bool` (defaulting to `true`).
2. In the test loop, only call `os.Setenv` when `setEnv` is `true`; otherwise, call `os.Unsetenv`.
3. Add a test case, e.g. `"Unset env var uses default"`, with `setEnv: false`, `defaultValue: []int{5,5,5}`, and `expectedValue: []int{5,5,5}`, and choose appropriate min/max bounds.
You will need to integrate this pattern into the existing test setup where the environment variable is configured for each case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+44
to
+46
| func (s *IntegerArraySetting) IntegerArraySetting() []int { | ||
| val := os.Getenv(s.envVar) | ||
| if val == "" { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Empty-string handling makes it impossible to ever use the default value when the env var is unset (and breaks the documented default behavior for fan-out levels).
The val == "" branch uses minLength == 0 as a proxy for "empty string is allowed" and returns an empty slice instead of defaultValue. That causes two issues:
- For settings where
minLengthis 0 (includingProcessFilterFanOutLevels), an unset env var (e.g.ROX_PROCESS_FILTER_FAN_OUT_LEVELSnot present) yields an empty slice instead of the configured default ([]int{8, 6, 4, 2}), diverging from the usual "no env var -> use default" behavior. - Since
os.Getenvreturns""for both unset and explicitly-empty values, the code cannot distinguish "use default" from "use empty slice", so the default is effectively unreachable whenever empty slices are allowed.
In practice this makes ProcessFilterFanOutLevels default to no arg tracking (empty slice) rather than the documented default. To support both a default and an explicit "empty" value, you’ll likely need a different encoding (e.g. a special value like "none" or a separate flag), or treat "" as defaultValue and drop the special empty-slice behavior.
Comment on lines
+81
to
+89
| "Return default for partially invalid format": { | ||
| value: "1,2,", | ||
| defaultValue: []int{5, 5}, | ||
| minValue: 0, | ||
| maxValue: 100, | ||
| minLength: 0, | ||
| maxLength: 100, | ||
| expectedPanic: false, | ||
| expectedValue: []int{1, 2}, |
Contributor
There was a problem hiding this comment.
nitpick (testing): Test name suggests default is returned, but the expectations verify partial parsing instead
This case currently asserts that the parsed prefix [1, 2] is used despite the trailing comma, but the name/comment suggest the default should be returned for partially invalid input. Please either rename the test (e.g., "Ignore trailing comma" / "Partial parse with trailing comma") or change the expectation to use the default, depending on the intended behavior.
Contributor
|
Images are ready for the commit at bdd10f0. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18345 +/- ##
==========================================
- Coverage 49.39% 48.89% -0.50%
==========================================
Files 2720 2625 -95
Lines 200952 198224 -2728
==========================================
- Hits 99261 96928 -2333
+ Misses 94024 93903 -121
+ Partials 7667 7393 -274
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
There is process indicator filtering, but the parameters used for it are currently hard coded. Users might want to make the filtering more or less aggressive. Some users have so many processes and network flows that they decide to disable collection. This is not desirable. Such users might instead want to reduce collection instead of disabling it all together. With the changes here a user could set
ROX_PROCESS_FILTER_FAN_OUT_LEVELSto an empty string and only one instance of an executable would be saved for each unique container.User-facing documentation
Testing and quality
Automated testing
How I validated my change
CI.