lua-nginx-module icon indicating copy to clipboard operation
lua-nginx-module copied to clipboard

feature: implemented a configure_by_lua phase

Open thibaultcha opened this issue 7 years ago • 15 comments

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:

  • [x] a new configure_by_lua phase (block/file) for http context
  • ngx_lua directives
    • [x] ngx_configure.shared_dict() -> lua_shared_dict
    • [x] ngx_configure.max_pending_timers() -> lua_max_pending_timers
    • [x] ngx_configure.max_running_timers() -> lua_max_running_timers
  • NGINX directives
    • [x] ngx_configure.env() -> env
  • [x] no NGINX patch required
  • [x] 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: https://github.com/openresty/lua-resty-core/pull/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.

thibaultcha avatar Feb 16 '18 03:02 thibaultcha

@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 avatar Feb 16 '18 11:02 spacewander

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.

spacewander avatar Feb 16 '18 11:02 spacewander

@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 avatar Feb 16 '18 16:02 thibaultcha

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.

thibaultcha avatar Feb 16 '18 16:02 thibaultcha

@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.

agentzh avatar Feb 26 '18 00:02 agentzh

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 avatar Feb 26 '18 01:02 thibaultcha

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! :)

thibaultcha avatar Feb 26 '18 01:02 thibaultcha

@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.

agentzh avatar Feb 26 '18 03:02 agentzh

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.

Someguynamedpie avatar May 02 '18 07:05 Someguynamedpie

This pull request is now in conflict :(

mergify[bot] avatar Jun 26 '20 00:06 mergify[bot]