-
Notifications
You must be signed in to change notification settings - Fork 15
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
Bug: header_filter_by_lua_block parser doesn't detect directive end #32
Comments
It looks like the python version handles this already - https://github.com/nginxinc/crossplane/blob/master/crossplane/ext/lua.py |
Apologies, this feature already exists - https://github.com/nginxinc/nginx-go-crossplane/blob/main/parse.go#L52 |
On reflection, this is a bug in the parser which is not solved by ignoring the directive. After looking in the code the bug is worked around by ignoring the directive, and the other blocks are NOT excluded from the payload. The issue ATM appears to be the parser trying to handle the One path forward would be to detect these directives and consume their blocks, but instead of discarding them, turn them into strings. This could probably be done within the existing parser, but I've only been looking at it briefly and it's hurting my head. |
So I haven't actually addressed the bug here, it still stands. A config file which illustrates the issue I have is
Fixing the bug would only partially solve my problem, which is that I want to keep the lua code, but not have it parsed into the Directive structure. I've implemented another option for directives that should be handled similarly to the ignored directives. The difference is that we don't discard their content, instead we collect it and put it in the comment field for the directive in question. This appears to resolve the immediate issue I'm having, but it's not MR worthy - https://github.com/nginxinc/nginx-go-crossplane/compare/main...asharpe:nginx-go-crossplane:skip-block-contents?expand=1 Edit: Collecting the content is easy enough, but we lose all the structure which makes it impossible to reconstruct. I've tested on some moderately difficult lua and it got hard pretty quick (eg. single line comments). This is my first dip into lex/parse type stuff and I'm not sure it will be possible to preserve the lua without having the lexer understand it as well |
My current thinking is that This change moves the logic into the lexer and allows it to identify Edit: There are no tests on this yet, and I'm aware this doesn't yet handle |
The problem apparently is because the parser cannot detect end of lines, as we don't add the ";" after each directive. Example that works:
if you remove the ";" (which is right on by_lua blocks btw) it breaks |
I am not sure if this is the same issue, but I have figured out that Lua parsers are not used by default, so I created some example here: https://gist.github.com/rikatz/2928938c6d354366e7b30e8cffc69bf1 |
Is your feature request related to a problem? Please describe
This is mostly a feature request because it's related to openresty which adds additional directives to the nginx configuration, and I'm not sure that's a core concern for this organisation.
When using
*_by_lua_bock
directives the parser seems to get confused and adds configuration outside the block into the lua block.Example config
This generates JSON which looks like
where the
error_page
directive and subsequent directives come from the included file.Describe the solution you'd like
To get a valid representation of the configuration. The approach suggested here is to identify specific directives to exclude them from further parsing. This means the offending directive will still appear in the output, but its block will be empty. If these directives could be identified by command line options then the solution would work for as-yet unforeseen directives.
Describe alternatives you've considered
Fixing the parser to correctly process the
*_by_lua_block
directives (probably by turning them into strings). I don't think this is a great option because any nginx addon could add a directive which could fall afoul of the same issue.Additional context
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: