Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pkg/ottl] Fix bug where function names were allowed in comparison #33051

Merged
merged 2 commits into from
May 17, 2024
Merged

[pkg/ottl] Fix bug where function names were allowed in comparison #33051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00b8cdd
Don't allow function names in comparison
TylerHelmuth May 14, 2024
bd62459
Changelog
TylerHelmuth May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .chloggen/ottl-no-funcnames-in-conditions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: pkg/ottl

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Fixes a bug where function name could be used in a condition, resulting in a cryptic error message.

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [33051]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: []
6 changes: 6 additions & 0 deletions pkg/ottl/e2e/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,12 @@ func Test_e2e_editors(t *testing.T) {
tCtx.GetLogRecord().Attributes().PutStr("http.path", "@health")
},
},
{
statement: `replace_pattern(attributes["http.path"], "/", "@", SHA256)`,
want: func(tCtx ottllog.TransformContext) {
tCtx.GetLogRecord().Attributes().PutStr("http.path", "c3641f8544d7c02f3580b07c0f9887f0c6a27ff5ab1d4a3e29caf197cfc299aehealth")
},
},
{
statement: `set(attributes["test"], "pass")`,
want: func(tCtx ottllog.TransformContext) {
Expand Down
4 changes: 2 additions & 2 deletions pkg/ottl/functions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,8 +283,8 @@ func (p *Parser[K]) buildArgs(ed editor, argsVal reflect.Value) error {
switch {
case arg.Value.Enum != nil:
name = string(*arg.Value.Enum)
case arg.Value.FunctionName != nil:
name = *arg.Value.FunctionName
case arg.FunctionName != nil:
name = *arg.FunctionName
default:
return fmt.Errorf("invalid function name given")
}
Expand Down
12 changes: 3 additions & 9 deletions pkg/ottl/functions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,9 +380,7 @@ func Test_NewFunctionCall_invalid(t *testing.T) {
Function: "testing_functiongetter",
Arguments: []argument{
{
Value: value{
FunctionName: (ottltest.Strp("SHA256")),
},
FunctionName: ottltest.Strp("SHA256"),
},
},
},
Expand Down Expand Up @@ -1117,9 +1115,7 @@ func Test_NewFunctionCall(t *testing.T) {
Function: "testing_functiongetter",
Arguments: []argument{
{
Value: value{
FunctionName: (ottltest.Strp("SHA256")),
},
FunctionName: ottltest.Strp("SHA256"),
},
},
},
Expand All @@ -1131,9 +1127,7 @@ func Test_NewFunctionCall(t *testing.T) {
Function: "testing_functiongetter",
Arguments: []argument{
{
Value: value{
FunctionName: (ottltest.Strp("Sha256")),
},
FunctionName: ottltest.Strp("Sha256"),
},
},
},
Expand Down
6 changes: 3 additions & 3 deletions pkg/ottl/grammar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,9 @@ type converter struct {
}

type argument struct {
Name string `parser:"(@(Lowercase(Uppercase | Lowercase)*) Equal)?"`
Value value `parser:"@@"`
Name string `parser:"(@(Lowercase(Uppercase | Lowercase)*) Equal)?"`
Value value `parser:"( @@"`
FunctionName *string `parser:"| @(Uppercase(Uppercase | Lowercase)*) )"`
}

func (a *argument) checkForCustomError() error {
Expand All @@ -236,7 +237,6 @@ type value struct {
String *string `parser:"| @String"`
Bool *boolean `parser:"| @Boolean"`
Enum *enumSymbol `parser:"| @Uppercase (?! Lowercase)"`
FunctionName *string `parser:"| @(Uppercase(Uppercase | Lowercase)*)"`
List *list `parser:"| @@)"`
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/ottl/parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,9 +253,7 @@ func Test_parse(t *testing.T) {
},
},
{
Value: value{
FunctionName: (ottltest.Strp("Sha256")),
},
FunctionName: ottltest.Strp("Sha256"),
},
},
},
Expand Down Expand Up @@ -1952,6 +1950,7 @@ func Test_parseCondition(t *testing.T) {
{`One() == 1`, false},
{`test(fail())`, true},
{`Test()`, false},
{`"test" == Foo`, true},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the error look like now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like

statement has invalid syntax: 1:50: unexpected token "<EOF>" (expected "(" (Argument ("," Argument)*)? ")" Key*)

Which isn't the best, but at least it gives you hints where the issue is. I played around with keeping FunctionName in value and then doing a custom error when it is set, but that caused parsing issues bc value is checked before FunctionName in arguments. It might be possible with some fancy negative lookaheads

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was playing with this today, mainly to understand it better and did some more or less crazy/dumb ideas but didn't managed to get any better error message than this one

Copy link
Contributor

@evan-bradley evan-bradley May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think overall we should try to avoid printing any grammar parsing errors since I think to most users they're incredibly cryptic. That said, this is still better than an error with "This is a bug in the OpenTelemetry Transformation Language" in it. I'd say let's get this in for now and circle back later.

}
pat := regexp.MustCompile("[^a-zA-Z0-9]+")
for _, tt := range tests {
Expand Down