feat(config) add control- and data-plane configuration #3147

Tieske · 2018-01-12T21:39:03Z

Adds option proxy=on/off to en/disable the Kong http proxy port. As well as admin=on/off to en/disable the http access to the admin port. Complemented with the existing ssl and admin_ssl options this allows complete control to create data and control planes.

thibaultcha · 2018-01-12T21:45:32Z

kong.conf.default

@@ -110,6 +110,11 @@
                                 # `m`, with a minimum recommended value of
                                 # a few MBs.

+#proxy = on                      # Determines if Nginx should be listening for


If this is off, then ssl = on should probably have no impact whether the proxy server is created or not. In other words, this proxy setting should be considered top-level, and take precedence over both proxy_listen and admin_listen. Ditto for admin. Thoughts?

Yes, I expected that comment, this was to be discussed. The issue is with the naming, because it is called proxy which tends to lead to believe the entire proxy part is disabled, where it only disables the http port now.

What I implemented was basically 4 settings (2 existing, 2 new), controlling all 4 ports: proxy, proxy-ssl, admin and admin-ssl. This allows for maximum flexibility. eg. a configuration with only proxy-ssl enabled would not be possible if proxy were top-level as you suggested.

So this current implementation is not clear. Imo we have 4 options:

Find a better name for proxy (considering that ssl is its counterpart), and admin (considering admin_ssl is that ones counterpart)

Provide less flexibility, where admin and proxy are both "top-level" and control both their respective http and https ports

rename all 4 parameters, such that the names make sense. But this would probably be a breaking change when the existing 2 are renamed.

over-the-top: do 3, but also add 2 'top-level' ones.

I like the flexibility, so I'd go with 1 or 3.

I have given this quite a lot of thought in the past already, and my belief is that some configuration values are cases of "over-configuration", and could be factored together. Some other configuration properties could be set to sane hard-coded values too.

Something like:

proxy_listen = off | 127.0.0.1:80 | 127.0.0.1:443 ssl

(the values are comma separated).

This way:

# control plane proxy_listen = off # data plane proxy_listen = 127.0.0.1:80, 127.0.0.1:443 ssl admin_listen = off # https only, both planes proxy_listen = 127.0.0.1:443 ssl admin_listen = 127.0.0.1:8443 ssl

That would introduce a completely new way of setting options, more complex. Try explaining that in the template conf file, in 2 lines of text. Not saying we shouldn't, just that it might turn into a can of worms.

Also might give the impression that you could do:

proxy_listen = 127.0.0.1:80, 127.0.0.1:81, 127.0.0.1:443 ssl, 127.0.0.1:444 ssl

I think of it as a much more flexible way to configure Kong (and simpler too). Yes, this does mean we can listen on several interfaces/ports (awesome). This means one can now do:

proxy_listen = 127.0.0.1:80, [::1]:80

What is there not to want about this, really? (this is rhetorical :D)

That would introduce a completely new way of setting options, more complex.

Let's be honest: we need new ways of doing a lot of things that we have been (wrongly) assuming for so long now... Time to break away from a few hastily designed patterns and move forward.

and could be factored together

A hundred +1.

Tieske · 2018-01-22T20:33:42Z

The format now is:

proxy_listen = [off] | <ip>:<port> [ssl] [http2], [... next entry ...]
admin_listen = [off] | <ip>:<port> [ssl] [http2], [... next entry ...]

so:

multiple address+port combo's,
comma separated
with optional flags ssl and http2
can add as many entries as you want
silently allows IPv6 addresses

thibaultcha

This is definitely moving in the right direction, but I think we need to make extra efforts for it. I also believe I spotted a few important bugs that should be fixed (unrelated to the user-friendliness aspect of the other comments).

thibaultcha · 2018-01-23T05:24:57Z

kong.conf.default

+#proxy_listen = 0.0.0.0:8000, 0.0.0.0:8443 ssl
+                         # Address and port on which Kong will accept HTTP
+                         # requests, comma separated. The `ssl` suffix sets
+                         # it to accept HTTPS, and `http2` accepts HTTP2.


(aside: we should refer to it as HTTP/2)

thibaultcha · 2018-01-23T05:27:36Z

kong.conf.default

-                                 # Kong, to which your consumers will make
-                                 # requests.
-# Note: See http://nginx.org/en/docs/http/ngx_http_core_module.html#listen for
-# a description of the accepted formats for this and other *_listen values.


This should still be included; eventually, we should also mention that proxy_listen results in various NGINX listen directives, so that users can have a deeper understanding of the rules that apply with this property's uses or format.

thibaultcha · 2018-01-23T05:39:49Z

kong.conf.default

+                         # it to accept HTTPS, and `http2` accepts HTTP2.
+                         # This is the public-facing entrypoint of Kong, to
+                         # which your consumers will make requests.
+                         # If set to `off` the proxy will be disabled.


Trying to be more descriptive and refer to concepts our documentation mentions:

Comma-separated list of addresses and ports on which the proxy server should listen to. The proxy server is the public entrypoint of Kong, which proxies traffic from your consumers. The `ssl` suffix requires that all connections through this port be made through SSL, and `http2` allows for HTTP/2 connections. The values accept a variety of options inherited from the NGINX `location` directive[1]. Examples: localhost:8000 127.0.0.1:8000 *:80 http2 0.0.0.0:443 ssl [1]: http://nginx.org/en/docs/http/ngx_http_core_module.html#listen

thibaultcha · 2018-01-23T05:42:02Z

kong/cmd/utils/prefix_handler.lua

@@ -202,7 +202,8 @@ local function prepare_prefix(kong_config, nginx_custom_template_path)
  end

  -- generate default SSL certs if needed
-  if kong_config.ssl and not kong_config.ssl_cert and not kong_config.ssl_cert_key then
+  if table.concat(kong_config.proxy_listen, ","):find("ssl") and


For the sake of writing reference code, better use :find("ssl", nil, true) when patterns are not necessary (things like this tend to spread and sometimes trigger bugs, as it happened in the test suite in the past; better spreading knowledge of the rarely used arguments of find() instead)

Another, more important note: This will also trigger certificate generation if the values look like so:

proxy_listen = ssl-cert-service.com:80

If we move the "at least one listen directive has SSL enabled" logic away from the template (as pointed in another comment) and into the Lua-land, we can avoid such surprises.

thibaultcha · 2018-01-23T05:46:27Z

kong/conf_loader.lua

+-- @return list of parsed entries, each entry having fields `ip` (normalized string),
+-- `port` (number), `ssl` (bool), `http2` (bool), `listener` (string, full listener).
+local function parse_listeners(values)
+  local flags = { "ssl", "http2" }


The NGINX location directive supports many other flags - it's too bad that we have to limit them like so. For instance, we could furthermore simplify configuration by supporting the proxy_protocol flag of listen, instead of making users rely on real_ip_header = proxy_protocol. We could also support the spdy or ipv6only options out of the box... I do get the benefit of extracting some flags (like the ssl one), but we are limiting ourselves.

Since this is a tradeoff between usability/flexibility (and since we don't run nginx -t before start, not parse this output for error reporting), maybe we ought to simply hard-coding the values we support, and ensuring they are valid as per NGINX's documentation?

@thibaultcha Not sure I'm following you. I specifically created a flexible list such that we can quickly add other flags. Yet you seem to indicate something else?

Unfortunately some of these location directive flags accept values, and your patch does not seem to support them. There are many of them: https://nginx.org/en/docs/http/ngx_http_core_module.html#listen

ah, yes, now I get it. I'll look into it.

thibaultcha · 2018-01-23T05:54:51Z

spec/02-integration/05-proxy/00-proxy_spec.lua

+  local _, count = file:gsub("[%\n%s]+server%s{","")
+  return count
+end
+


style: this file somewhat follows the 2 line jump rule, but not here

thibaultcha · 2018-01-23T05:57:23Z

spec/01-unit/003-prefix_handler_spec.lua

@@ -58,7 +58,7 @@ describe("NGINX conf compiler", function()
    it("compiles the Kong NGINX conf chunk", function()
      local kong_nginx_conf = prefix_handler.compile_kong_conf(helpers.test_conf)
      assert.matches("lua_package_path './?.lua;./?/init.lua;;;'", kong_nginx_conf, nil, true)
-      assert.matches("listen 0.0.0.0:9000;", kong_nginx_conf, nil, true)
+      assert.matches("listen 0.0.0.0:9000 ;", kong_nginx_conf, nil, true)


(this could be avoided if we manage to embed the proxy_protocol flag into the listen directive - it goes without saying that this also gives greater granularity as to which listening entrypoints enable/disable the PROXY protocol)

thibaultcha · 2018-01-23T05:58:09Z

kong/conf_loader.lua

+    mt = { __tostring = function() return "" end }
+
+    conf.proxy_listeners, err = parse_listeners(conf.proxy_listen)
+    if err then return nil, "proxy_listen " .. err end


style: we should really avoid such 1 liner conditions nowadays

thibaultcha · 2018-01-23T06:01:37Z

kong/conf_loader.lua

+                table.concat(flags, "] [") .. "], [... next entry ...]"
+
+  local list = {}
+  if values[1]:match("^%s*off") then


This would match a value such as offshore.com:80, and disable proxying, going against the user's intent.

thibaultcha · 2018-01-23T06:06:02Z

kong/templates/nginx_kong.lua

@@ -63,7 +64,10 @@ upstream kong_upstream {

 server {
    server_name kong;
-    listen ${{PROXY_LISTEN}}${{PROXY_PROTOCOL}};
+> for i = 1, #proxy_listeners do
+>   if proxy_listeners[i].ssl then _ssl_in_use = true end


This logic should belong to our Lua logic prefix handling instead (passed when we compute the template), so that we keep a better separation of concern between our logic and this template, we should have no logic of its own.

Tieske · 2018-01-23T12:29:29Z

Updated again, the format now is:

xxxx_listen = [off] | <ip>:<port> [ssl] [http2] [proxy_protocol], [... next entry ...]

so:

multiple address/name+port combo's,
comma separated
with optional flags ssl, http2, and proxy_protocol
off will disable the listener all together
can add as many entries as you want
silently allows IPv6 addresses

Not implemented from the review comments is extending the flags beyond ssl, http2, and proxy_protocol. Since it would be out of scope for this PR to extend the configuration parser beyond what's required for data-plane and control-plane mode.

thibaultcha

Sorry, but still seeing a few issues with this approach. We need a few more tests around all this, especially considering the added flexibility and the new validation rules we are introducing...

thibaultcha · 2018-01-23T23:51:13Z

kong/conf_loader.lua

+    conf.proxy_ssl_enabled = false
+    for _, listener in ipairs(conf.proxy_listeners) do
+      if listener.ssl == true then
+        conf.proxy_ssl_enabled = true


Much saner 👍

thibaultcha · 2018-01-24T00:10:21Z

kong/conf_loader.lua

+      -- It's an IPv4 or IPv6, just normalize it
+      ip = utils.normalize_ip(remainder)
+    end
+    if (not ip) or (not ip.port) then


(The parenthesis are truly extraneous here)

thibaultcha · 2018-01-24T00:15:39Z

kong/cmd/utils/prefix_handler.lua

@@ -202,7 +201,10 @@ local function prepare_prefix(kong_config, nginx_custom_template_path)
  end

  -- generate default SSL certs if needed
-  if kong_config.ssl and not kong_config.ssl_cert and not kong_config.ssl_cert_key then
+  local has_ssl = (table.concat(kong_config.proxy_listen, ",") .. " "):find("%sssl%s")


Looks like we can (and should) use kong_config.proxy_ssl_enabled here?

thibaultcha · 2018-01-24T00:16:23Z

kong/cmd/utils/prefix_handler.lua

@@ -211,7 +213,8 @@ local function prepare_prefix(kong_config, nginx_custom_template_path)
    kong_config.ssl_cert = kong_config.ssl_cert_default
    kong_config.ssl_cert_key = kong_config.ssl_cert_key_default
  end
-  if kong_config.admin_ssl and not kong_config.admin_ssl_cert and not kong_config.admin_ssl_cert_key then
+  if has_ssl and not kong_config.admin_ssl_cert and


And here, we are using has_ssl which is computed over the proxy_listen values, not the Admin listen ones. We should probably use kong_config.admin_ssl_enabled anyway.

There really should be tests covering this...

thibaultcha · 2018-01-24T00:17:07Z

kong/conf_loader.lua

@@ -230,7 +224,7 @@ local function check_and_infer(conf)
    end
  end

-  if conf.ssl then
+  if table.concat(conf.proxy_listen, ","):find("ssl") then


This approach still has the issue described yesterday wrt. specifying a domain name that contains "ssl".

Alternatively, to avoid duplicated logic and reduce cognitive load, we could move this validation logic down below, once the listeners are already parsed. After all, there is no need for this validation logic to be part of the inference function it currently belongs to...

There should be unit tests for this - it is self-contained and easy to test.

the reordering would mean quite a reshuffle of code. So I didn't, this is the lesser of the evils imo. Updated the pattern to prevent false positives (and added tests), but otherwise left as is.

Yeah, I can understand that. It wouldn't be too hard to do but that's fine.

thibaultcha · 2018-01-24T00:17:30Z

kong/conf_loader.lua

@@ -260,7 +254,7 @@ local function check_and_infer(conf)
    end
  end

-  if conf.admin_ssl then
+  if table.concat(conf.admin_listen, ","):find("ssl") then


ditto: still has the same issue but could be moved to a better place anyway

thibaultcha · 2018-01-24T00:20:54Z

spec/01-unit/002-conf_loader_spec.lua

+          admin_listen = "off",
+        }))
+    assert.same({}, conf.proxy_listeners)
+    assert.same({}, conf.admin_listeners)


Can we also add a test for the issue described yesterday with values such as offshore.com:80 and ensuring they do not disable the proxy? I find that we lack testing in corner cases and invalid user input in this PR

Tieske · 2018-01-24T00:50:01Z

@thibaultcha Whoops... seems I fixed some issues but not all occurrences of those. Sorry about that and making you go through all that again (and thx for catching them)

thibaultcha

Alright, this looks bug-free for now! Lots of stuff I don't like (sorry :(, e.g. a get_proxy_ip() but no get_admin_ip() (and different implementations for their retrieval, one with == true/false, the other == ssl, and other such minor details...) but I think that's fine...

thibaultcha · 2018-01-26T02:34:25Z

spec/01-unit/002-conf_loader_spec.lua

+    }))
+    assert.same({}, conf.proxy_listeners)
+    assert.same({}, conf.admin_listeners)
+    -- not off with names containing 'off'


I don't see the same test for a hostname containing 'ssl' (with a key and no cert defined, so it returns the appropriate configuration error) (for both admin and proxy interfaces). Shall we add it?

subnetmarco · 2018-02-01T04:53:02Z

@Tieske any updates?

Tieske · 2018-02-02T11:25:13Z

@thibaultcha

extra test added.

Lots of stuff I don't like (sorry :(, e.g. a get_proxy_ip() but no get_admin_ip() (and different implementations for their retrieval, one with == true/false, the other == ssl, and other such minor details...) but I think that's fine...

Hmmmm.... I can add those if you want, but what's the use? there are several tests using get_proxy_ip() and get_proxy_port() but none is using this for the admin listener? So I don't see the point of adding functions we do not use?

Also the comparisons are always against a boolean, since ssl is always a boolean, so they are the same. The implicit nil is not allowed in the config so we should make these test as explicit as possible imo hence the == true/false and not if entry.ssl then ....

thibaultcha · 2018-02-02T18:30:48Z

All of what you said is true, but my argument was rather in favor of readability, consistency and reducing cognitive load. Code is for humans, not machines. That's fine.

Tieske · 2018-02-08T00:06:12Z

@thibaultcha I made the implementations symmetrical for proxy/admin helpers. Please review and merge if ok.

