Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat (config) allow the user the ability to change #3382

Conversation

maguec
Copy link

@maguec maguec commented Apr 6, 2018

Summary

large_client_header_buffers nginx configuration option in kong.conf

Nginx returns "400 Request Header Or Cookie Too Large" when cookies are
too large, so the large_client_header_buffers needs to be configured to
allow large cookie request through Kong.

Full changelog

  • set nginx default in kong_defaults.lua
  • allow overrides in the templating
  • tests to ensure that changes git picked up

Issues resolved

large_client_header_buffers nginx configuration option in kong.conf

Nginx returns "400 Request Header Or Cookie Too Large" when cookies are
too large, so the large_client_header_buffers needs to be configured to
allow large cookie request through Kong.
@thibaultcha
Copy link
Member

Thanks for the suggestion,

Unfortunately, I am afraid this falls under the same category as #3010, #3323, and a few other ones. See this comment/thread for context on our current stance with regards to customizing the nginx directives from the Kong template.

We'd rather introduce a nicer way to solve this issue altogether (otherwise, the list of configuration variables will grow out of control). Our options are:

  1. [community feedback requested] A new way to install and configure Kong #2355 (the cleanest imho)
  2. Hooks into the nginx configuration, such as proposed by add extend points in nginx/kong conf templates #2675
  3. Another generic way of overriding any nginx directive from kong.conf. I have given this some thought as a temporary solution, or compromise until/if we ever get to 1.

@thibaultcha
Copy link
Member

Another approach we have been discussing with @maguec that does not involve managing different configuration files (as per suggestion 2.), and still allows for granular nginx directive override (with both kong.conf and environment variables), is the declaration of "flexible" configuration values in kong.conf (therefore picked up by env variables as well):

There would exist at least 3 prefixes that I can think of for now, in order to inject those directives in various levels of the nginx configuration:

# injected into the http {} block
nginx_directive_http_<directive_name> = <directive value>

# injected into the proxy server {} block
nginx_directive_proxy_<directive_name> = <directive value>

# injected into the admin server {} block
nginx_directive_admin_<directive_name> = <directive value>

Translated to environment variables, those would give:

KONG_NGINX_DIRECTIVE_HTTP<DIRECTIVE_NAME>=<directive value>

This addition would require nginx template changes (as would the proposal 2. above), and some changes in the configuration loader (conf_loader.lua).

@thibaultcha
Copy link
Member

Closing this since some internal decisions and progress has been made on the above proposal. Thanks for the suggestion @maguec

hbagdi added a commit that referenced this pull request Jun 11, 2018
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
thibaultcha pushed a commit that referenced this pull request Jun 13, 2018
Problem
-------

Kong ships with an NGINX template which is rendered when Kong starts.
There exists no mechanisms to add/update arbitrary NGINX directives 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.

Proposed Solution
-----------------

Proposed in #3382:

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 approach as described in #3382, and keeps solutions
similar to the one proposed in #2675 possible (by way of injecting an
`include` directive).

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

* `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
  `lua_shared_dict custom_cache 2k;` directive to `http` block of Kong.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants