feature: implemented a configure_by_lua phase #1259

thibaultcha · 2018-02-16T03:50:34Z

I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.

This phase's purpose would be to allow the configuration of NGINX/ngx_lua with Lua scripts. This PR proposes a first draft with just a few APIs, hoping that more APIs can be added progressively.

Ultimately, I cherish the vision to some day being able to dynamically configure http, location, stream, and other blocks right from Lua.

Example of what is currently implemented:

http {
    configure_by_lua_block {
        local ngx_configure = require "ngx.configure"

        -- hard-coded, but could be retrieved from I/O or env
        local shms = {
            { "dogs", "12k" },
            { "cats", "24k" }
        }

        -- create shms
        for _, shm in ipairs(shms) do
            ngx_configure.shared_dict(shm[1], shm[2])
        end

        -- conditionally expose env variables for workers
        ngx_configure.env("PROXY_PASS_URL")

        -- configure the Lua VM
        ngx_configure.max_pending_timers(1024)
        ngx_configure.max_running_timers(128)
    }

    init_by_lua_block {
        ngx.shared.dogs:set("hello", true)
    }
}

List of what is currently implemented:

a new configure_by_lua phase (block/file) for http context
ngx_lua directives
- ngx_configure.shared_dict() -> lua_shared_dict
- ngx_configure.max_pending_timers() -> lua_max_pending_timers
- ngx_configure.max_running_timers() -> lua_max_running_timers
NGINX directives
- ngx_configure.env() -> env
no NGINX patch required
naming convention of APIs strips lua_ prefix from directives

Everything is implemented via the LuaJIT FFI in the lua-resty-core sister PR located at: openresty/lua-resty-core#176.

My current goal is to get feedback, reviews, guidance... anything :) In the long term, I wish to gradually add more relevant API to this phase and make OpenResty more dynamic. If anyone can think of anything in the current implementation that would eventually impact this goal or limit future possibilities, please let me know.

spacewander · 2018-02-16T11:03:59Z

@thibaultcha
Sorry for mentioning one thing: Why don't you try to make the Nginx configure file as a template, and render it before starting/reloading the server?
The team I last worked with did this in production over one year ago, and it works.

spacewander · 2018-02-16T11:10:31Z

PS. You could use priviledge process to detect configure change and send reload signal. And the template render could be implemented in Lua, so no other dependency is required.

thibaultcha · 2018-02-16T16:55:54Z

@spacewander This is precisely what we have been doing for the past few years, and what this PR tries to get rid of :)

The goal here is to avoid having to depend so heavily on templating all the time to configure our applications. Additionally, templating causes huge headaches to users who need to make their own modifications to their Nginx configuration file, not only to learn it but also for each upgrade with configuration changes. Really, the goal here is to not have to do that anymore.

thibaultcha · 2018-02-16T16:59:14Z

Especially considering the maintainability cost seems to be relatively low. No Nginx patch required, a dependency on an init, process that is very unlikely to ever change, and a self-contained patch in the codebase. I believe the ability to configure servers and location blocks would be possible by respecting the same constraints as well.

agentzh · 2018-02-17T02:22:53Z

src/ngx_http_lua_configureby.c

+
+
+char *
+ngx_http_lua_configure_by_lua_block(ngx_conf_t *cf, ngx_command_t *cmd,


I don't like all the code duplications in this C source file. It makes the code base harder to maintain and sync new changes.

I can definitely improve it :) Which area? The context runner and/or the shm configure API?

@thibaultcha Almost all the code in this C source file is duplicated C code.

@agentzh I just added a new commit which introduces ngx_http_lua_shared_dict_add, which factorizes all of the shared code for shm create for lua_shared_dict and ngx_configure.shared_dict().

As stated before, the code related to parsing and running the _by_*_block and _by_*_file directives of this module are already always duplicated - this feels outside the scope of this PR, but if you want me to address this, please advise.

@thibaultcha

All of the Lua phases code is duplicated already, I just followed the pattern.

What exactly are you referring to?

The thing is if the config phase handlers can call these functions, then we should make it that way. I don't want to maintain 2 duplicated copies of configurations. Also, how do you handle config directive overriding and inheritance here? Seems like you already bypass the nginx config system entirely?

how do you handle config directive overriding and inheritance here? Seems like you already bypass the nginx config system entirely?

There is no inheritance involved here since we only support configuration of http (or above) level directives, and this is deliberate.

Ultimately, I do wish to support lower levels of configuration as explained in the PR (server, location...), but without it there are no need for supporting inheritance yet.

What exactly are you referring to?

I was reffering to the only part of the code that I see is duplicated from other parts of ngx_lua, and that is the ngx_http_lua_configure_by_lua and other handlers that run the Lua code for this phase.

If not, then I am not sure what you are referring to when you are saying things are duplicated?

agentzh · 2018-02-17T02:24:27Z

src/ngx_http_lua_shdict.c