thibaultcha · 2018-02-08T01:01:12Z

spec/fixtures/custom_nginx.template


        access_log logs/admin_access.log;

        client_max_body_size 10m;
        client_body_buffer_size 10m;

-> if admin_ssl then
-        listen ${{ADMIN_LISTEN_SSL}} ssl${{ADMIN_HTTP2}};
+> if admin_ssl_enabled then


FYI, this file does not match the actual template (will fix during merge)

Updates to the `proxy_listen` and `admin_listen` configuration properties allow users to configure Kong in more flexible ways, potentially listening on multiple interfaces/ports at once, with granular options, or not at all. Disabling proxy and/or admin interfaces allows a Kong node to run in "proxy only" or "admin only" modes, opening the way for "data-plane" and "control-plane" modes. Overall, the new format is closer to that of the NGINX `listen` directive for compatibility and flexibility reasons. From #3147 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha · 2018-02-08T02:24:04Z

Merged to next with some cleanup modifications, improved commit message and additional commit to the description of several proeprties from kong.conf. Awesome!

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

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

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>

Tieske added the pr/status/please review label Jan 12, 2018

Tieske self-assigned this Jan 12, 2018

thibaultcha reviewed Jan 12, 2018

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jan 20, 2018

Tieske force-pushed the planes branch from 58d9f94 to 363fe50 Compare January 22, 2018 20:37

Tieske added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jan 22, 2018

thibaultcha requested changes Jan 23, 2018

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jan 23, 2018

Tieske force-pushed the planes branch 3 times, most recently from baac61f to e3a9602 Compare January 23, 2018 12:18

Tieske added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jan 23, 2018

thibaultcha requested changes Jan 24, 2018

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Jan 24, 2018

Tieske added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jan 25, 2018

thibaultcha reviewed Jan 26, 2018

View reviewed changes

Tieske force-pushed the planes branch from 96c0d91 to 34c1e55 Compare February 7, 2018 23:09

thibaultcha reviewed Feb 8, 2018

View reviewed changes

thibaultcha force-pushed the planes branch from 34c1e55 to a6048fa Compare February 8, 2018 01:57

thibaultcha closed this Feb 8, 2018

thibaultcha deleted the planes branch February 8, 2018 02:24

thibaultcha mentioned this pull request Mar 13, 2018

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

Closed

hbagdi mentioned this pull request Mar 20, 2018

feat(config) flexible tokens/headers configuration #3300

Closed

feat(config) add control- and data-plane configuration #3147

feat(config) add control- and data-plane configuration #3147

Conversation

Tieske commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha Jan 12, 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

Tieske commented Jan 22, 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

thibaultcha Jan 23, 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

Tieske commented Jan 23, 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

Choose a reason for hiding this comment

thibaultcha Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tieske commented Jan 24, 2018

thibaultcha left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subnetmarco commented Feb 1, 2018

Tieske commented Feb 2, 2018

thibaultcha commented Feb 2, 2018

Tieske commented Feb 8, 2018

Choose a reason for hiding this comment

thibaultcha commented Feb 8, 2018

thibaultcha Jan 12, 2018 •

edited

Loading

thibaultcha Jan 23, 2018 •

edited

Loading

thibaultcha Jan 26, 2018 •

edited

Loading

thibaultcha left a comment •

edited

Loading