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

refactor add LOG_FORMAT option #3274

Closed
wants to merge 3 commits into from
Closed

refactor add LOG_FORMAT option #3274

wants to merge 3 commits into from

Conversation

ymatsiuk
Copy link

@ymatsiuk ymatsiuk commented Mar 2, 2018

Summary

According to this https://getkong.org/docs/0.12.x/configuration/#custom-nginx-configuration-embedding-kong there are two ways of using custom template:

  • tune global nginx configuration
  • inline nginx_kong.lua in global

First method is pretty cool and easy. And it doesn't break in case of change in nginx_kong.lua. So it's a pretty solid way to add functionality. Second method involves more maintenance as every update we have to check what was changed in templates, backport the changes into our custom template and try to re-build image (yes we run docker).

Desired way of defining log format for nginx:

http {
     # custome configuration goes here:
     map $upstream_response_time $temprt {
       default $upstream_response_time;
       ""      0;
     }
     log_format json '{ "@timestamp": "$time_iso8601", '
                        '"remote_addr": "$remote_addr", '
                        '"body_bytes_sent": "$body_bytes_sent", '
                        '"status": $status, '
                        '"request": "$request", '
                        '"url": "$uri", '
                        '"request_method": "$request_method", '
                        '"response_time": $temprt, '
                        '"http_referrer": "$http_referer", '
                        '"http_user_agent": "$http_user_agent" }';
     
     access_log ${{PROXY_ACCESS_LOG}} json;
     # include default Kong Nginx config
     include 'nginx-kong.conf';
}

But in current situation we need to inline whole nginx_kong.lua in order to append access_log... with json in the end.

Full changelog

  • Add LOG_FORMAT option to nginx_kong.lua template
  • Add log_forma option to kong.conf.default

@ymatsiuk ymatsiuk changed the base branch from master to next March 2, 2018 14:25
@ymatsiuk ymatsiuk changed the base branch from next to master March 2, 2018 14:26
@ymatsiuk ymatsiuk changed the title Add LOG_FORMAT option refactor add LOG_FORMAT option Mar 3, 2018
@thibaultcha
Copy link
Member

Hi! Customizing the log format from the configuration file would be handy. However, I'm not sure about the approach followed in this patch.

The combined log format is a predefined one, but to use a custom one, we would need to generate log_format directives in the generated nginx config, and then specify them at the end of the access_log exposed directives, something like this:

log_format = my_format '$remote_addr - [$time_local] - $status'

proxy_access_log = logs/access.log my_format

But the current patch would specify a log_format directly appended to the access_log directive without defining said format first, so I have doubts this even works? In fact, I tried locally (without this patch) and the problem appears quite evidently:

$ KONG_PROXY_ACCESS_LOG="logs/access.log foo" bin/kong restart
Error: ./kong/cmd/start.lua:51: nginx: [emerg] unknown log format "foo" in /usr/local/opt/kong/nginx-kong.conf:46

It is also my belief that we should be able to define multiple logging formats instead of a single one (one might want to use a different format for the proxy or the admin access logs altogether, for example), so we should probably try to support that in the configuration file as well.

@thibaultcha thibaultcha added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Mar 7, 2018
@ymatsiuk
Copy link
Author

ymatsiuk commented Mar 8, 2018

Thanks @thibaultcha for the reply. Not sure if you understood me correctly, so I'll try to add more explanation with simpler example.

My main idea was to have mechanism to add/change log_format.
I assumed that default value for it is combined as per documentation of nginx. So when you create custom template, you can define log_format there and switch between default and defined using KONG_LOG_FORMAT environment variable.

So assuming my custom_nginx.template contains:

http {
     log_format custom_format '{ ... my custom format here ... }';
     # include default Kong Nginx config
     include 'nginx-kong.conf';
}

I should be able to switch between formats like this:

KONG_LOG_FORMAT="custom_format" kong start -c kong.conf --nginx-conf custom_nginx.template
#nginx log should be formatted as specified in custom_nginx.template

Otherwise it should fallback to default which is combined if KONG_LOG_FORMAT is not provided:

kong start -c kong.conf --nginx-conf custom_nginx.template
#nginx log should be formatted with default combined nginx formatter

This allows me to not ever touch nginx_kong.lua and makes my template upgrade proof. So if you guys decided to delete any of lua_* directives or change the whole template in next release, as long as my change mentioned in this PR is in place, I'm safe to just bump the version and build new image with my custom_nginx.template.

@thibaultcha
Copy link
Member

I see, so this solution still requires one to create their own nginx configuration template (as per the default main template) in order to define their custom logging format... I find this a little bit obscure to use. Usually, I would be a fan of a solution that fully plays into the Kong model (abstract away the Nginx directives), or fully plays with the Nginx approach (when made obvious that such customizations would require an Nginx configuration change). But this approach sits in the middle, which I find unintuitive to use...

@thibaultcha thibaultcha added pr/status/do not merge (to discuss) pr/discussion This PR is being debated. Probably just a few details. and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. pr/do not merge labels Mar 13, 2018
@ymatsiuk
Copy link
Author

@thibaultcha I refined a bit the solution.

@zhouzhuojie
Copy link
Contributor

Look forward to this pr! I want to customize the default access log in kong in order to hide basic_auth's username defined in combined log_format.

@thibaultcha
Copy link
Member

@ymatsiuk Sorry, I don't understand what this patch is supposed to do, nor how it differs from the original one. As a user, I'd still have to specify my custom logging format in a custom nginx configuration template, so the configuration of a logging format is still split between the two files... (The kong.conf.default values lack documentation and default values as well).

I think I will close this until we come up with a better integration. For now, this can still be achieved with a custom nginx template.

@thibaultcha thibaultcha added pr/do not merge and removed pr/discussion This PR is being debated. Probably just a few details. labels Apr 9, 2018
@ymatsiuk
Copy link
Author

@thibaultcha with this idea there is no need to create custom temple. All you need is to set few env variables.
Just now I realized that it's more convenient to use 2 environment variables: PROXY_LOG_FORMAT_NAME (by default it's our combined) and PROXY_LOG_FORMAT (this is custom format string)

So in the kong/templates/nginx_kong.lua we do the following:

> if proxy_log_format then
    log_format ${{PROXY_LOG_FORMAT_NAME}} ${{PROXY_LOG_FORMAT}} ;
> end

And you can start kong defining 2 environment variables to have custom format:

export PROXY_LOG_FORMAT_NAME=my_custom_format 
export PROXY_LOG_FORMAT='log_format compression $remote_addr - $remote_user [$time_local] "$request" $status $body_bytes_sent "$http_referer" "$http_user_agent" "$gzip_ratio"'
# once PROXY_LOG_FORMAT is defined, appropriate section is added to nginx.conf using condition in template
kong start

Otherwise kong will fallback to default combined format.
Please let me know your thoughts.

@thibaultcha
Copy link
Member

@ymatsiuk Since this is now doable via #3530, I don't think there is a need for this PR anymore - we followed a more generic and powerful approach, due to the pains experienced in having to maintain a plethora of Nginx directive aliases in our own configuration file.

Nonetheless, thank you for the patch!

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.

3 participants