fix(filter:drop): add validation to restrict inclusion of single quote to condition

[jcantrill]

ref: LOG-9704

Description

• Restrict drop filter criteria to exclude single quotes
• fix broken "NotMatches" in drop filter

Links

• https://redhat.atlassian.net/browse/LOG-9704

cc @Clee2691 @vparfonov

Summary by CodeRabbit

• Bug Fixes

  • Drop-filter expressions now reject unsupported single quotes, newlines, and carriage returns.
  • Invalid filter configurations are reported clearly before log forwarding is generated.
  • Corrected validation for negative-match expressions.
  • Pipeline generation now stops with a clear error when a filter cannot be converted.
  • Configuration-generation errors are propagated instead of producing incomplete configurations.
  • Improved pruning of Kubernetes audit log fields.

• Documentation

  • Documented terminationGracePeriodSeconds for collector pods, including its 10-second default.
  • Clarified sequence descriptions in the Viaq v1 data model documentation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: bab06957-93b5-4a80-b1f0-11e96e880e81

📥 Commits

Reviewing files that changed from the base of the PR and between 7d74b0b and 32c75df.

📒 Files selected for processing (1)

• test/functional/filters/prune/prune_filter_test.go

---

📝 Walkthrough

Walkthrough

The change adds schema and runtime validation for drop-filter expressions, propagates filter and transform errors during configuration generation, adds unit and end-to-end coverage, documents collector termination settings, and corrects sequence descriptions.

Changes

Drop-filter validation and generation

