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

Add support for access-log-path and error-log-path #1243

Merged
merged 1 commit into from
Aug 25, 2017

Conversation

maxlaverse
Copy link
Contributor

@maxlaverse maxlaverse commented Aug 25, 2017

By default, the access logs of the Nginx Ingress go to /var/log/nginx/access.log which is a synlink to /dev/stdout. We are using a syslog server to store all our access logs which leaves us with two options:

  • parsing the access logs out of the container stdout
  • modifying the template to specify a different destination (our syslog server)

For performance reason and simplicity we choose to directly send our logs from Nginx to a syslog service. This is working fine by modifying the template.

However, since it might interest other people to have the log destination customizable, a cleaner solution would be to make it configurable in the ConfigMap and that's what this PR is about.

This pull-requests adds support for a access-log-path field in the Nginx ConfigMap to set a different path than the default one (/var/log/nginx/access.log). The value is put as is in the Nginx template, therefore also allowing to configure a syslog server as log destination.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 25, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 25, 2017
@maxlaverse maxlaverse force-pushed the custom_access_log_path branch 2 times, most recently from badaaec to 9049b93 Compare August 25, 2017 08:15
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 25, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 44.321% when pulling 9049b93 on maxlaverse:custom_access_log_path into e7d2ff6 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 44.321% when pulling 9049b93 on maxlaverse:custom_access_log_path into e7d2ff6 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 44.321% when pulling 39de6bd on maxlaverse:custom_access_log_path into e7d2ff6 on kubernetes:master.

@@ -118,7 +118,7 @@ http {
{{ if $cfg.DisableAccessLog }}
access_log off;
{{ else }}
access_log /var/log/nginx/access.log upstreaminfo if=$loggable;
access_log {{ $cfg.AccessLogPath }} upstreaminfo if=$loggable;
{{ end }}
error_log /var/log/nginx/error.log {{ $cfg.ErrorLogLevel }};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not also add a variable for error_log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made it configurable in the PR since I hadn't any reason not to add it

@aledbf
Copy link
Member

aledbf commented Aug 25, 2017

@maxlaverse please squash the commits

@maxlaverse maxlaverse changed the title Add support for access-log-path Add support for access-log-path and error-log-path Aug 25, 2017
@maxlaverse maxlaverse force-pushed the custom_access_log_path branch 2 times, most recently from 70f72de to 6310814 Compare August 25, 2017 12:02
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 44.328% when pulling dd00b6d on maxlaverse:custom_access_log_path into e7d2ff6 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 44.303% when pulling dd00b6d on maxlaverse:custom_access_log_path into e7d2ff6 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 44.341% when pulling dd00b6d on maxlaverse:custom_access_log_path into e7d2ff6 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 44.328% when pulling dd00b6d on maxlaverse:custom_access_log_path into e7d2ff6 on kubernetes:master.

@aledbf aledbf self-assigned this Aug 25, 2017
@aledbf
Copy link
Member

aledbf commented Aug 25, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 25, 2017
@aledbf
Copy link
Member

aledbf commented Aug 25, 2017

@maxlaverse thanks!

@aledbf aledbf merged commit 3499524 into kubernetes:master Aug 25, 2017
@maxlaverse maxlaverse deleted the custom_access_log_path branch August 25, 2017 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants