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

Add all 16 *_by_lua_block and 9 *_by_lua directives to crossplane #86

Merged
merged 40 commits into from
Jun 12, 2024

Conversation

xynicole
Copy link
Collaborator

@xynicole xynicole commented Apr 17, 2024

Proposed changes

Add all 16 *_by_lua_block and 9 *_by_lua directives to crossplane along with tests.
Add Lua lexer to allow Crossplane to read Lua blocks
Add Lua built
Fix incorrect Lua testdata

How do we parse Lua:
In lexer:

  1. Grab everything in the Lua block as a single token and drop the {}. Example:
    content_by_lua_block {some code} will be content_by_lua_block "some code"; "some code" is an argument of directive
  2. If a user puts the same argument name as the Lua block directives' name, we will treat it as a normal argument parse. Check if it starts with { which means token depth == 0.
  3. If a user puts the same upstream name as the Lua block directives' name, we will treat it as a normal argument parse. Check if the token is a directive
  4. Set_by_lua_block has a return value, so we need to additionally check for this directive. Get the return value token first and then obtain the rest of the block token.
  5. If a user puts quotes around the Lua directives, we need to skip these quotes.
  6. If there are braces inside the Lua block, we should retain them and not remove them as we did in # 1.

In parser (analyzer):
We want to use normal directives way to parse lua block, so
remove ngxConfBlock and change ngxConfNoArgs to ngxConfTake1, for set_by_lua_block change to ngxConfTake2. Just similar to server_name

In builder:
Add ExternalBuild to handle specific directives
Add { } back to the string

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • I have updated any relevant documentation (README.md)

@xynicole xynicole requested a review from a team as a code owner April 17, 2024 00:18
@xynicole xynicole changed the title WIP: Add Lex for *by_lua_block and *by_lua directives WIP: Add all 16 *_by_lua_block and 9 *_by_lua directives to crossplane May 14, 2024
@xynicole xynicole changed the title WIP: Add all 16 *_by_lua_block and 9 *_by_lua directives to crossplane Add all 16 *_by_lua_block and 9 *_by_lua directives to crossplane May 14, 2024
@xynicole xynicole force-pushed the ext-lua branch 4 times, most recently from 0afcfef to fba1886 Compare May 15, 2024 00:13
@xynicole xynicole requested a review from ornj May 15, 2024 17:38
Copy link
Member

@ornj ornj left a comment

Choose a reason for hiding this comment

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

Just commenting for now as I want to take a little more time to look at the lexing of Lua as I'm seeing differences between this implementation and Python other than what I expected.

analyze.go Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
@xynicole xynicole force-pushed the ext-lua branch 2 times, most recently from 7e66fb8 to 35c862c Compare May 20, 2024 19:55
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
@xynicole xynicole requested review from a team, nginx-nickc and nickchen May 28, 2024 18:12
build.go Outdated Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
ornj and others added 7 commits June 10, 2024 15:49
Added an extension for lexing an NGINX config that contains lua content.
Lua follows a different grammar than most NGINX configs. The extension
is based off the
[Python implementation](https://github.com/nginxinc/crossplane/blob/master/crossplane/ext/lua.py)
which just handles the Lua as a big multi-line string.
Wraps calls to `Scan()` to count EOL strings to retrieve token line
numbers and update the main tokenizer's line count.
build.go Outdated Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
lex.go Outdated Show resolved Hide resolved
build.go Outdated Show resolved Hide resolved
build.go Show resolved Hide resolved
ornj and others added 9 commits June 11, 2024 10:41
Moves the "register" responsibility to the options types, not 100% on
the API, but it's an idea. The interfaces are now single-method which
is pretty much as small as it gets. You could implement these interfaces
with a single function that calls itself.

A benefit to this is most obvious with the external Lexer I think as now
it is not responsible for storing a value for later use that carries
state and is not concurrent-safe.
Refactored the options so that callers cannot accidentally on purpose
modify other parts of the config struct.
Also removed some commented out code.
Refactor idea to simplify interfaces
Copy link
Member

@ornj ornj left a comment

Choose a reason for hiding this comment

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

I'd like to see docstrings on most of the added exported types/functions where it would help the user understand what to do with them (in case it's not obvious) but other than that, lgtm.

lex.go Outdated Show resolved Hide resolved
lua.go Outdated Show resolved Hide resolved
lua.go Outdated Show resolved Hide resolved
lua.go Show resolved Hide resolved
lua.go Show resolved Hide resolved
lua.go Outdated
}

case next == `"` || next == "'":
inQuotes = !inQuotes
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we know the "closing quote" is of the same type?
if i do the following, wouldn't this mess things up?

" ' foo ' "

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thanks for pointing that out! I added var quoteType string to track if quotes are matched

lua.go Show resolved Hide resolved
@xynicole xynicole merged commit a44aa6b into main Jun 12, 2024
2 checks passed
@xynicole xynicole deleted the ext-lua branch June 12, 2024 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants