feat(conf) add `plugins` config option to kong.conf #3387

hbagdi · 2018-04-10T16:44:34Z

Problem:
All plugins bundled with Kong are loaded
by default. There exists no mechanism
to disable any of the bundled plugins.

Solution:
Introduce a config option which
controls which of the bundled plugins
are loaded into Kong.

Possible implementations:
A) Introduce `plugins` config option and
remove `custom_plugins` option. Kong will
load only plugins which are specified in this
config options. This simplifies configurations.

B) Introduce a new config option `default_plugins`.
This is separate from `custom_plugins`.
The reason to keep it different is keeping a
simpler end user experience.
Any user in the wild today
probably sets the custom_plugins field or
KONG_CUSTOM_PLUGINS environment variable.
If a user were to introduce a custom plugin,
she would have to concat all the default plugins
and then her plugin as well.
But, at the same time, this heterogeneous
approach makes the end-user feel that Kong
differentiates between default and
custom plugins.

Approach (B) is chosen for this patch.

thibaultcha · 2018-04-10T17:22:16Z

kong.conf.default

@@ -59,6 +59,12 @@
                                          # adjusted by the `log_level`
                                          # directive.

+#default_plugins = acl, aws-lambda, basic-auth, bot-detection, correlation-id, cors, datadog, file-log, hmac-auth, http-log, ip-restriction, jwt, key-auth, ldap-auth, loggly, oauth2, rate-limiting, request-size-limiting, request-termination, response-ratelimiting, request-transformer, response-transformer, runscope, statsd, syslog, tcp-log, udp-log


We need to find a way to respect the 80 chars limit applied everywhere in this file - have you tried to, and if so, what seems to be needed to allow for it?

I did not try to find a solution before for this.
However, the following patch seems to be working fine:

diff --git a/kong.conf.default b/kong.conf.default index 48658cb6..3a6dcf79 100644 --- a/kong.conf.default +++ b/kong.conf.default @@ -59,7 +59,13 @@ # adjusted by the `log_level` # directive. -#default_plugins = acl, aws-lambda, basic-auth, bot-detection, correlation-id, cors, datadog, file-log, hmac-auth, http-log, ip-restriction, jwt, key-auth, ldap-auth, loggly, oauth2, rate-limiting, request-size-limiting, request-termination, response-ratelimiting, request-transformer, response-transformer, runscope, statsd, syslog, tcp-log, udp-log +default_plugins = acl, aws-lambda, basic-auth, bot-detection, \ + correlation-id, cors, datadog, file-log, hmac-auth, \ + http-log, ip-restriction, jwt, key-auth, ldap-auth, \ + loggly, oauth2, rate-limiting, request-size-limiting, \ + request-termination, response-ratelimiting, \ + request-transformer, response-transformer, \ + runscope, statsd, syslog, tcp-log, udp-log

Note to self: add test for this

https://stevedonovan.github.io/Penlight/api/manual/06-data.md.html#Reading_Configuration_Files

hbagdi · 2018-04-10T17:41:41Z

I realized that this has to be opened against next and not master.
I'll cherry-pick this commit on next and update the PR.

thibaultcha

LGTM, except for the added logging - left a comment, please object if I missed something about this logging statement, otherwise once removed, I will slightly extend the description in kong.conf.default and proceed with the merge!

thibaultcha · 2018-04-18T03:54:38Z

kong/conf_loader.lua

    for i = 1, #conf.custom_plugins do
      local plugin_name = pl_stringx.strip(conf.custom_plugins[i])
-      custom_plugins[plugin_name] = true
+      plugin_list[plugin_name] = true
+      log.verbose("custom plugins from conf file: %s", plugin_name)


Logging those seems superfluous. When using kong start --vv, the CLI will print both of these configuration values already, past the merging between config and environment variables (here we also hard-code those have been found in the config file, which might not be true). This would also be very verbose tbh. Logging within loops should rather be done with the debug level only (but, as said before, such logging statements are already in place).

