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

Enhance filter parser #41

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Enhance filter parser #41

wants to merge 24 commits into from

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Apr 27, 2023

After all the testing we did last week, it turns out that the current implementation of the filter Parser is very buggy and sometimes crashes with very simple filter strings. This PR solves this by using goyacc generated Parser. This is similar to how we parse Icinga2 DSL with flex/bison. The grammar rules can be found in the parser.y file and you can find the Lexer and other parsing utils in the lexer.go file.

For local testing/debugging purposes you can re-build the parser with the following command.

go env -w GOBIN=/Users/yhabteab/go/bin
go install golang.org/x/tools/cmd/goyacc@latest
go generate './...'

This will also generate a parser.output file which contains all the parser states and is useful to resolve some shift/reduce conflicts.

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Apr 27, 2023
@yhabteab yhabteab force-pushed the enhanced-filter-parser branch 3 times, most recently from 941daff to 080e335 Compare May 2, 2023 07:50
@yhabteab yhabteab changed the title [Draft]: Enhance filter parser Enhance filter parser May 2, 2023
@yhabteab yhabteab requested a review from julianbrost May 2, 2023 08:12
@julianbrost
Copy link
Collaborator

However, I haven't pushed the auto-generated parser yet because it involves a lot of code changes that don't need to be reviewed, but when this PR is done, we can think about merging this parser here or auto-generating it every time when building noma, basically just like the Icinga2 *.ti files.

Have a look at go generate. Also, common practice in Go is to commit these files to the repo so that you don't have to bother with the these dependencies unless you actually want to change the parser.

@yhabteab yhabteab force-pushed the enhanced-filter-parser branch 2 times, most recently from 2337de5 to 176b442 Compare May 3, 2023 10:00
@yhabteab
Copy link
Member Author

yhabteab commented May 3, 2023

fuzz: elapsed: 3m30s, execs: 1483038 (7807/sec), new interesting: 94 (total: 1558)
fuzz: elapsed: 3m33s, execs: 1507525 (8160/sec), new interesting: 94 (total: 1558)
fuzz: elapsed: 3m36s, execs: 1530168 (7550/sec), new interesting: 94 (total: 1558)
fuzz: elapsed: 3m39s, execs: 1553639 (7821/sec), new interesting: 94 (total: 1558)
fuzz: elapsed: 3m42s, execs: 1576623 (7661/sec), new interesting: 94 (total: 1558)
fuzz: elapsed: 3m45s, execs: 1595193 (6190/sec), new interesting: 95 (total: 1559)
fuzz: elapsed: 3m48s, execs: 1609919 (4910/sec), new interesting: 97 (total: 1561)
fuzz: elapsed: 3m51s, execs: 1630670 (6917/sec), new interesting: 99 (total: 1563)
fuzz: elapsed: 3m54s, execs: 1648763 (6029/sec), new interesting: 99 (total: 1563)
fuzz: elapsed: 3m57s, execs: 1667104 (6113/sec), new interesting: 99 (total: 1563)
fuzz: elapsed: 4m0s, execs: 1685923 (6255/sec), new interesting: 99 (total: 1563)
fuzz: elapsed: 4m3s, execs: 1704380 (6154/sec), new interesting: 99 (total: 1563)
fuzz: elapsed: 4m6s, execs: 1723764 (6479/sec), new interesting: 99 (total: 1563)
fuzz: elapsed: 4m9s, execs: 1744018 (6752/sec), new interesting: 99 (total: 1563)
fuzz: elapsed: 4m12s, execs: 1758080 (5418/sec), new interesting: 100 (total: 1564)
--- PASS: FuzzParser (251.66s)
PASS
ok  	github.com/icinga/noma/internal/filter	251.870s

@yhabteab yhabteab force-pushed the enhanced-filter-parser branch 5 times, most recently from 089cfc5 to 877a020 Compare May 3, 2023 16:32
@yhabteab yhabteab force-pushed the enhanced-filter-parser branch 7 times, most recently from 6cbb497 to 67991f2 Compare October 16, 2023 06:51
@yhabteab yhabteab force-pushed the enhanced-filter-parser branch 7 times, most recently from 8b2c4eb to 3603283 Compare October 26, 2023 14:52
@yhabteab yhabteab force-pushed the enhanced-filter-parser branch from 1ccb78e to e5cd4e2 Compare December 1, 2023 09:02
@yhabteab yhabteab force-pushed the enhanced-filter-parser branch from e5cd4e2 to 98471a8 Compare April 16, 2024 12:52
@yhabteab yhabteab force-pushed the enhanced-filter-parser branch from 98471a8 to af89454 Compare August 1, 2024 11:53
@yhabteab yhabteab requested review from oxzi and julianbrost August 1, 2024 14:45
yhabteab and others added 24 commits August 13, 2024 21:51
By default, the scanner mode is set to `scanner.GoTokens` which contains
way more flags than are actually needed. This commit now sets this to
only recognize identifiers and `scanner#Scan()` will return all other
unrecognized tokens as they are.
Otherwise the dwarf information of that parser will get messed
up - incomplete and one will not be able to step into that source
code using a debugger (tested it using delve).
@yhabteab yhabteab force-pushed the enhanced-filter-parser branch from af89454 to 7142584 Compare August 16, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants