fix(filter:drop): add validation to restrict inclusion of single quote to condition - #3390
Open
jcantrill wants to merge 1 commit into
Open
fix(filter:drop): add validation to restrict inclusion of single quote to condition#3390jcantrill wants to merge 1 commit into
jcantrill wants to merge 1 commit into
Conversation
…e to condition ref: LOG-9704
📝 WalkthroughWalkthroughChangesDrop filter validation
Documentation updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoHarden drop filter regex handling: reject single quotes and fix notMatches validation
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcantrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/datamodels/viaq/v1.adoc`:
- Line 521: Replace the misspelled word “estblish” with “establish” in every
repeated ViaQ sequence description, including the entries identified in the
review, while leaving the rest of each description unchanged.
In `@internal/validations/observability/filters/validate_filters.go`:
- Around line 58-67: Update validation in
internal/validations/observability/filters/validate_filters.go lines 58-67 to
reject newline and carriage-return characters in both testCondition.Matches and
testCondition.NotMatches, alongside single quotes. Also update the pattern
checks in internal/generator/vector/filter/drop/filter.go lines 33-60 to reject
\n and \r before interpolating either pattern into VRL, keeping both runtime
paths aligned with the API contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b2b4ddb0-ffd6-42d2-8fc5-8dea56889b90
📒 Files selected for processing (13)
api/observability/v1/filter_types.gobundle/manifests/observability.openshift.io_clusterlogforwarders.yamlconfig/crd/bases/observability.openshift.io_clusterlogforwarders.yamldocs/reference/datamodels/viaq/v1.adocdocs/reference/operator/api_observability_v1.adocinternal/generator/vector/filter/drop/filter.gointernal/generator/vector/filter/drop/filter_test.gointernal/validations/observability/filters/validate_filters.gointernal/validations/observability/filters/validate_filters_test.gotest/e2e/collection/apivalidations/api_validations_test.gotest/e2e/collection/apivalidations/drop-filter-single-quote-matches.yamltest/e2e/collection/apivalidations/drop-filter-single-quote-notmatches.yamltest/e2e/collection/apivalidations/drop-filter-valid.yaml
| |object a| *(optional)* Labels is a set of common, static labels that were spec'd for log forwarding to be sent with the log Records | ||
| |sequence | ||
| |string a| Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. | ||
| |string a| Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore establish in all ViaQ sequence descriptions.
The change introduces the typo estblish in eight repeated descriptions. Replace it with establish.
Also applies to: 539-539, 919-919, 937-937, 1479-1479, 1497-1497, 2328-2328, 2346-2346
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/datamodels/viaq/v1.adoc` at line 521, Replace the misspelled
word “estblish” with “establish” in every repeated ViaQ sequence description,
including the entries identified in the review, while leaving the rest of each
description unchanged.
Comment on lines
+58
to
+67
| // Reject single quotes — they break VRL r'...' raw string literals | ||
| // and TOML '''...''' literal strings, enabling config injection | ||
| if strings.ContainsRune(testCondition.Matches, '\'') || strings.ContainsRune(testCondition.NotMatches, '\'') { | ||
| testErrors = append(testErrors, "matches/notMatches must not contain single quotes") | ||
| } | ||
| // Validate provided regex | ||
| if testCondition.Matches != "" { | ||
| _, err = regexp.Compile(testCondition.Matches) | ||
| } else if testCondition.NotMatches != "" { | ||
| _, err = regexp.Compile(testCondition.Matches) | ||
| _, err = regexp.Compile(testCondition.NotMatches) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep runtime expression validation consistent with the API contract.
The API contract excludes single quotes, newlines, and carriage returns. Both runtime paths reject only single quotes.
internal/validations/observability/filters/validate_filters.go#L58-L67: reject\nand\rin bothMatchesandNotMatches.internal/generator/vector/filter/drop/filter.go#L33-L60: reject\nand\rbefore interpolating a pattern into VRL.
📍 Affects 2 files
internal/validations/observability/filters/validate_filters.go#L58-L67(this comment)internal/generator/vector/filter/drop/filter.go#L33-L60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/validations/observability/filters/validate_filters.go` around lines
58 - 67, Update validation in
internal/validations/observability/filters/validate_filters.go lines 58-67 to
reject newline and carriage-return characters in both testCondition.Matches and
testCondition.NotMatches, alongside single quotes. Also update the pattern
checks in internal/generator/vector/filter/drop/filter.go lines 33-60 to reject
\n and \r before interpolating either pattern into VRL, keeping both runtime
paths aligned with the API contract.
Code Review by Qodo
1. Swallowed VRL build error
|
![qodo-for-rh-openshift[bot]](https://avatars.githubusercontent.com/u/792337?s=60&v=4){kind=small}
Comment on lines
+34
to
+36
| if strings.ContainsRune(pattern, '\'') { | ||
| return "", fmt.Errorf("match pattern must not contain single quotes: %q", pattern) | ||
| } |
There was a problem hiding this comment.
1. Swallowed vrl build error 🐞 Bug ☼ Reliability
buildMatchCondition now returns an error for patterns containing a single quote, but the drop filter factory logs the error and returns a nil transform, preventing callers from surfacing a clear validation-style failure. Code paths that generate configs without running the controller validation layer (e.g., internal/pkg/generator/forwarder.Generate) can therefore produce confusing downstream generation errors instead of a direct "invalid drop filter" message.
Agent Prompt
### Issue description
`buildMatchCondition()` can now return an error (single quotes), and `(*Filter).VRL()` propagates it. However, `drop.New()` currently swallows VRL errors by logging and returning `nil`, which prevents callers from returning an actionable error and can lead to unclear downstream failures during config generation.
### Issue Context
The controller reconcile path validates specs before generation, but `internal/pkg/generator/forwarder.Generate()` builds configs from YAML without invoking validations, so this new error path is reachable outside the CRD-admission flow.
### Fix Focus Areas
- internal/generator/vector/filter/drop/filter.go[23-31]
- internal/generator/vector/filter/drop/filter.go[33-60]
- internal/pkg/generator/forwarder/generator.go[33-59]
- internal/generator/vector/adapters/pipeline.go[25-31]
### Suggested fix approach
- Preferably: make the non-controller generator path validate the forwarder spec (or at least validate filters) and return a clear error when invalid.
- Additionally/alternatively: avoid returning `nil` transforms from `drop.New()` on VRL errors (either propagate an error upward via a new API, or return a safe no-op transform plus an explicit, user-facing error signal in the generator path).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| |object a| *(optional)* Labels is a set of common, static labels that were spec'd for log forwarding to be sent with the log Records | ||
| |sequence | ||
| |string a| Sequence is increasing id used in conjunction with the timestamp to establish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. | ||
| |string a| Sequence is increasing id used in conjunction with the timestamp to estblish a linear timeline of log records. This was added as a workaround for logstores that do not have nano-second precision. |
There was a problem hiding this comment.
2. Docs spelling regression 🐞 Bug ⚙ Maintainability
The ViaQ data model reference replaces the correct word "establish" with the misspelled "estblish" in multiple places, reducing documentation correctness. This appears to be an accidental regression introduced by this PR.
Agent Prompt
### Issue description
The documentation text for the `openshift.sequence` field contains a spelling regression: `establish` was changed to `estblish` in multiple locations.
### Issue Context
These docs are reference material; typos here tend to get propagated into published artifacts.
### Fix Focus Areas
- docs/reference/datamodels/viaq/v1.adoc[513-542]
- docs/reference/datamodels/viaq/v1.adoc[911-940]
- docs/reference/datamodels/viaq/v1.adoc[1471-1500]
- docs/reference/datamodels/viaq/v1.adoc[2320-2347]
### Suggested fix approach
Replace `estblish` with `establish` in all affected sections (search for `estblish` and correct each occurrence).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
|
/retest |
Contributor
|
@jcantrill: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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.
ref: LOG-9704
Description
Links
cc @Clee2691 @vparfonov
Summary by CodeRabbit
Bug Fixes
notMatchesexpressions.Documentation
Tests