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.

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

Fix ip matcher lexer to differentiate filter from identifier #4598

Merged
merged 4 commits into from
Nov 2, 2021

Conversation

kavirajk
Copy link
Contributor

@kavirajk kavirajk commented Oct 29, 2021

What this PR does / why we need it:
Fix ip matcher lexer to differentiate filter from identifier

Which issue(s) this PR fixes:
Fixes: #4571

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@kavirajk kavirajk marked this pull request as ready for review October 31, 2021 18:24
@kavirajk kavirajk requested a review from a team as a code owner October 31, 2021 18:24
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

Clever solution! This is looking great. Would you also be willing to include other reserved words, like the following?

	"by":           BY,
	"without":      WITHOUT,
	"bool":         BOOL,

@kavirajk
Copy link
Contributor Author

kavirajk commented Nov 2, 2021

thanks @owen-d :) Yea defnitely we can include more tokens there (to be treated both as functions and identifiers). But I would prefer to take that in separate PR, if you don't mind.

Mainly because to that may need more tests to be added which increases the surface area of the change just for the problem we are trying to address with ip filter :)

},
{
in: `{ foo = "bar" }|logfmt|ip="2.3.4.5"|ip="abc"|ipaddr=ip("4.5.6.7")|ip=ip("6.7.8.9")`, // `ip` should work as both label name and filter in same query with same name.
exp: newPipelineExpr(
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also include something like this

{
			in: `{ foo = "bar" , ip="foo"}|logfmt|= ip("127.0.0.1")|ip="2.3.4.5"|ip="abc"|ipaddr=ip("4.5.6.7")|ip=ip("6.7.8.9")`,
			exp: newPipelineExpr(
				newMatcherExpr([]*labels.Matcher{mustNewMatcher(labels.MatchEqual, "foo", "bar"), mustNewMatcher(labels.MatchEqual, "ip", "foo")}),
				MultiStageExpr{
					newLabelParserExpr(OpParserTypeLogfmt, ""),
					newLineFilterExpr(labels.MatchEqual, OpFilterIP, "127.0.0.1"),
					newLabelFilterExpr(log.NewStringLabelFilter(mustNewMatcher(labels.MatchEqual, "ip", "2.3.4.5"))),
					newLabelFilterExpr(log.NewStringLabelFilter(mustNewMatcher(labels.MatchEqual, "ip", "abc"))),
					newLabelFilterExpr(log.NewIPLabelFilter("4.5.6.7", "ipaddr", log.LabelFilterEqual)),
					newLabelFilterExpr(log.NewIPLabelFilter("6.7.8.9", "ip", log.LabelFilterEqual)),
				},
			),
		},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. you mean with line filter expression! good idea :) Adding it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct !

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

I think this would be cleaner.

diff --git a/pkg/logql/lex.go b/pkg/logql/lex.go
index 11eacdb44..4d2d1f22b 100644
--- a/pkg/logql/lex.go
+++ b/pkg/logql/lex.go
@@ -200,7 +200,11 @@ func (l *lexer) Lex(lval *exprSymType) int {
                }
        }
 
-       if tok, ok := functionTokens[tokenText]; ok && isFunction(l.Scanner) {
+       if tok, ok := functionTokens[tokenText]; ok {
+               if !isFunction(l.Scanner) {
+                       lval.str = tokenText
+                       return IDENTIFIER
+               }
                return tok
        }
 
@@ -209,7 +213,7 @@ func (l *lexer) Lex(lval *exprSymType) int {
                return tok
        }
 
-       if tok, ok := tokens[tokenText]; ok && !alsoIdentifier[tokenText] {
+       if tok, ok := tokens[tokenText]; ok {
                return tok
        }

Basically it means if we found a function token, but there's not parenthesis then consider it as a identifier.

WDYT ? This will solve also the problem for other function.

EDIT: tested with rate instead of IP it fails the same way so you really only fixing the problem for the IP function. I suggest you add test for another function like rate="foo"

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit 1e0386e into main Nov 2, 2021
@cyriltovena cyriltovena deleted the fix-ip-filter-lexer branch November 2, 2021 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Any label (extracted or not) which has the name ip leads to a collision with the ip() filter function
3 participants