feat(conf) add support to configure nginx directives via kong.conf #3530

hbagdi · 2018-06-11T20:24:51Z

Problem

Kong ships with an NGINX template which is rendered when Kong starts.
There exists no mechanisms to add/update any NGINX directive to the
nginx.conf used to run Kong. To change or add any directive, user has
to use a custom NGINX template which has to be synced with Kong for a
release which introduces changes to Kong's template.
Including options in kong.conf to configure NGINX directives is not a
good solution since the list will be endless.
This problem can be seen in #3010, #3323 and #3382.

Solution

There needs to be a flexible way to specify any NGINX directive
via Kong's config file without Kong needing to maintain a list of all
NGINX directives.
While a clean and ideal solution would be #2355, this commit
adopts a simpler as discussed like the one proposed in #2675.

NGINX directives can be specified using config variables with prefixes,
which help determine the block in which to place a directive.
eg:

nginx_proxy_add_header=Header-Name header-value will add a add_header Header-Name header-value; directive in the proxy server block of Kong.

nginx_http_lua_shared_dict=custom_cache 2k will add a a
lua_shared_dict custom_cache 2k; directive to HTTP block of Kong.

Problem ------- Kong ships with an NGINX template which is rendered when Kong starts. There exists no mechanisms to add/update any NGINX directive to the `nginx.conf` used to run Kong. To change or add any directive, user has to use a custom NGINX template which has to be synced with Kong for a release which introduces changes to Kong's template. Including options in `kong.conf` to configure NGINX directives is not a good solution since the list will be endless. This problem can be seen in #3010, #3323 and #3382. Solution -------- There needs to be a flexible way to specify any NGINX directive via Kong's config file without Kong needing to maintain a list of all NGINX directives. While a clean and ideal solution would be #2355, this commit adopts a simpler as discussed like the one proposed in #2675. NGINX directives can be specified using config variables with prefixes, which help determine the block in which to place a directive. eg: `nginx_proxy_add_header=Header-Name header-value` will add a `add_header Header-Name header-value;` directive in the proxy server block of Kong. `nginx_http_lua_shared_dict=custom_cache 2k` will add a a `lua_shared_dict custom_cache 2k;` directive to HTTP block of Kong.s

Signed-off-by: Harry Bagdi <[email protected]>

hishamhm · 2018-06-11T20:59:59Z

kong/conf_loader.lua

+
+  for k, v in pairs(conf) do
+    if type(k) == "string" then
+      local _ , _ , directive= string.find(k, prefix .. "(.+)")


minor: spacing (local _, _, directive = string.find...)

hishamhm · 2018-06-11T21:09:19Z

A wild idea that came to mind for supporting duplicated nginx directives: how about taking the key and running k:gsub("__", " ") at the appropriate place, so one can add multiple entries of the same nginx key using syntax like this: KONG_NGINX_HTTP_LUA_SHARED_DICT__CUSTOM_CACHE=200k, producing lua_shared_dict custom_cache 200k; in the nginx config file.

(The downside is that the arguments would always become lowercase when sent via environment variables (which may or may not be okay for headers), but for kong.conf that wouldn't be a problem.)

Signed-off-by: Harry Bagdi <[email protected]>

thibaultcha · 2018-06-11T23:04:20Z

kong/cmd/utils/env.lua

+local fmt = string.format
+
+
+local cmd = [[ printenv ]]


Maybe there is no need for this extra variable anymore, it is used only once.

It is used for the debug log as well.

thibaultcha · 2018-06-11T23:04:33Z

kong/cmd/utils/env.lua

+  local success, ret_code, stdout, stderr = pl_utils.executeex(cmd)
+  if not success or ret_code ~= 0 then
+    return nil, fmt("could not read environment variables (exit code: %d): %s",
+           ret_code, stderr)


style: alignment

thibaultcha · 2018-06-11T23:04:57Z

kong/cmd/utils/env.lua

+  for line in stdout:gmatch("[^\r\n]+") do
+    local i = string.find(line, "=") -- match first =
+    if i then
+      local k = string.sub(line,1, i-1)


style: sub(line, 1, i - 1)

thibaultcha · 2018-06-11T23:05:31Z

kong/cmd/utils/nginx_signals.lua

+  local ok, retcode, _, stderr = pl_utils.executeex(cmd)
+  if not ok then
+    return false, ("failed to validate nginx configuration " ..
+           "(exit code %d):\n%s"):format(retcode ,stderr)


style: alignment and space after the comma

thibaultcha · 2018-06-11T23:06:16Z

kong/cmd/utils/nginx_signals.lua

+    return false, ("failed to validate nginx configuration " ..
+           "(exit code %d):\n%s"):format(retcode ,stderr)
+  end
+


Let's also add a debug log when the validation succeededs, otherwise the user just sees "checking nginx conf" and does not know what happened after that.

UNIX philosophy of no output is good?

thibaultcha · 2018-06-11T23:10:05Z

kong/conf_loader.lua

+      find_dynamic_keys(from_file_conf, prefix)
+    end
+
+    defaults = tablex.merge(defaults, dynamic_keys, true)


Can you add a comment that this is building the union? This way the intent is clearer (as discussed) to readers who see tablex.merge() usage several times in this module. See below:

conf = tablex.merge(conf, defaults) -- intersection (remove extraneous properties)

thibaultcha · 2018-06-11T23:12:14Z

kong/init.lua

+      init = function()
+      end,
+    }
+  end


