fix(syslog): normalize syslog severity to lowercase in VRL validation#3326
fix(syslog): normalize syslog severity to lowercase in VRL validation#3326vparfonov wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe syslog output remap logic across the generator source and all syslog output TOML templates now lowercases ChangesSyslog severity normalization
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)Not applicable — the change is a repetitive, homogeneous edit applied uniformly across templates plus straightforward parameterized tests. Suggested labels: syslog, tests, generator Suggested reviewers: No specific reviewer information available from the provided context. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The VRL to_syslog_severity() function is case-sensitive and only accepts lowercase keywords, causing spurious "Invalid syslog severity value" warnings when users configure capitalized severity keywords like "Informational" or "Critical". Add downcase normalization before the to_syslog_severity() call so case-insensitive keywords are correctly validated without warnings. Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcantrill, vparfonov 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
🧹 Nitpick comments (1)
internal/generator/vector/output/syslog/syslog.go (1)
98-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFacility conversion has the same case-sensitivity gap but wasn't fixed.
The
Facilityfield docs state values can be case-insensitive keywords (e.g.user,local0, etc.), same asSeverity. The facility block directly above (Lines 79-96) still callsto_syslog_facility_code(._syslog.facility)without any downcase normalization, so capitalized facility keywords would still trip the same "Invalid syslog facility value" warning this PR is fixing for severity.Consider applying the identical
downcase(to_string!(...))normalization to._syslog.facilityfor consistency.🤖 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/generator/vector/output/syslog/syslog.go` around lines 98 - 101, The syslog generator still has the same case-sensitivity bug for facility values that was fixed for severity. Update the facility handling in the syslog transform block around the existing to_syslog_facility_code(._syslog.facility) call to normalize ._syslog.facility with downcase(to_string(...)) before conversion, mirroring the severity logic in the same output generator so capitalized keywords like local0 and user are accepted consistently.
🤖 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/output/syslog/syslog.go`:
- Around line 98-101: The syslog severity normalization in the
`._syslog.severity` handling block uses a fallible string conversion, which can
break the shared remap. Update the severity assignment in the syslog transform
to use `to_string!()` (or add equivalent explicit error handling) before
`downcase(...)`, since the value may come from `.level` and must not rely on an
infallible conversion.
In `@internal/generator/vector/output/syslog/tcp_with_tuning.toml`:
- Around line 50-53: The syslog severity normalization in the remap block uses a
fallible to_string() call, which can break VRL compilation before downcase()
runs. Update the logic in the severity handling path so the conversion is
handled explicitly, using to_string!(...) or a fallback with ?? before passing
the value to downcase(), and keep the change localized to the ._syslog.severity
normalization in the syslog TCP tuning generator.
---
Nitpick comments:
In `@internal/generator/vector/output/syslog/syslog.go`:
- Around line 98-101: The syslog generator still has the same case-sensitivity
bug for facility values that was fixed for severity. Update the facility
handling in the syslog transform block around the existing
to_syslog_facility_code(._syslog.facility) call to normalize ._syslog.facility
with downcase(to_string(...)) before conversion, mirroring the severity logic in
the same output generator so capitalized keywords like local0 and user are
accepted consistently.
🪄 Autofix (Beta)
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: 2a5375f6-9e3b-4110-9d13-19703ef1b743
📒 Files selected for processing (9)
internal/generator/vector/output/syslog/rfc3164_with_defaults.tomlinternal/generator/vector/output/syslog/syslog.gointernal/generator/vector/output/syslog/tcp_with_defaults.tomlinternal/generator/vector/output/syslog/tcp_with_kubernetes_minimal_enrichment.tomlinternal/generator/vector/output/syslog/tcp_with_tuning.tomlinternal/generator/vector/output/syslog/tls_with_field_references.tomlinternal/generator/vector/output/syslog/udp_with_every_setting.tomltest/functional/outputs/syslog/rfc3164_test.gotest/functional/outputs/syslog/rfc5424_test.go
| if exists(._syslog.severity) { | ||
| # Normalize to lowercase since to_syslog_severity() is case-sensitive | ||
| ._syslog.severity = downcase(to_string(._syslog.severity)) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is VRL to_string() function fallible when applied to a value of type "any"?
💡 Result:
In the Vector Remap Language (VRL), the to_string() function is classified as a fallible function [1]. Whether it effectively behaves as fallible depends on the type of the input argument known to the compiler [2]. When the input type is "any" (or otherwise unknown to the compiler), to_string() is treated as fallible because the language cannot guarantee the operation will succeed for all possible inputs [2]. In VRL, users are required to handle errors for any expression that can result in a runtime error [3][4]. If a function is identified by the compiler as fallible, you must either handle the potential error (e.g., using a variable assignment for the result and an error) or use the "fallible variant" of the function (suffixed with !) to abort execution if an error occurs [4][5]. While to_string() can convert integers, floats, booleans, timestamps, and null values into their string representations [6], its status as "fallible" or "infallible" is a property of the type checking system [2]. When the input is "any," the compiler adopts a safe stance by marking the operation as potentially failing, thereby forcing the developer to account for that possibility [4][2].
Citations:
- 1: https://vector.dev/docs/reference/vrl/functions/
- 2: Expression that can't fail, but fail vectordotdev/vector#19702
- 3: https://vector.dev/docs/reference/vrl/
- 4: https://vector.dev/blog/vector-remap-language/
- 5: Reimplement progressive type checking in Remap vectordotdev/vector#6507
- 6: https://docs.rs/crate/vrl/latest/source/src/stdlib/to_string.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the reported lines.
sed -n '1,220p' internal/generator/vector/output/syslog/syslog.go
echo
echo '---'
echo
# Find other occurrences of to_string in syslog output generation and related VRL helpers.
rg -n "to_string!?\\(|downcase\\(|to_syslog_severity|severity" internal/generator/vector/output internal/generator/vector -g '!**/*_test.go'Repository: openshift/cluster-logging-operator
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the parseEncoding/buildDefaults logic that produces the VRL block.
sed -n '220,420p' internal/generator/vector/output/syslog/syslog.go
echo
echo '---'
echo
# Inspect the generated syslog templates to compare the same line across variants.
sed -n '45,70p' internal/generator/vector/output/syslog/tcp_with_defaults.toml
echo '---'
sed -n '30,55p' internal/generator/vector/output/syslog/udp_with_every_setting.toml
echo '---'
sed -n '30,55p' internal/generator/vector/output/syslog/tls_with_field_references.tomlRepository: openshift/cluster-logging-operator
Length of output: 7197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the functions that emit the syslog VRL fragment.
ast-grep outline internal/generator/vector/output/syslog/syslog.go --view expanded
echo
echo '---'
echo
# Read the parseEncoding and buildDefaults implementations.
sed -n '420,520p' internal/generator/vector/output/syslog/syslog.goRepository: openshift/cluster-logging-operator
Length of output: 2915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# See how .level is used elsewhere in vector output templates.
rg -n '\.level\b' internal/generator/vector/output -g '*.toml' -g '*.go' | sed -n '1,120p'
echo
echo '---'
echo
# Find other unguarded to_string() calls that the generator already relies on.
rg -n 'to_string\([^!]' internal/generator/vector/output -g '*.toml' -g '*.go' | sed -n '1,120p'Repository: openshift/cluster-logging-operator
Length of output: 24001
🌐 Web query:
Vector VRL to_string any fallible compiler unhandled fallible assignment downcase(to_string(.level))
💡 Result:
In the Vector Remap Language (VRL), functions like to_string and downcase are classified as fallible because they cannot guarantee success for all possible input types [1][2]. When you use these functions, the VRL compiler requires you to handle potential errors to ensure runtime safety [3][2]. If you encounter an "unhandled fallible assignment" or a "fallible argument" error, it means the compiler cannot prove the operation will succeed based on the known type of the input [4][5][2]. To resolve this when using a construct like downcase(to_string(.level)), you have two primary options: 1. Use the "bang" (!) operator: This explicitly tells the compiler to treat the function as infallible [5][2]. If the function fails at runtime, the transform will drop the event or trigger an error depending on your configuration [3]. Example:.level = downcase!(to_string!(.level)) 2. Handle the error explicitly: You can assign the result to a variable along with an error variable, then check the error [3][5]. Example: level_str, err = to_string(.level) if err == null {.level = downcase(level_str) } The "bang" operator is the most concise way to proceed if you are confident that the input will be a valid type (e.g., if.level is guaranteed to be a string or integer by your upstream source) [5][2]. If you cannot guarantee the input type, explicit error handling is recommended to prevent runtime failures [2].
Citations:
- 1: https://website-formatting.d1a7j77663uxsc.amplifyapp.com/docs/reference/vrl/functions/
- 2: https://website-formatting.d1a7j77663uxsc.amplifyapp.com/docs/reference/vrl/
- 3: https://vector.dev/docs/reference/vrl/errors/
- 4: Expression that can't fail, but fail vectordotdev/vector#19702
- 5: Getting "fallible argument" with handled error vectordotdev/vector#21767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the rule generation logic around parseEncoding/buildDefaults.
sed -n '420,520p' internal/generator/vector/output/syslog/syslog.go
echo
echo '---'
echo
# Show helper functions that decide when severity gets populated.
sed -n '520,620p' internal/generator/vector/output/syslog/syslog.goRepository: openshift/cluster-logging-operator
Length of output: 178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find any explicit assignments to .level in the syslog generator and its generated templates.
rg -n '\.level\s*=' internal/generator/vector/output/syslog internal/generator/vector/output -g '*.go' -g '*.toml' | sed -n '1,120p'
echo
echo '---'
echo
# Inspect the relevant parts of the syslog generator around buildDefaults/parseEncoding.
sed -n '260,420p' internal/generator/vector/output/syslog/syslog.goRepository: openshift/cluster-logging-operator
Length of output: 6881
Use to_string!() here ._syslog.severity can come from .level, so this fallible conversion needs ! or explicit error handling; otherwise the shared syslog remap can fail to compile.
🤖 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/generator/vector/output/syslog/syslog.go` around lines 98 - 101, The
syslog severity normalization in the `._syslog.severity` handling block uses a
fallible string conversion, which can break the shared remap. Update the
severity assignment in the syslog transform to use `to_string!()` (or add
equivalent explicit error handling) before `downcase(...)`, since the value may
come from `.level` and must not rely on an infallible conversion.
| if exists(._syslog.severity) { | ||
| # Normalize to lowercase since to_syslog_severity() is case-sensitive | ||
| ._syslog.severity = downcase(to_string(._syslog.severity)) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target file ==\n'
cat -n internal/generator/vector/output/syslog/tcp_with_tuning.toml | sed -n '1,120p'
printf '\n== related variant ==\n'
cat -n internal/generator/vector/output/syslog/tcp_with_kubernetes_minimal_enrichment.toml | sed -n '1,120p'Repository: openshift/cluster-logging-operator
Length of output: 8611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search for severity handling in syslog outputs ==\n'
rg -n "to_string\\(|downcase\\(|_syslog\\.severity|level" internal/generator/vector/output/syslog -g '*.toml'Repository: openshift/cluster-logging-operator
Length of output: 11566
🌐 Web query:
Vector VRL to_string function fallible infallible assignment E103 downcase(to_string(...)) compile error
💡 Result:
In the Vector Remap Language (VRL), the to_string function is classified as fallible [1] because it can fail if the input value cannot be coerced into a string (e.g., if it is an array or object) [2]. The VRL compiler ensures that all potential runtime errors are handled; if you invoke a fallible function without explicit error handling, the compiler will raise an E103 error ("unhandled fallible assignment") [3][4][5]. The E103 compile error occurs when a fallible function is used in an expression without handling the potential error [3][4]. To resolve this, you must explicitly handle the fallibility using one of the following methods: 1. Use the abort operator (!) to force the program to abort if the function fails (e.g., to_string!(.field)) [6]. 2. Use the error coalescing operator (??) to provide a fallback value if the function fails (e.g., to_string(.field) ?? "default") [3]. 3. Assign the result to an error-aware variable structure (e.g., .ok, .err = to_string(.field)) [3]. When you see an error related to downcase(to_string(...)), it is likely because to_string(...) is being treated as fallible, and the result is not being handled as required by the VRL compiler [6][3]. Even if you believe the input will always be a valid string, the VRL type system may not have enough information to guarantee infallibility, necessitating explicit error handling [6][7]. Using the abort operator to_string!(...) is a common way to satisfy the compiler when you expect the input type to be valid [6][8].
Citations:
- 1: https://vector.dev/docs/reference/vrl/functions/
- 2: https://docs.rs/crate/vrl/latest/source/src/stdlib/to_string.rs
- 3: https://vector.dev/docs/reference/vrl/errors/
- 4: https://vector.dev/docs/reference/vrl/
- 5: Remap: contradicting error messages around merge function vectordotdev/vector#6792
- 6: https://vector.dev/docs/reference/vrl/expressions/
- 7: Expression that can't fail, but fail vectordotdev/vector#19702
- 8: Reimplement progressive type checking in Remap vectordotdev/vector#6507
Unhandled to_string() in syslog severity normalization
downcase(to_string(._syslog.severity)) leaves the fallible to_string() unhandled, so this remap block will fail VRL compilation with E103. Use to_string!(...) or ?? before downcase().
🤖 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/generator/vector/output/syslog/tcp_with_tuning.toml` around lines 50
- 53, The syslog severity normalization in the remap block uses a fallible
to_string() call, which can break VRL compilation before downcase() runs. Update
the logic in the severity handling path so the conversion is handled explicitly,
using to_string!(...) or a fallback with ?? before passing the value to
downcase(), and keep the change localized to the ._syslog.severity normalization
in the syslog TCP tuning generator.
|
@vparfonov: 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. |
2 participants
Description
The VRL
to_syslog_severity()function is case-sensitive and only accepts lowercase keywords, causing spurious "Invalid syslog severity value" warnings when users configure capitalized severity keywords like "Informational" or "Critical".Add downcase normalization before the
to_syslog_severity()call so case-insensitive keywords are correctly validated without warnings./cc @Clee2691
/assign @jcantrill
Links
Summary by CodeRabbit
Bug Fixes
Tests