Layer / File(s)	Summary
Expression validation contracts api/observability/v1/filter_types.go, config/crd/bases/..., bundle/manifests/...	The matches and notMatches fields reject single quotes, newlines, and carriage returns.
Runtime filter validation internal/validations/observability/filters/validate_filters.go, internal/pkg/generator/forwarder/generator.go	Runtime validation checks both expression fields, compiles the correct notMatches value, and aggregates invalid-filter errors.
Vector error propagation internal/generator/vector/filter/drop/filter.go, internal/generator/vector/adapters/pipeline.go, internal/generator/vector/conf/conf.go, internal/generator/forwarder/generator.go	Vector filter, pipeline, and configuration generation now return validation and transform errors.
Validation and generation coverage internal/validations/observability/filters/*_test.go, internal/generator/vector/filter/drop/filter_test.go, internal/generator/vector/adapters/pipeline_test.go, internal/generator/vector/conf/conf_test.go, test/e2e/collection/apivalidations/*	Tests cover valid filters, invalid expressions, single quotes, nil transforms, and configuration errors.

Documentation updates

Layer / File(s)	Summary
Collector and sequence documentation docs/reference/operator/api_observability_v1.adoc, docs/reference/datamodels/viaq/v1.adoc, test/helpers/types/types.go	The API reference documents terminationGracePeriodSeconds. ViaQ sequence descriptions and a related comment use corrected wording.

Test adjustment

Layer / File(s)	Summary
Prune-filter test test/functional/filters/prune/prune_filter_test.go	The Kubernetes audit-log prune test adds .message to the NotIn field list.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: clee2691, vparfonov

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description states the main changes and links LOG-9704, but it omits the mandatory reviewer and approver assignment commands.	Add a valid /cc reviewer command and a valid /assign approver command from the top-level OWNERS file.
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: adding validation for single quotes in drop-filter conditions.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-for-rh-openshift]

PR Summary by Qodo

Harden drop filter regex handling: reject single quotes and fix notMatches validation

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 📝 Documentation 🕐 20-40 Minutes

AI Description

• Add schema + runtime validation rejecting single quotes in drop filter regex patterns.
• Fix drop filter validation to compile notMatches regex correctly.
• Add unit + e2e coverage for valid patterns and rejection cases.

Diagram

graph TD
  A["ClusterLogForwarder spec"] --> B["CRD schema (pattern)"] --> C["API server validation"] --> D["Drop filter validation"] --> E["VRL generator"] --> F["Vector filter config"]
  T["Unit + e2e tests"] --> D

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Escape quotes instead of rejecting them

• ➕ Keeps regex expressiveness (allows literal single quotes)
• ➕ Avoids breaking existing configs that include single quotes
• ➖ Requires careful escaping across both VRL raw strings and TOML literal strings
• ➖ Higher risk of missing an injection edge case; more complex implementation and tests

2. Switch VRL string literal strategy (avoid r'...')

• ➕ Could allow a broader set of regex patterns by using a different quoting/escaping model
• ➖ Still needs robust escaping and may reduce readability of generated VRL
• ➖ Potential behavioral differences in how regex literals are parsed/represented

Recommendation: Keep the PR’s approach (reject single quotes) because it is the simplest, safest way to prevent VRL/TOML injection and aligns schema + runtime behavior. If future requirements demand supporting quotes, prefer a deliberate end-to-end escaping strategy with dedicated fuzz/property tests.

Files changed (13) +256 / -13

Bug fix (3) +31 / -5

filter_types.goAdd kubebuilder regex pattern to drop match fields +2/-0

Add kubebuilder regex pattern to drop match fields

• Adds kubebuilder validation patterns on DropCondition.Matches and DropCondition.NotMatches to disallow single quotes and newline characters at the API/schema level.

api/observability/v1/filter_types.go

filter.goReject single-quote patterns and unify match/negated-match VRL building +20/-2

Reject single-quote patterns and unify match/negated-match VRL building

• Introduces a helper to build match/!match VRL expressions while rejecting patterns containing single quotes (to avoid breaking VRL r'...' literals and TOML literal strings). Updates VRL generation to use this helper and return errors on invalid patterns.

internal/generator/vector/filter/drop/filter.go

validate_filters.goFix notMatches regex compilation and reject single quotes during validation +9/-3

Fix notMatches regex compilation and reject single quotes during validation

• Adds validation to forbid single quotes in matches/notMatches to prevent VRL/TOML injection. Fixes a bug where the notMatches branch incorrectly compiled Matches instead of NotMatches.

internal/validations/observability/filters/validate_filters.go

Tests (6) +208 / -0

filter_test.goAdd unit tests for single-quote rejection in drop filter VRL generation +32/-0

Add unit tests for single-quote rejection in drop filter VRL generation

• Adds tests asserting that VRL generation fails when Matches or NotMatches contains a single quote and that the error message is informative.

internal/generator/vector/filter/drop/filter_test.go

validate_filters_test.goExpand drop filter validation coverage for notMatches and single quotes +39/-0

Expand drop filter validation coverage for notMatches and single quotes

• Adds test cases for invalid notMatches regex compilation and for rejecting single quotes in both matches and notMatches.

internal/validations/observability/filters/validate_filters_test.go

api_validations_test.goAdd e2e API validation coverage for drop filter patterns +11/-0

Add e2e API validation coverage for drop filter patterns

• Extends the API validation suite with one passing case and two failing cases verifying the CRD rejects single quotes in matches/notMatches.

test/e2e/collection/apivalidations/api_validations_test.go

drop-filter-single-quote-matches.yamlAdd invalid CLF fixture: single quote in matches +42/-0

Add invalid CLF fixture: single quote in matches

• Introduces an e2e fixture ClusterLogForwarder resource that sets matches to a value containing a single quote to assert schema validation failure.

test/e2e/collection/apivalidations/drop-filter-single-quote-matches.yaml

drop-filter-single-quote-notmatches.yamlAdd invalid CLF fixture: single quote in notMatches +42/-0

Add invalid CLF fixture: single quote in notMatches

• Introduces an e2e fixture ClusterLogForwarder resource that sets notMatches to a value containing single quotes to assert schema validation failure.

test/e2e/collection/apivalidations/drop-filter-single-quote-notmatches.yaml

drop-filter-valid.yamlAdd valid CLF fixture for drop filter matches +42/-0

Add valid CLF fixture for drop filter matches

• Introduces an e2e fixture ClusterLogForwarder resource with a valid drop filter matches value to assert successful creation.

test/e2e/collection/apivalidations/drop-filter-valid.yaml

Documentation (2) +13 / -8

v1.adocUpdate viaq datamodel docs text (typo introduced) +8/-8

Update viaq datamodel docs text (typo introduced)

• Edits multiple occurrences of the sequence field description, changing “establish” to “estblish”. This appears to be an accidental spelling regression.

docs/reference/datamodels/viaq/v1.adoc

api_observability_v1.adocDocument collector terminationGracePeriodSeconds field +5/-0

Document collector terminationGracePeriodSeconds field

• Adds documentation entries for spec.collector.terminationGracePeriodSeconds, likely from regenerated API docs.

docs/reference/operator/api_observability_v1.adoc

Other (2) +4 / -0

observability.openshift.io_clusterlogforwarders.yamlPropagate drop filter pattern constraints into the shipped CRD bundle +2/-0

Propagate drop filter pattern constraints into the shipped CRD bundle

• Updates the bundled ClusterLogForwarder CRD schema to include pattern validation for matches and notMatches, rejecting single quotes/newlines.

bundle/manifests/observability.openshift.io_clusterlogforwarders.yaml

observability.openshift.io_clusterlogforwarders.yamlUpdate generated CRD base schema for drop filter patterns +2/-0

Update generated CRD base schema for drop filter patterns

• Mirrors the same OpenAPI pattern additions for matches and notMatches in the CRD base used for generation.

config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml

[openshift-ci]

[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

Details

Needs approval from an approver in each of these files:

• OWNERS [jcantrill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 03eee53 and 33b88bf.

📒 Files selected for processing (13)

• api/observability/v1/filter_types.go
• bundle/manifests/observability.openshift.io_clusterlogforwarders.yaml
• config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml
• docs/reference/datamodels/viaq/v1.adoc
• docs/reference/operator/api_observability_v1.adoc
• internal/generator/vector/filter/drop/filter.go
• internal/generator/vector/filter/drop/filter_test.go
• internal/validations/observability/filters/validate_filters.go
• internal/validations/observability/filters/validate_filters_test.go
• test/e2e/collection/apivalidations/api_validations_test.go
• test/e2e/collection/apivalidations/drop-filter-single-quote-matches.yaml
• test/e2e/collection/apivalidations/drop-filter-single-quote-notmatches.yaml
• test/e2e/collection/apivalidations/drop-filter-valid.yaml

[qodo-for-rh-openshift]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Swallowed VRL build error ✓ Resolved 🐞 Bug ☼ Reliability

Description

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.

Code

internal/generator/vector/filter/drop/filter.go[R34-36]

+	if strings.ContainsRune(pattern, '\'') {
+		return "", fmt.Errorf("match pattern must not contain single quotes: %q", pattern)
+	}

Relevance

●● Moderate

Error-propagation vs logging/nil is behavioral; no close precedent found, team sometimes rejects
generator semantic tweaks.

PR-#3374
PR-#3265

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR introduces a new error-returning path in drop filter VRL generation, while the drop filter
factory still returns nil on VRL errors; pipeline transform collection does not guard against nil,
and the standalone YAML-based generator does not run validations before generating TOML.

internal/generator/vector/filter/drop/filter.go[23-31]
internal/generator/vector/filter/drop/filter.go[33-60]
internal/generator/vector/adapters/pipeline.go[25-31]
internal/pkg/generator/forwarder/generator.go[33-59]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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

---

2. Docs spelling regression ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

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.

Code

docs/reference/datamodels/viaq/v1.adoc[521]

+|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.

Relevance

●●● Strong

Docs/content fixes are routinely accepted; this is a clear misspelling regression to correct.

PR-#3251

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The modified documentation lines show the misspelling in the openshift.sequence description; the
same change pattern repeats in later sections of the same file.

docs/reference/datamodels/viaq/v1.adoc[513-542]
docs/reference/datamodels/viaq/v1.adoc[911-940]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### 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

---

Context used

⚠️ Tickets: not configured — ticket URL found in PR but could not be fetched — check ticket provider credentials

✅ Compliance rules (platform): 9 rules

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[jcantrill]

/retest

[coderabbitai]

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 `@internal/generator/vector/adapters/pipeline.go`:
- Around line 29-33: Update Pipeline.Transforms and the NewPipeline generation
flow so a nil result from pf.Transform() returns an error and aborts generation
instead of logging and omitting the transform. Ensure no later filters or
outputs are wired after this failure, and add regression coverage using a
factory that returns nil.

In `@test/helpers/types/types.go`:
- Line 168: Update the comment describing Sequence to use the grammatically
correct wording “Sequence is an increasing ID,” preserving the existing
explanation about its use with the timestamp.

🪄 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: 341ae269-437e-437f-91f7-5e19b79736de

📥 Commits

Reviewing files that changed from the base of the PR and between 33b88bf and 8ae316e.

📒 Files selected for processing (6)

• internal/generator/vector/adapters/pipeline.go
• internal/generator/vector/filter/drop/filter.go
• internal/pkg/generator/forwarder/generator.go
• internal/validations/observability/filters/validate_filters.go
• internal/validations/observability/filters/validate_filters_test.go
• test/helpers/types/types.go

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/validations/observability/filters/validate_filters_test.go
• internal/validations/observability/filters/validate_filters.go
• internal/generator/vector/filter/drop/filter.go

[openshift-ci]

@jcantrill: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-target	33b88bf	link	true	/test e2e-target
ci/prow/e2e-using-bundle	dc2751e	link	true	/test e2e-using-bundle

Full PR test history. Your PR dashboard.

Details

Instructions 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.