This should go below the check for required shms, otherwise configs lacking the shm would error during the actual start, and yet configuration validation would report a success...

Signed-off-by: Harry Bagdi <[email protected]>

hishamhm · 2018-06-12T13:48:29Z

spec/02-integration/05-proxy/15-custom_nginx_directive_spec.lua

+  local function start(config)
+    return function()
+      bp.routes:insert {
+        hosts = { "headers-inspect.com" },


Ah, a super minor thing but which I think we should start paying attention in our tests (perhaps worth an addition to the style guide even?) — I suggest we stop using "valid-looking" URLs in tests, even if we're not hitting them (we've had in the past tests that used an innocent-looking URL which ended up pointing to a porn site!) — we can use instead example.com or any URL using the reserved .test TLD (which then allows for "descriptive" test hostnames such as headers-inspect.test)

Thanks for sharing pointing this out!
I like the idea of using the .test TLD. I've updated the code to use headers-inspect.test.

+1

we've had in the past tests that used an innocent-looking URL which ended up pointing to a porn site

I've also seen cases of test/example code in public documentation using "domain.com", which at one point hosted malware; customers who blindly copy-pasted were in for a nasty surprise :p

IANA has reserved example.com for cases like this, FWIW. https://www.iana.org/domains/reserved

Just my $0.02.

thibaultcha · 2018-06-13T21:44:17Z

I like @hishamhm's idea of specifying multiple directives as such. Another approach we discussed with @hbagdi was to use semicolons to separate multiple values for a directive (ngx_http_lua_shared_dict=cats 12k; dogs24k). Yesterday we decided to postpone support for multiple instances of the same directive for now, in order to comfortably ship this in 0.14 and re-assessing later (also based on potential feature requests for this). So, delaying this for now unless someone else sees it as a blocker (or anything else), I am polishing this branch locally and am planning on pushing to next very soon.

thibaultcha · 2018-06-14T00:30:31Z

Manually merged, thanks!

This commit introduces a new shm, `kong_locks`, to be used by instances of lua-resty-lock across Kong or its libraries, if possible. Since #3543 bumped the mlcache dependency, the DB cache is the first consumer of this shm to store its locks, which helps in limiting the effects of LRU churning in certain workloads. More components should use this shm to store their resty-lock instances, especially instead of relying on the `kong` shm, which is to be considered "immutable". The chosen size for this shm is 8Mb: 1. In the not too far future, users should be able to customize the size of _any_ shm via configuration properties. Some additional work on top of #3530 is needed for this. 2. Such a size allows to store (based on estimations with a test script) about 65.000 lua-resty-lock items. While this number is very much workload specific, it should be appropriate even in situations where requests trigger cache misses in the runloop and the core, which would amount for up to 10, or maybe 15 locks (with very bad luck), which means we could still process several thousands of such cold requests concurrently, and comfortably. 3. Ideally, abstraction layers on top of eviction-sensitive shms should be built, in order to monitor LRU eviction on locks, or forbid them entirely on immutable shms. This commit is considered backwards-compatible and thus does not require the `kong_locks` shm to be defined.