Additionally, when starting with log_level = debug, Kong will also print (from the nginx logs) all of the enabled plugins.

I agree with you on this, I've dropped the log statements in the updated commit.

thibaultcha · 2018-04-18T04:07:34Z

I realized that this has to be opened against next and not master.

Mhhhh why so? It seems to me like this change would be backwards-compatible? Option A would have been breaking, but B, chosen here, isn't, as far as I can tell.

hbagdi · 2018-04-18T15:25:47Z

Mhhhh why so? It seems to me like this change would be backwards-compatible? Option A would have been breaking, but B, chosen here, isn't, as far as I can tell.
I incorrectly categorized kong.conf file changes as breaking change even though it is backward-compatible. Also, at the time of opening the initial PR, I was unsure which route we will take forward.

I've removed the logging statements.

One thing that I noticed but haven't updated for the sake of simplicity is the following error line returned when a user tries to configure a plugin which is not present in conf.

$ http post :8001/plugins name=foo-bar
HTTP/1.1 400 Bad Request
{
    "config": "plugin 'foo-bar' not enabled; add it to the 'custom_plugins' configuration property"
}

The default_plugins would work even if they are added to the custom_plugins since all plugins are the same to Kong.
Another unrelated enhancement could be to return not enabled when the plugin module is found but not enabled in config and return not found when the plugin module is not present on the filesystem.

I digress, let's .

hbagdi · 2018-04-27T23:36:59Z

@thibaultcha ping

thibaultcha · 2018-04-30T22:55:52Z

One thought I had: if the user chooses to disable all plugins, can they do so? It doesn't seem like it to me, since a default value is given to default_plugins, and we do not distinguish an unspecified configuration value (commented out or not in file), vs a specified but empty value (to signify "no plugins"), or even NONE which is reserved for defaults and undocumented.
This PR isn't at fault here, but I am wondering if users would be thrown off by this behaviour...

hbagdi · 2018-05-01T01:59:00Z

@thibaultcha I see your point. If we make plugins configurable then we sure should provide a way to disable all plugins and implicitly a user would assume an empty default_plugins would disable all plugins.

This could be achieved with a placeholder value and then parse it like we do with other options.
Although off doesn't sound correct here, it would make sense to stick to one thing as long as it doesn't alter the meaning. IMHO off is not bad and can convey the meaning clearly.
Thoughts?

hbagdi · 2018-05-10T20:48:24Z

spec/kong_tests.conf

@@ -25,3 +25,4 @@ nginx_optimizations = off

 prefix = servroot
 log_level = debug
+lua_package_path=./spec/fixtures/custom_plugins/?.lua


This was done to fix a failing test case in spec/02-integration/02-cmd/02-start_stop_spec.lua:157.

hbagdi · 2018-05-11T21:35:51Z

This PR deprecates a custom_plugin config option and hence should be merged into next.

thibaultcha · 2018-05-11T23:08:46Z

spec/02-integration/04-admin_api/24-plugins-conf.lua

+                   "in kong.conf, which will be removed in a future release. " ..
+                   "Please use 'plugins' option to specify your " ..
+                   "custom plugins.", stderr, nil, true)
+  end)


This test needs to go in the CLI suite, instead of this Admin API test suite

Problem: All plugins bundled with Kong are loaded by default. There exists no mechanism to disable any of the bundled plugins. Solution: Introduce a config option which controls which of the bundled plugins are loaded into Kong. Implementation: Introduce `plugins` config option and deprecate `custom_plugins` option. Kong will load only plugins which are specified in `plugins` config options. This also is inline in the goal of simplifying Kong's configurations. Plugins specified in `plugins` and `custom_plugins` are merged together to avoid a break change but `custom_plugins` will be removed in future. The new `plugins` options can take in a sugar value `bundled` which loads all the plugins bundled with Kong. This accomplishes good usability and flexibility.

kikito · 2018-06-07T13:23:17Z

spec/02-integration/04-admin_api/24-plugins-conf.lua

