feat(config) flexible tokens/headers configuration #3300

hbagdi · 2018-03-15T05:33:19Z

Instead of introducing a config option for each
and every header or set of headers, an array of
these values can be now specified using the headers
config option.

Only headers or tokens specified in the headers will
be set by Kong when applicable.

The goal here is to move towards a simpler and
easier to understand configuration, similar to
1b9976f (#3147).

hbagdi · 2018-03-15T18:02:18Z

@thibaultcha Does this look like the right way to go forward?

thibaultcha · 2018-03-17T07:57:29Z

@hbagdi Nice! It looks like there is more work that needs to happen still (we can improve the "sugar" and make the checks more readable probably, + style issues), but otherwise it looks like it is on the right track :)

thibaultcha · 2018-03-17T07:58:05Z

kong/conf_loader.lua

@@ -11,7 +11,9 @@ local utils = require "kong.tools.utils"
 local log = require "kong.cmd.utils.log"
 local ip = require "kong.tools.ip"
 local ciphers = require "kong.tools.ciphers"
+local bit           = require "bit"


let's keep this simple:

local bit = require "bit"

thibaultcha · 2018-03-17T07:59:21Z

kong/conf_loader.lua


+local bor           = bit.bor


no need to cache that as an upvalue, in fact not doing so might provide more readable code in this particular case (bit.bor being more specific than bor). This is also not a global, nor is this a hot code path, nor is the function name long, so...

thibaultcha · 2018-03-17T08:00:26Z

kong/conf_loader.lua

+      if token == "off" then
+          mask = 0
+          break
+      elseif allowed[token] == nil then -- TODO what about case sensetive??


style: need line jump before this line

It's ok to be case-sensitive imho, and expect server and not SERVER or ServER.

thibaultcha · 2018-03-17T08:00:35Z

kong/conf_loader.lua

+          break
+      elseif allowed[token] == nil then -- TODO what about case sensetive??
+        errors[#errors+1] = "tokens: invalid entry '" .. tostring(token) .. "'"
+      else


style: ditto (line jump)

Sorry about all the styling issues in this (and all the previous) PR. I need to figure out my styling and code convention issues.

thibaultcha · 2018-03-17T08:02:24Z

kong/constants.lua

+local bit           = require "bit"
+
+
+local bor           = bit.bor


ditto, these 2 instructions are adding quite a lot of whitespaces and blank lines

thibaultcha · 2018-03-17T08:07:57Z

kong/core/error_handlers.lua

@@ -56,7 +59,7 @@ return function(ngx)
  local status = ngx.status
  message = BODIES["s" .. status] and BODIES["s" .. status] or format(BODIES.default, status)

-  if singletons.configuration.server_tokens then
+  if band(constants.HEADER_MASKS[constants.HEADERS.SERVER], singletons.configuration.header_mask) then


Thinking about this, we can probably optimize this by building a table in conf_loader that we can query like so:

local config = singletons.config if config.header_tokens.server then -- ... end if config.header_tokens.via then -- ... end

This way we save both the AND operation, and simplify the readability of those conditions.

This makes a lot of sense and gets rid off all the bit operations all together.

-- in conf_loader.load() local headers = { [headers.PROXY_LATENCY] = false, [headers.UPSTREAM_LATENCY] = false, [headers.SERVER] = false, [headers.VIA] = false, } -- loop through headers field in configuration and update above table -- and finally config.headers = headers

One question,
Do we want to have server_tokens or latency_tokens boolean in our singleton configuration table or having only header granularity is enough? We will still have the above tokens in the config file as sugar which simply enable the set of headers.

thibaultcha · 2018-03-17T08:08:35Z

kong/constants.lua

+    [headers.UPSTREAM_LATENCY] = 0x02,
+    [headers.SERVER] = 0x04,
+    [headers.VIA] = 0x08,
+    latency = bor(0x01,0x02),


Now that this property refers to multiple headers, this should be latencies I believe

thibaultcha · 2018-03-17T08:09:18Z

kong/constants.lua

+    [headers.SERVER] = 0x04,
+    [headers.VIA] = 0x08,
+    latency = bor(0x01,0x02),
+    server = bor(0x04,0x08),


This one could be server_tokens (and the property in the configuration could be headers probably?) Sorry if I mislead you while drafting the quick implementation on the fly while we chatted. What do you think?

I put in names just as placeholders since I was having a difficult time in deciding on which one to chose.

headers as a property in user visible configuration property is much easier to grok.
I understand that server_tokens is coming from NGINX world and we would like to stick to the conventions but could that instead be server_names? Is it a bit more descriptive?
I'm thinking:

headers = server_names, kong_latencies, X-Kong-Foo-Bar

hbagdi · 2018-03-19T20:19:36Z

kong/conf_loader.lua

+    [headers.SERVER] = false,
+    [headers.VIA] = false,
+    server_tokens = false,
+    latency_tokens = false,


Not sure if *_tokens are needed here.

hbagdi · 2018-03-19T20:22:42Z

kong/conf_loader.lua

+      header_tokens[headers.UPSTREAM_LATENCY] = true
+    end
+  end
+  conf.headers = header_tokens


conf.headers = build_headers_table(conf.headers)

?

In check_and_infer, we should only do validation (the errors[#errors+1] = ... part). Below and once we know all values are correct, we can mutate the conf.headers table.

hbagdi · 2018-03-19T22:23:45Z

spec-old-api/02-integration/05-proxy/12-server_tokens_spec.lua

@@ -80,7 +80,7 @@ describe("Server Tokens", function()

    setup(start {
      nginx_conf    = "spec/fixtures/custom_nginx.template",
-      server_tokens = "on",
+      headers = "server_tokens",


@thibaultcha what is the approach to put new tests? Should they go into both spec and spec-old-api? '
I think only in spec but want to confirm

Yes, just spec. old-spec is for backards-compatibility with 0.12 and the API entity

Thanks for the quick response!

hbagdi · 2018-04-03T17:55:34Z

@thibaultcha ping

thibaultcha

This is looking really good, but still a few items to fix before we can merge it.

thibaultcha · 2018-04-11T17:49:20Z

kong/conf_loader.lua

+          break
+
+      elseif header_tokens[token] == nil then
+        errors[#errors+1] = "tokens: invalid entry '" .. tostring(token) .. "'"


prefix should be "headers: ..."?

We also need tests for this error which I don't see any for?

Addressed both of these in the updated commit.

thibaultcha · 2018-04-11T17:52:03Z

kong/conf_loader.lua

+      header_tokens[headers.UPSTREAM_LATENCY] = true
+    end
+  end
+  conf.headers = header_tokens


In check_and_infer, we should only do validation (the errors[#errors+1] = ... part). Below and once we know all values are correct, we can mutate the conf.headers table.

thibaultcha · 2018-04-11T17:53:37Z

kong/core/handler.lua

          header[constants.HEADERS.UPSTREAM_LATENCY] = ctx.KONG_WAITING_TIME
+        end
+
+        if singletons.configuration.headers[constants.HEADERS.PROXY_LATENCY] then


style: the line below has now has a weird looking assignment operator with too many spaces, fixing it should be in the scope of this PR

Apologies for this one!

thibaultcha · 2018-04-18T04:19:47Z

kong/conf_loader.lua

+  -- load headers configuration
+  for _, token in ipairs(conf.headers) do
+    if token == "off" then
+        break


This behaviour is slightly off with that of the new directives like proxy_listen - fixing it should be easy.

In the new listeners directives, off only has meaning if it is the first value:

proxy_listen = off, ...

With this headers directive however, the following is possible but slightly confusing:

headers = latency_tokens, off, server_tokens

Here, we end up in a situation where "half" of the specified tokens were taken into consideration. This could have (arguably) been considered an acceptable behaviour, but better stay consistent with the stricter semantics of the *_listen directives imho. So the above would set both latency_tokens and server_tokens, and to disable headers, one must do:

headers = off

Frankly, the off flag probably should not be mixed with actual values (that is bad implementation from the listeners directives to accept that imho), but well, not the scope of this PR to address, and better stick to this behaviour for now probably.

The above validation can probably implement the same behaviour (or ignore "off" values altogether, thus validating all non-off values in this list, up to you).

hbagdi · 2018-04-18T16:00:00Z

With this headers directive however, the following is possible but slightly confusing:
headers = latency_tokens, off, server_tokens
Here, we end up in a situation where "half" of the specified tokens were taken into consideration. This could have (arguably) been considered an acceptable behaviour, but better stay consistent with the stricter semantics of the *_listen directives imho. So the above would set both latency_tokens and server_tokens, and to disable headers, one must do:
headers = off
Frankly, the off flag probably should not be mixed with actual values (that is bad implementation from the listeners directives to accept that imho), but well, not the scope of this PR to address, and better stick to this behaviour for now probably.

I expected that this comment was coming.
I thought about this while writing the code but brushed it off with a corner case that is unlikely to happen. An empty value would suite well as an "off" but IIRC Penlight removes empty strings.

I've updated the code to match the behavior you describe above and added a test for it.

An explicit solution would be to check for a mix of off and other valid values in check_and_infer and throw an error.

This is looking really good, but still a few items to fix before we can merge it.

Let me know if there's anything other than the off value issue that needs to be fixed.

thibaultcha · 2018-04-18T23:33:50Z

kong/conf_loader.lua

+  for _, token in ipairs(conf.headers) do
+    if token ~= "off" then
+      header_tokens_clone[token] = true
+    end


I'm not sure how this behaviour is different from yesterday's version of this patch? It still doesn't seem like it follows the same approach as other array-like directives that accept off?

Oh, I see. If the first element is off then turn it off else don't. My bad.

thibaultcha · 2018-04-18T23:36:08Z

kong/conf_loader.lua

+  -- load headers configuration
+  for _, token in ipairs(conf.headers) do
+    if token ~= "off" then
+      header_tokens_clone[token] = true


I think we should allow for case-insensitive values when specifying headers in this directive (just like HTTP headers are). E.g.:

headers = latency_tokens, Via is same as: headers = latency_tokens, via

As new headers are introduced by Kong's core, instead of introducing a config option for each and every header or set of headers, an array of these values can be now specified using the `headers` config option. This offers a fine grained controlled and does not blow up the number of configuration options. The goal here is to move towards a simpler and easier to understand configuration, similar to 1b9976f (#3147).

hbagdi · 2018-04-20T03:54:57Z

@thibaultcha Updated the PR with the requested fixes.

As new headers are introduced by Kong's core, instead of introducing a config option for each and every header or set of headers, an array of these values can be now specified using the `headers` config option. This offers a fine grained controlled and does not blow up the number of configuration options. The goal here is to move towards a simpler and easier to understand configuration, similar to 1b9976f (#3147). From #3300 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha · 2018-04-20T05:54:30Z

Merged! Thank you!

This fixes a bug introduced in 976dd87 (#3300) where headers config option was being overwritten as a table. The table is not persisted in the intermediate config file and hence the config option does not take effect.

This fixes a bug introduced in 976dd87 (#3300) where headers config option was being overwritten as a table. The table is not persisted in the intermediate config file and hence the config option does not take effect. This commit also exposes `get_running_conf` helper function to read conf from prefix directory in use during the current test.

This fixes a bug introduced in 976dd87 (#3300) where headers config option was being overwritten as a table. The table is not persisted in the intermediate config file and hence the config option does not take effect.

This fixes a bug introduced in 976dd87 (#3300) where the `headers` config option were not properly written to `.kong_env`. This would be an issue when the value was specified from the configuration file (vs. via environment variables). From #3419 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

With CE 0.14.0, a new `headers` configuration option is introduced and `server_tokens` and `latency_tokens` are removed. See Kong/kong#3300 Signed-off-by: Harry Bagdi <harrybagdi@gmail.com>

With CE 0.14.0, a new `headers` configuration option is introduced and `server_tokens` and `latency_tokens` are removed. See Kong/kong#3300

With CE 0.14.0, a new `headers` configuration option is introduced and `server_tokens` and `latency_tokens` are removed. See Kong/kong#3300 From #729

hbagdi added the pr/wip A work in progress PR opened to receive feedback label Mar 15, 2018

thibaultcha reviewed Mar 17, 2018

View reviewed changes

hbagdi force-pushed the feat/tokens-config branch from a8d94f4 to f5890bf Compare March 19, 2018 20:05

hbagdi commented Mar 19, 2018

View reviewed changes

hbagdi added pr/please review and removed pr/wip A work in progress PR opened to receive feedback labels Mar 19, 2018

hbagdi commented Mar 19, 2018

View reviewed changes

hbagdi force-pushed the feat/tokens-config branch from 3594cc5 to 47a8024 Compare March 20, 2018 02:46

hbagdi changed the title ~~WIP: feat(config) flexible tokens/headers configuration~~ feat(config) flexible tokens/headers configuration Mar 20, 2018

hbagdi force-pushed the feat/tokens-config branch from 47a8024 to 2d5437a Compare March 20, 2018 02:55

thibaultcha mentioned this pull request Mar 20, 2018

feat(core) add X-Kong-Upstream-Status header #3263

Closed

thibaultcha requested changes Apr 11, 2018

View reviewed changes

hbagdi force-pushed the feat/tokens-config branch 2 times, most recently from 0dbec50 to 37713c6 Compare April 13, 2018 14:32

thibaultcha reviewed Apr 18, 2018

View reviewed changes

hbagdi force-pushed the feat/tokens-config branch from 37713c6 to 4e34d1c Compare April 18, 2018 16:00

thibaultcha reviewed Apr 18, 2018

View reviewed changes

hbagdi force-pushed the feat/tokens-config branch 5 times, most recently from 9fc3ccf to 5bcc063 Compare April 19, 2018 15:37

hbagdi force-pushed the feat/tokens-config branch from 5bcc063 to 5afad1e Compare April 20, 2018 03:03

thibaultcha closed this Apr 20, 2018

thibaultcha deleted the feat/tokens-config branch April 20, 2018 05:54

hbagdi mentioned this pull request Apr 26, 2018

hotfix(config) parse headers config option from .kong_env #3419

Closed

hbagdi mentioned this pull request May 29, 2018

docs(configuration) document headers field Kong/docs.konghq.com#729

Closed

hbagdi added a commit to Kong/docs.konghq.com that referenced this pull request Jun 22, 2018

docs(configuration) document headers field

7206c2e

With CE 0.14.0, a new `headers` configuration option is introduced and `server_tokens` and `latency_tokens` are removed. See Kong/kong#3300

feat(config) flexible tokens/headers configuration #3300

feat(config) flexible tokens/headers configuration #3300

Conversation

hbagdi commented Mar 15, 2018 • edited Loading

hbagdi commented Mar 15, 2018

thibaultcha commented Mar 17, 2018

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

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

hbagdi commented Apr 3, 2018

thibaultcha left a 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

hbagdi commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Apr 20, 2018

thibaultcha commented Apr 20, 2018

hbagdi commented Mar 15, 2018 •

edited

Loading