- if (lmcf->shdict_zones != NULL) {


This also makes such optimizations no longer exist. I'm kinda proud of the concept that you do not pay for the features you never use :)

What do you mean? We only create the shdict metatable if we need it still (via lua_shared_dict or ngx_configure.shared_dict())

@thibaultcha We used to save the metatable when lua_shared_dict is not configured in nginx.conf. This is a minor thing though.

@thibaultcha It seems that you essentially created a second parallel configuring code path for those config directives we already have.

We used to save the metatable when lua_shared_dict is not configured in nginx.conf.

And we still do, I made sure of that, this method has been renamed to reflect the change but both of its calls in the codebase are enclosed within conditions. I could re-add the safeguard condition in here too just in case.

It seems that you essentially created a second parallel configuring code path for those config directives we already have.

All of the Lua phases code is duplicated already, I just followed the pattern.
Regarding the add shm API, I can certainly improve it - I originally didn't want to modify existing code too much by fear the patch would be refused if so.
Regarding the timers code, I was thinking of coming up with a more generic way of setting integer values onto the lmcf structure (we could follow the NGINX pattern with ngx_conf_set_num_slot and the likes) but that seems unnecessary for only 2 fields as of today and considering it would result in a lot more code.

thibaultcha · 2018-02-18T22:37:57Z

src/ngx_http_lua_directive.c

 return NGX_CONF_ERROR;
 }

- *zp = zone;
-
- lmcf->requires_shm = 1;


This was duplicated (as in the master branch) - we now only set it from ngx_http_lua_shared_memory_add.

thibaultcha · 2018-02-18T22:39:16Z

src/ngx_http_lua_api.c

@@ -122,9 +121,7 @@ ngx_http_lua_shared_memory_add(ngx_conf_t *cf, ngx_str_t *name, size_t size,
 return &ctx->zone;
 }

- n = sizeof(ngx_http_lua_shm_zone_ctx_t);
-
- ctx = ngx_pcalloc(cf->pool, n);


Just cleaning up here but I can revert if not desired.

agentzh · 2018-02-26T00:56:08Z

@thibaultcha I appreciate your exploration in this direction. I'm thinking about maybe we could make this work on a much lower level, by dynamically configuring all the nginx main/http/stream/server/location blocks in Lua. Your approach looks like we will simply duplicate all the existing nginx configuraiton directives in this new facility, which does not have a future IMHO (not to mention there might be an infinite number of 3rd-party nginx modules out there providing their infinite number of possible directive that could interact with OpenResty) :)

If we want to get rid of nginx.conf, then let's do it in a thorough way :) Just my 2 cents.

thibaultcha · 2018-02-26T01:18:36Z

we could make this work on a much lower level, by dynamically configuring all the nginx main/http/stream/server/location blocks in Lua

Yes, this is exactly my point and my intent on the long term (as explained above). The current patch is more of an intermediate phase for the nginx main and http context directives, because I am of the opinion that we should build this step-by-step, and add more APIs (including APIs dynamically adding server and location blocks) to it in the future.

Your approach looks like we will simply duplicate all the existing nginx configuraiton directives in this new facility, which does not have a future IMHO

Maybe not all :) This approach would already be quite helpful imho - we've been wanting for years to only allocate the lua_shared_dict that our customers' dynamically configured instances need, without requiring of them that they learn or modify our nginx configuration templating system (that we wish to get rid of in the long term). I believe a lot of users would be happy to allocate shared dicts from the Lua-land (I have witnessed many asks for it over the years in the mailing list, issues, or chats...).

not to mention there might be an infinite number of 3rd-party nginx modules out there providing their infinite number of possible directive that could interact with OpenResty

Agreed. But it is not because we cannot please everybody that we should please nobody :) Covering the directives that are used the most often (like env, lua_shared_dict for starters) would already satisfy a lot of users imho. Besides, how do you intend to support the much larger amount of available directives in the server and location contexts?

One solution I thought of (I did not explore its feasibility) was to avoid creating structures representing the state of the configuration like this:

ngx.server.my_server:add_location("/")

Which would become a maintenance nightmare, but instead supporting a more lose API, like:

ngx.configure.append [[
server {
    listen :8080;
    
    location {
        return 200;
    }
}
]]

Maybe the final solution will be something in the middle.

If we want to get rid of nginx.conf, then let's do it in a thorough way

I am of the same opinion, but I also think that this intermediate step is a good one to take until more APIs are added to this phase that will ultimately allow us to get rid of nginx.conf.

thibaultcha · 2018-02-26T01:27:08Z

I very much want to highlight the following sentences from this PR's opening thread:

This phase's purpose would be to allow the configuration of NGINX/ngx_lua with Lua scripts. This PR proposes a first draft with just a few APIs, hoping that more APIs can be added progressively.

Ultimately, I cherish the vision to some day being able to dynamically configure http, location, stream, and other blocks right from Lua.

The current proposal is to iterate slowly towards that goal with a few APIs that would already be helpful. If you prefer the idea I have outlined above, or if you have a better one altogether that you wish to share, or if dynamically configuring nginx is not something that OpenResty wishes to achieve, I am fine with this, please do let me know! :)

agentzh · 2018-02-26T03:16:26Z

@thibaultcha I do understand your intention but unfortunately I don't see this PR as an iterative approach to the right direction from the perspective of an implementator. This patch adds quite some maintenance overhead with very little real world merit IMHO.

Someguynamedpie · 2018-05-02T07:35:18Z

Down the line, being able to append or "execute" blocks of NGINX config (even as strings to lower the implementation requirements) from init_by_lua would be a godsend for server management tools.

mergify · 2020-06-26T00:39:33Z