This commit introduces a new shm, `kong_locks`, to be used by instances of lua-resty-lock across Kong or its libraries, if possible. Since #3543 bumped the mlcache dependency, the DB cache is the first consumer of this shm to store its locks, which helps in limiting the effects of LRU churning in certain workloads. More components should use this shm to store their resty-lock instances, especially instead of relying on the `kong` shm, which is to be considered "immutable". The chosen size for this shm is 8Mb: 1. In the not too far future, users should be able to customize the size of _any_ shm via configuration properties. Some additional work on top of #3530 is needed for this. 2. Such a size allows to store (based on estimations with a test script) about 65.000 lua-resty-lock items. While this number is very much workload specific, it should be appropriate even in situations where requests trigger cache misses in the runloop and the core, which would amount for up to 10, or maybe 15 locks (with very bad luck), which means we could still process several thousands of such cold requests concurrently, and comfortably. 3. Ideally, abstraction layers on top of eviction-sensitive shms should be built, in order to monitor LRU eviction on locks, or forbid them entirely on immutable shms. This commit is considered backwards-compatible and thus does not require the `kong_locks` shm to be defined. From #3550

TijoVarghese · 2018-12-19T21:42:39Z

I see that if we are trying to add multiple values for a directive, the later overrides the former value.
For eg:
nginx_http_log_format = a 'aa' nginx_http_log_format = b 'bb'
b will gets overrided for log_format. Is there any option available to resolve this? I need both log_format a & b should be defined

tzssangglass · 2020-07-16T15:26:33Z

我看到，如果我们尝试为一个指令添加多个值，则后者会覆盖前一个值。
例如：
nginx_http_log_format = a 'aa' nginx_http_log_format = b 'bb'
b将被log_format覆盖。是否有任何解决方案？我需要同时定义log_format a和b

Have you found a solution to this problem？

hbagdi added 2 commits June 11, 2018 13:22

[squash] fix environment read

392e203

Signed-off-by: Harry Bagdi <[email protected]>

hishamhm reviewed Jun 11, 2018

View reviewed changes

[squash] specify prefix in test case and rename file

659bbb6

Signed-off-by: Harry Bagdi <[email protected]>

thibaultcha reviewed Jun 11, 2018

View reviewed changes

[squash] check for shms, style and add comment

2317d42

Signed-off-by: Harry Bagdi <[email protected]>

hishamhm reviewed Jun 12, 2018

View reviewed changes

thibaultcha closed this Jun 14, 2018

thibaultcha deleted the feat/nginx-template branch June 14, 2018 00:30

thibaultcha mentioned this pull request Jun 14, 2018

refactor add LOG_FORMAT option #3274

Closed

thibaultcha mentioned this pull request Jun 15, 2018

feat(nginx-conf) add a 'kong_locks' shm for resty-lock users #3550

Merged

bungle mentioned this pull request Jul 3, 2018

Kong dropping some of the request headers #3586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(conf) add support to configure nginx directives via kong.conf #3530

feat(conf) add support to configure nginx directives via kong.conf #3530

hbagdi commented Jun 11, 2018

hishamhm Jun 11, 2018

hishamhm commented Jun 11, 2018 •

edited

Loading

thibaultcha Jun 11, 2018

hbagdi Jun 12, 2018

thibaultcha Jun 11, 2018

thibaultcha Jun 11, 2018

thibaultcha Jun 11, 2018

thibaultcha Jun 11, 2018

hbagdi Jun 12, 2018

thibaultcha Jun 11, 2018

thibaultcha Jun 11, 2018

hishamhm Jun 12, 2018

hbagdi Jun 12, 2018

p0pr0ck5 Jun 12, 2018

thibaultcha commented Jun 13, 2018

thibaultcha commented Jun 14, 2018

TijoVarghese commented Dec 19, 2018 •

edited

Loading

tzssangglass commented Jul 16, 2020

feat(conf) add support to configure nginx directives via kong.conf #3530

feat(conf) add support to configure nginx directives via kong.conf #3530

Conversation

hbagdi commented Jun 11, 2018

Problem

Solution

Choose a reason for hiding this comment

hishamhm commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Jun 13, 2018

thibaultcha commented Jun 14, 2018

TijoVarghese commented Dec 19, 2018 • edited Loading

tzssangglass commented Jul 16, 2020

hishamhm commented Jun 11, 2018 •

edited

Loading

TijoVarghese commented Dec 19, 2018 •

edited

Loading