Skip to content

feat: implemented the lua_load_resty_core directive. #1501

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

feat: implemented the lua_load_resty_core directive. #1501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e052887
feat: implemented the lua_load_resty_core directive which loads resty…
thibaultcha Apr 2, 2019
08f3959
tests: fixed default lua path to load resty.core from many test cases.
thibaultcha Apr 2, 2019
36ab412
tests: fixed a few failing test cases due to loading resty.core.
thibaultcha Apr 3, 2019
3d3ce00
[removeme] travis: switched to the contributor's branch of lua-resty-…
thibaultcha Apr 3, 2019
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ install:
- git clone https://github.com/openresty/rds-json-nginx-module.git ../rds-json-nginx-module
- git clone https://github.com/openresty/srcache-nginx-module.git ../srcache-nginx-module
- git clone https://github.com/openresty/redis2-nginx-module.git ../redis2-nginx-module
- git clone https://github.com/openresty/lua-resty-core.git ../lua-resty-core
- git clone -b feat/lua-load-resty-core https://github.com/thibaultcha/lua-resty-core.git ../lua-resty-core
- git clone https://github.com/openresty/lua-resty-lrucache.git ../lua-resty-lrucache
- git clone https://github.com/openresty/lua-resty-mysql.git ../lua-resty-mysql
- git clone https://github.com/openresty/stream-lua-nginx-module.git ../stream-lua-nginx-module
Expand Down
33 changes: 33 additions & 0 deletions README.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,7 @@ See Also
Directives
==========