+    end)
+  end)
+
+  describe("with 'plugins=off, key-auth'", function()


I don't think we should support this case. off should not be combinable with other plugin names. If you want to only have key-auth, the correct way of doing so should be plugins=key-auth.

I suggest treating off + anything else as an error.

This was done to keep it consistent with parse_listeners which have the same behavior.

Could this be covered in a future commit which changes the behavior for all settings or changing this should be in the scope of this PR?

@kikito @hbagdi and I agreed beforehand that it'd be better to stay consistent with other existing directives for now. Revisiting the behaviour in another PR is reasonable.

thibaultcha · 2018-06-08T03:43:07Z

So far, this looks good to me now. However it will need manual merging for a bunch of minor stuff. Good job @hbagdi!

thibaultcha · 2018-06-08T03:44:08Z

Also, giving it some time if any further reviews come in, or a chance for me to revisit in a day or two.

hishamhm · 2018-06-13T04:12:01Z

LGTM, but I suggest renaming bundled to default — given that in the future plugins will no longer be bundled in the Kong repo (and some already aren't), this terminology may become confusing: plugins will continue to be "bundled" in Kong binary packages, but won't be "bundled" in the Kong code as they appear as dependencies. Once a user installs a custom plugin as a rock, it appears indistinguishable in luarocks list from to the ones such as azure and zipkin (unlike the other ones which are still part of the kong rock and thus appear more tightly "bundled").

The notion that Kong holds an internal default list of plugins will be easier to explain in the documentation, without having to explain what we mean by "bundled".

thibaultcha · 2018-06-13T16:49:12Z

@hishamhm Well, we actually chose bundled on purpose instead of default. The rationale is that plugins are bundled in the distribution packages (used in the vast majority of installations), and will continue to be so even when all plugins are out of the core, and when plugins are installable via the CLI. The meaning of bundled is a lot clearer and self-documenting than that of default, especially in a configuration file where all default values are specified as they are (and not = default). Less importantly, but in conclusion, I would argue that in the case of Kong, LuaRocks is an implementation detail and the everyday user should not have to run commands such as luarocks list. By the time Kong supports installable plugins, a kong plugins list command should be available and distinguish officially bundled plugins from ones that are just installed.

Overall, bundled simply carries more meaning than default, and it it easier to explain that bundled are the ones shipped with Kong, documented on https://konghq.com/plugins/.

Problem ------- All plugins bundled with Kong are loaded by default. There exists no mechanism to disable any of the bundled plugins, which can yield considerable performance improvements in the runloop in specific workloads. Solution -------- Introduce a config option which controls which of the bundled plugins are loaded into Kong. Implementation -------------- Introduce `plugins` config option and deprecate `custom_plugins` option. Kong will load only plugins which are specified in `plugins` config options. This also is inline in the goal of simplifying Kong's configurations. Plugins specified in `plugins` and `custom_plugins` are merged together to avoid a break change but `custom_plugins` will be removed in future. The new `plugins` options can take in a sugar value `bundled` which loads all the plugins bundled with Kong. This accomplishes good usability and flexibility. From #3387

thibaultcha · 2018-06-15T02:15:09Z

Now merged to next, thank you @hbagdi !

hbagdi added the pr/please review label Apr 10, 2018

thibaultcha reviewed Apr 10, 2018

View reviewed changes

hbagdi changed the base branch from master to next April 10, 2018 17:39

hbagdi changed the base branch from next to master April 10, 2018 17:40

hbagdi force-pushed the feat/plugins-kong-conf branch from 21500f5 to 7fa49ec Compare April 10, 2018 17:45

hbagdi changed the base branch from master to next April 10, 2018 17:46

hbagdi force-pushed the feat/plugins-kong-conf branch from 7fa49ec to 9104a28 Compare April 13, 2018 15:41

thibaultcha reviewed Apr 18, 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/please review labels Apr 18, 2018

hbagdi force-pushed the feat/plugins-kong-conf branch from 9104a28 to d25b86b Compare April 18, 2018 15:12

hbagdi changed the base branch from next to master April 18, 2018 15:12

hbagdi changed the title ~~feat(conf) add new plugins config option~~ feat(conf) add new default_plugins config option Apr 18, 2018

thibaultcha added pr/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 Apr 18, 2018

hbagdi force-pushed the feat/plugins-kong-conf branch from db417fa to 85da1dc Compare May 8, 2018 01:57

hbagdi changed the title ~~feat(conf) add new default_plugins config option~~ feat(conf) add plugins config option to kong.conf May 8, 2018

hbagdi force-pushed the feat/plugins-kong-conf branch 5 times, most recently from 5775a7d to 4a5773e Compare May 10, 2018 16:18

hbagdi commented May 10, 2018

View reviewed changes

hbagdi force-pushed the feat/plugins-kong-conf branch from 4a5773e to 9677556 Compare May 11, 2018 21:30

hbagdi force-pushed the feat/plugins-kong-conf branch from 9677556 to f8486c7 Compare May 11, 2018 21:34

hbagdi changed the base branch from master to next May 11, 2018 21:34

hbagdi force-pushed the feat/plugins-kong-conf branch 3 times, most recently from b51c33a to 4db99e9 Compare May 11, 2018 21:51

thibaultcha reviewed May 11, 2018

View reviewed changes

hbagdi added 3 commits May 29, 2018 10:50

[squash] move test into correct place

6aa6ec9

[squash] fix fixture

0bff011

hbagdi force-pushed the feat/plugins-kong-conf branch from ecabe45 to 0bff011 Compare May 29, 2018 17:51

kikito requested changes Jun 7, 2018

View reviewed changes

thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/please review labels Jun 8, 2018

thibaultcha closed this Jun 15, 2018

thibaultcha deleted the feat/plugins-kong-conf branch June 15, 2018 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(conf) add `plugins` config option to kong.conf #3387

feat(conf) add `plugins` config option to kong.conf #3387

hbagdi commented Apr 10, 2018 •

edited

Loading

thibaultcha Apr 10, 2018

hbagdi Apr 10, 2018

hbagdi Apr 13, 2018

hbagdi Apr 13, 2018

hbagdi commented Apr 10, 2018

thibaultcha left a comment

thibaultcha Apr 18, 2018

hbagdi Apr 18, 2018

thibaultcha commented Apr 18, 2018 •

edited

Loading

hbagdi commented Apr 18, 2018

hbagdi commented Apr 27, 2018

thibaultcha commented Apr 30, 2018

hbagdi commented May 1, 2018

hbagdi May 10, 2018

hbagdi commented May 11, 2018

thibaultcha May 11, 2018

kikito Jun 7, 2018

hbagdi Jun 7, 2018

thibaultcha Jun 8, 2018

thibaultcha commented Jun 8, 2018

thibaultcha commented Jun 8, 2018

hishamhm commented Jun 13, 2018

thibaultcha commented Jun 13, 2018

thibaultcha commented Jun 15, 2018

feat(conf) add plugins config option to kong.conf #3387

feat(conf) add plugins config option to kong.conf #3387

Conversation

hbagdi commented Apr 10, 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

hbagdi commented Apr 10, 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

thibaultcha commented Apr 18, 2018 • edited Loading

hbagdi commented Apr 18, 2018

hbagdi commented Apr 27, 2018

thibaultcha commented Apr 30, 2018

hbagdi commented May 1, 2018

Choose a reason for hiding this comment

hbagdi commented May 11, 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

thibaultcha commented Jun 8, 2018

thibaultcha commented Jun 8, 2018

hishamhm commented Jun 13, 2018

thibaultcha commented Jun 13, 2018

thibaultcha commented Jun 15, 2018

feat(conf) add `plugins` config option to kong.conf #3387

feat(conf) add `plugins` config option to kong.conf #3387

hbagdi commented Apr 10, 2018 •

edited

Loading

thibaultcha commented Apr 18, 2018 •

edited

Loading