nginxinc / nginx-go-crossplane

A library for working with NGINX configs in Go
Apache License 2.0
46 stars 12 forks source link

NLB-4532: Add lua module #84

Closed xynicole closed 4 months ago

xynicole commented 5 months ago

Proposed changes

Add lua module to crossplane. Validation reference to https://github.com/openresty/lua-nginx-module/blob/1d2aeb309c592484808f9b7313bc798461a37591/src/ngx_http_lua_module.c and https://github.com/openresty/lua-nginx-module/tree/master?tab=readme-ov-file#directives

_by_lua and _by_lua_block directives need additional lexer, I will do it in other PR

Checklist

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

dakshinai commented 5 months ago

LGTM with reference to https://github.com/openresty/lua-nginx-module/blob/1d2aeb309c592484808f9b7313bc798461a37591/src/ngx_http_lua_module.c

ornj commented 4 months ago

I think this PR is incomplete and you should add some tests for covering the lexer processing some of the more interesting looking lua directives. Here's a rather terse example test that covers this config demonstrating at least one *_by_lua directive is handled just fine:

http {
    server {
        set_by_lua $res ' return 32 + math.cos(32) ';
    }
}
diff --git a/lex_test.go b/lex_test.go
index 1b9504d..01303eb 100644
--- a/lex_test.go
+++ b/lex_test.go
@@ -226,6 +226,18 @@ var lexFixtures = []lexFixture{
        {";", 15},
        {"}", 16},
    }},
+   {"lua-simple", []tokenLine{
+       {"http", 1},
+       {"{", 1},
+       {"server", 2},
+       {"{", 2},
+       {"set_by_lua", 3},
+       {"$res", 3},
+       {" return 32 + math.cos(32) ", 3},
+       {";", 3},
+       {"}", 4},
+       {"}", 5},
+   }},
 }

 func TestLex(t *testing.T) {
xynicole commented 4 months ago

@ornj This MR is only for non *_by_luaand non *_by_lua_block directives, which don't need additional rules in the lexer. I'm still playing around with the lexer in another PR: https://github.com/nginxinc/nginx-go-crossplane/compare/NLB-4374-validation?expand=1 I might messed up some base cases, only passed half unit tests :(

ornj commented 4 months ago

which don't need additional rules in the lexer

That may be, but adding some additional tests that prove that fact would be nice. My example above demonstrates at least one *_by_lua directive that works without any changes to the lexer.

ornj commented 4 months ago

Approved, but I still would like it if there were some lexer tests. The analyzer tests are great, but some more coverage to prove the lexer works as well (even though we know it should) would make this more complete.

xynicole commented 4 months ago

@ornj added a lexer test for Lua

ornj commented 4 months ago

@xynicole looks good!