* [lua_load_resty_core](#lua_load_resty_core)
* [lua_capture_error_log](#lua_capture_error_log)
* [lua_use_default_type](#lua_use_default_type)
* [lua_malloc_trim](#lua_malloc_trim)
Expand Down Expand Up @@ -1079,6 +1080,38 @@ how the result will be used. Below is a diagram showing the order in which direc

[Back to TOC](#table-of-contents)

lua_load_resty_core
-------------------

**syntax:** *lua_load_resty_core on|off*

**default:** *lua_load_resty_core on*

**context:** *http*

Controls whether the `resty.core` module (from
[lua-resty-core](https://github.com/openresty/lua-resty-core)) should be loaded
or not. When enabled, this directive is equivalent to executing the following
when the Lua VM is created:

```lua

require "resty.core"
```

Note that usage of the `resty.core` module is recommended, as its
FFI implementation is both faster, safer, and more complete than the Lua C API
of the ngx_lua module.

It must also be noted that the Lua C API of the ngx_lua module will eventually
be removed, and usage of the FFI-based API (i.e. the `resty.core`
module) will become mandatory. This directive only aims at providing a
temporary backwards-compatibility mode in case of edge-cases.

This directive was first introduced in the `v0.10.15` release.

[Back to TOC](#directives)

lua_capture_error_log
---------------------
**syntax:** *lua_capture_error_log size*
Expand Down
28 changes: 28 additions & 0 deletions doc/HttpLuaModule.wiki
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,6 +835,34 @@ how the result will be used. Below is a diagram showing the order in which direc

![Lua Nginx Modules Directives](https://cloud.githubusercontent.com/assets/2137369/15272097/77d1c09e-1a37-11e6-97ef-d9767035fc3e.png)

== lua_load_resty_core ==

'''syntax:''' ''lua_load_resty_core on|off''

'''default:''' ''lua_load_resty_core on''

'''context:''' ''http''

Controls whether the <code>resty.core</code> module (from
[https://github.com/openresty/lua-resty-core lua-resty-core]) should be loaded
or not. When enabled, this directive is equivalent to executing the following
when the Lua VM is created:

<geshi lang="lua">
require "resty.core"
</geshi>

Note that usage of the <code>resty.core</code> module is recommended, as its
FFI implementation is both faster, safer, and more complete than the Lua C API
of the ngx_lua module.

It must also be noted that the Lua C API of the ngx_lua module will eventually
be removed, and usage of the FFI-based API (i.e. the <code>resty.core</code>
module) will become mandatory. This directive only aims at providing a
temporary backwards-compatibility mode in case of edge-cases.

This directive was first introduced in the <code>v0.10.15</code> release.

== lua_capture_error_log ==
'''syntax:''' ''lua_capture_error_log size''

Expand Down
2 changes: 2 additions & 0 deletions src/ngx_http_lua_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,8 @@ struct ngx_http_lua_main_conf_s {
ngx_cycle_t *cycle;
ngx_pool_t *pool;

ngx_flag_t load_resty_core;

ngx_int_t max_pending_timers;
ngx_int_t pending_timers;

Expand Down
12 changes: 12 additions & 0 deletions src/ngx_http_lua_module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ static ngx_conf_bitmask_t ngx_http_lua_ssl_protocols[] = {

static ngx_command_t ngx_http_lua_cmds[] = {

{ ngx_string("lua_load_resty_core"),
NGX_HTTP_MAIN_CONF|NGX_CONF_FLAG,
ngx_conf_set_flag_slot,
NGX_HTTP_MAIN_CONF_OFFSET,
offsetof(ngx_http_lua_main_conf_t, load_resty_core),
NULL },

{ ngx_string("lua_max_running_timers"),
NGX_HTTP_MAIN_CONF|NGX_CONF_TAKE1,
ngx_conf_set_num_slot,
Expand Down Expand Up @@ -877,6 +884,7 @@ ngx_http_lua_create_main_conf(ngx_conf_t *cf)
*/

lmcf->pool = cf->pool;
lmcf->load_resty_core = NGX_CONF_UNSET;
lmcf->max_pending_timers = NGX_CONF_UNSET;
lmcf->max_running_timers = NGX_CONF_UNSET;
#if (NGX_PCRE)
Expand Down Expand Up @@ -910,6 +918,10 @@ ngx_http_lua_init_main_conf(ngx_conf_t *cf, void *conf)
{
ngx_http_lua_main_conf_t *lmcf = conf;

if (lmcf->load_resty_core == NGX_CONF_UNSET) {
lmcf->load_resty_core = 1;
}

#if (NGX_PCRE)
if (lmcf->regex_cache_max_entries == NGX_CONF_UNSET) {
lmcf->regex_cache_max_entries = 1024;
Expand Down
23 changes: 23 additions & 0 deletions src/ngx_http_lua_util.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,13 @@ static int ngx_http_lua_get_raw_phase_context(lua_State *L);
#define LUA_PATH_SEP ";"
#endif


#if !defined(LUA_DEFAULT_PATH) && (NGX_DEBUG)
#define LUA_DEFAULT_PATH "../lua-resty-core/lib/?.lua;" \
"../lua-resty-lrucache/lib/?.lua"
#endif


#define AUX_MARK "\1"


Expand Down Expand Up @@ -3813,6 +3820,7 @@ ngx_http_lua_init_vm(lua_State *parent_vm, ngx_cycle_t *cycle,
ngx_pool_t *pool, ngx_http_lua_main_conf_t *lmcf, ngx_log_t *log,
ngx_pool_cleanup_t **pcln)
{
int rc;
lua_State *L;
ngx_uint_t i;
ngx_pool_cleanup_t *cln;
Expand Down Expand Up @@ -3880,6 +3888,21 @@ ngx_http_lua_init_vm(lua_State *parent_vm, ngx_cycle_t *cycle,
lua_pop(L, 2);
}

if (lmcf->load_resty_core) {
lua_getglobal(L, "require");
lua_pushstring(L, "resty.core");

rc = lua_pcall(L, 1, 1, 0);
if (rc != 0) {
ngx_log_error(NGX_LOG_ERR, log, 0,
"lua_load_resty_core failed to load the resty.core "
"module from https://github.com/openresty/lua-resty"
"-core; ensure you are using an OpenResty release "
"from https://openresty.org/en/download.html "
"(rc: %i, reason: %s)", rc, lua_tostring(L, -1));
Copy link
Contributor

@ElvinEfendi ElvinEfendi May 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Does this mean lua-nginx-module can not be used outside of OpenResty bundle anymore? In one of our projects we compile lua-nginx-module manually and also include lua-resty-core and set lua package path correctly. Since this PR Nginx fails when starting with this error. When I turn off lua_load_resty_core and do just require("resty.core") it works as expected.

Note that the error does not recur when I reload Nginx.
Copy link
Member

@agentzh agentzh May 27, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@ElvinEfendi It still can, just a little bit harder as you found out. If you want to add Lua library search paths to your own nginx build at build time, then just override the values of the macros LUA_DEFAULT_PATH and LUA_DEFAULT_CPATH to include your own installation paths of lua-resty-core (and lua-resty-lrucache). The OpenResty distribution's build system does exactly that.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it should also work if the user specify the lua_package_path and lua_package_cpath directives. @thibaultcha The lua_code_resty_core directive does not run after these search path nginx directives?
Copy link
Contributor

Choose a reason for hiding this comment

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

@agentzh the project I'm talking about is https://github.com/kubernetes/ingress-nginx/, and we are actually considering to use OpenResty as base image instead of Nginx.

--

I replicated CI image in this repository and renamed lua-resty-core and lua-resty-lrucache folders to something else. I then edited t/161-load-resty-core.t added ONLY to the first test and also added lua_package_path "/lua-resty-corexxx/lib/?.lua;/lua-resty-lrucachexxx/lib/?.lua;;"; using --- http_config.
/lua-resty-corexxx and /lua-resty-lrucachexxx are what I renamed lua-resty-core and lua-resty-lrucache folders to. Then running test it succeeds. So that tells me it respects lua_package_path in this case. Not sure yet why it does not work in https://github.com/kubernetes/ingress-nginx though. The only difference between the Nginx we use in ingress-nginx and 1.15.8 (also the version I used in the above test: https://github.com/ElvinEfendi/lua-nginx-module/pull/1/files#diff-3254677a7917c6c01f55212f86c57fbfR7) that OpenResty uses is that we use Nginx 1.17.0.
Copy link
Member

Choose a reason for hiding this comment

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

@ElvinEfendi Then it's not something on our side? For OpenResty, then it already includes lua-resty-core or lua-resty-lrucache in its default search paths.
Copy link
Member

Choose a reason for hiding this comment

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

@ElvinEfendi For us to support you, you need to prepare a minimal and self-contained example. We are not maintaining that ingress-nginx project anyway, not to mention your own setup.
Copy link
Contributor

@ElvinEfendi ElvinEfendi May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@agentzh all I wanted to clarify was "Does this mean lua-nginx-module can not be used outside of OpenResty bundle anymore?" :) Because the error message implies that. But I got my answer, which is it should be possible.

I co-maintain ingress-nginx project - will look into this further on our side to see what's going on and will also potentially switch to OpenResty as base image for that project if the dependencies we need play well with OpenResty bundle.

Thanks for taking time to look into this though!
Copy link
Member

Choose a reason for hiding this comment

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

Well, that's a recommendation. Not a requirement.
Copy link
Member Author

@thibaultcha thibaultcha May 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@ElvinEfendi The lua_load_resty_core directive respects the LUA_PATH environment variable and the lua_package_path directive. In fact, I just pushed a new test asserting this behavior: 4102b82

Make sure that you also clone lua-resty-lrucache which is itself a dependency of lua-resty-core.
Or maybe you set your Lua path in Lua-land (e.g. package.path = "...;" .. package.path)? In which case, that'd be too late for this directive to honor the change.

Providing a minimally reproducible example with error logs may be useful here, but again as @agentzh said, this isn't really an issue with this module.

This module can still be used independently but the margin for error is high for most users, hence the warnings and other logs (e.g. lua-resty-core, LuaJIT's fork, etc...) asking to prefer the formal OpenResty releases.
Copy link

Choose a reason for hiding this comment

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

Where can I set package.path?
}
}

return L;
}

Expand Down
2 changes: 1 addition & 1 deletion t/004-require.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ __DATA__

=== TEST 1: sanity
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
location /main {
echo_location /load;
Expand Down
2 changes: 1 addition & 1 deletion t/013-base64.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,4 +242,4 @@ GET /t
--- response_body_like: 500 Internal Server Error
--- error_code: 500
--- error_log eval
qr/bad argument \#2 to 'encode_base64' \(boolean expected, got number\)|\[error\] .*? boolean argument only/
qr/bad argument \#2 to 'encode_base64' \(boolean expected, got number\)|\[error\] .*? bad no_padding: boolean expected, got number/
10 changes: 5 additions & 5 deletions t/014-bugs.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ __DATA__

=== TEST 1: sanity
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
location /load {
content_by_lua '
Expand All @@ -69,7 +69,7 @@ end

=== TEST 2: sanity
--- http_config
lua_package_path '/home/agentz/rpm/BUILD/lua-yajl-1.1/build/?.so;/home/lz/luax/?.so;./?.so';
lua_package_cpath '/home/agentz/rpm/BUILD/lua-yajl-1.1/build/?.so;/home/lz/luax/?.so;./?.so';
--- config
location = '/report/listBidwordPrices4lzExtra.htm' {
content_by_lua '
Expand Down Expand Up @@ -847,7 +847,7 @@ ok

=== TEST 37: resolving names with a trailing dot
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
location /t {
resolver $TEST_NGINX_RESOLVER ipv6=off;
Expand All @@ -865,7 +865,7 @@ GET /t

=== TEST 38: resolving names with a trailing dot
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';
server {
listen 12354;

Expand All @@ -892,7 +892,7 @@ args: foo=1&bar=2

=== TEST 39: lua_code_cache off + setkeepalive
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
lua_code_cache off;
location = /t {
Expand Down
6 changes: 3 additions & 3 deletions t/023-rewrite/req-socket.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ found the end of the stream

=== TEST 4: attempt to use the req socket across request boundary
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
location /t {
rewrite_by_lua '
Expand Down Expand Up @@ -376,7 +376,7 @@ hello world
=== TEST 5: receive until on request_body - receiveuntil(1) on the last byte of the body
See https://groups.google.com/group/openresty/browse_thread/thread/43cf01da3c681aba for details
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
location /t {
rewrite_by_lua '
Expand Down Expand Up @@ -440,7 +440,7 @@ done

=== TEST 6: pipelined POST requests
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
location /t {
rewrite_by_lua '
Expand Down
12 changes: 6 additions & 6 deletions t/023-rewrite/socket-keepalive.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ __DATA__

=== TEST 1: sanity
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
location /t {
set $port $TEST_NGINX_MEMCACHED_PORT;
Expand Down Expand Up @@ -104,7 +104,7 @@ lua tcp socket get keepalive peer: using connection

=== TEST 2: free up the whole connection pool if no active connections
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
location /t {
set $port $TEST_NGINX_MEMCACHED_PORT;
Expand Down Expand Up @@ -177,7 +177,7 @@ received: OK

=== TEST 3: upstream sockets close prematurely
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
server_tokens off;
keepalive_timeout 100ms;
Expand Down Expand Up @@ -254,7 +254,7 @@ done

=== TEST 4: http keepalive
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
server_tokens off;
location /t {
Expand Down Expand Up @@ -815,7 +815,7 @@ lua tcp socket keepalive timeout: unlimited
=== TEST 11: sanity (uds)
--- http_config eval
"
lua_package_path '$::HtmlDir/?.lua;./?.lua';
lua_package_path '$::HtmlDir/?.lua;./?.lua;;';
server {
listen unix:$::HtmlDir/nginx.sock;
default_type 'text/plain';
Expand Down Expand Up @@ -953,7 +953,7 @@ lua tcp socket get keepalive peer: using connection

=== TEST 13: github issue #110: ngx.exit with HTTP_NOT_FOUND causes worker process to exit
--- http_config eval
"lua_package_path '$::HtmlDir/?.lua;./?.lua';"
"lua_package_path '$::HtmlDir/?.lua;./?.lua;;';"
--- config
error_page 404 /404.html;
location /t {
Expand Down
Loading