Ability to separately disable access log in http and stream contexts #5348

Antiarchitect · 2020-04-11T18:43:13Z

Two new configuration options:
disable-http-access-log
disable-stream-access-log

Should resolve issue with enormous amount of TCP 200 useless entries in logs

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Which issue/s this PR fixes

How Has This Been Tested?

No time to test. Changes are trivial. Having millions of useless TCP 200 in my logs and my Elasticsearch is going to explode soon

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

k8s-ci-robot · 2020-04-11T18:43:21Z

Welcome @Antiarchitect!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-04-11T18:43:21Z

Hi @Antiarchitect. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

aledbf · 2020-04-12T14:58:24Z

I prefer to find out the reason of that output than disabling logs completely.
What are you doing and what version are you using? Please post a complete line of the output you see

Antiarchitect · 2020-04-12T15:16:52Z

@aledbf I'm using stable/nginx-ingress helm chart 1.34.3 which is using quay.io/kubernetes-ingress-controller/nginx-ingress-controller:0.30.0 internally.
I'm using ingress-nginx feature https://kubernetes.github.io/ingress-nginx/user-guide/exposing-tcp-udp-services/ in order to expose my non-http(s) TCP and UDP services externally. And I have my log is full of messages of this kind:

[95.168.173.54] [05/Apr/2020:06:55:34 +0000] TCP 200 3460 2441 0.070
[95.168.167.248] [05/Apr/2020:06:55:34 +0000] TCP 200 3460 2441 0.075
[95.168.167.236] [05/Apr/2020:06:55:34 +0000] TCP 200 3460 2441 0.396
[95.168.173.54] [05/Apr/2020:06:55:41 +0000] TCP 200 3460 2441 3.782
[95.168.167.236] [05/Apr/2020:06:55:34 +0000] TCP 200 3460 2441 0.080
[5.79.106.162] [05/Apr/2020:06:55:35 +0000] TCP 200 3460 3059 0.059

It would be nice to have the ability to turn off Stream access log entirely but leave HTTP access log as I need it. For now, I've turned off both using disable-access-log. It looks very natural to me as the config itself contains two distinct places that can be easily managed by the new more specific directives.

Antiarchitect · 2020-05-02T16:53:40Z

@ElvinEfendi @cmluciano Any progress on this?

ElvinEfendi · 2020-05-02T23:48:21Z

This looks good to me, but I'd like to see some tests. @Antiarchitect any chance you can include e2e test to cover this new functionality? You can check the tests in test/e2e/settings/ for inspiration.

Antiarchitect · 2020-05-03T17:27:15Z

@ElvinEfendi I'm bad at Go, but will try to add tests in a few days or so. Still thinking this change is more than trivial.

cmluciano

/ok-to-test

adding label so that tests will kick in once pushed

Two new configuration options: `disable-http-access-log` `disable-stream-access-log` Should resolve issue with enormous amount of `TCP 200` useless entries in logs Signed-off-by: Andrey Voronkov <voronkovaa@gmail.com>

Signed-off-by: Andrey Voronkov <voronkovaa@gmail.com>

Antiarchitect · 2020-05-13T19:37:57Z

/retest

codecov-io · 2020-05-13T19:45:46Z

Codecov Report

Merging #5348 into master will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5348      +/-   ##
==========================================
+ Coverage   58.56%   58.57%   +0.01%     
==========================================
  Files          88       88              
  Lines        6948     6950       +2     
==========================================
+ Hits         4069     4071       +2     
  Misses       2431     2431              
  Partials      448      448

Impacted Files	Coverage Δ
internal/ingress/controller/template/template.go	`79.67% <0.00%> (-0.22%)`	⬇️
internal/ingress/metric/collectors/process.go	`90.62% <0.00%> (+2.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46cca5a...96d8699. Read the comment docs.

Antiarchitect · 2020-05-13T20:03:42Z

@ElvinEfendi @cmluciano Sorry for the delay guys. I've just added tests, but do not know how to make them better and test that access_log off appears in specific blocks. If you have any ideas of how to improve - you're welcome :)

Antiarchitect · 2020-05-19T01:23:14Z

@ElvinEfendi @cmluciano Any news guys?

Antiarchitect · 2020-05-29T11:28:53Z

@ElvinEfendi @cmluciano Bump

test/e2e/annotations/disableaccesslog.go

aledbf · 2020-06-04T18:31:59Z

disable-http-access-log
disable-stream-access-log

Can we change this to enable-http-access-log and enable-stream-access-log, just to be clear,
enable-http-access-log: "false"

Note: I know we have used disable-* in the past

Antiarchitect · 2020-06-04T23:00:57Z

@aledbf All three? It would be backward incompatible. In case we change only two annotations it would be awkward and even ambiguous.

Antiarchitect · 2020-06-15T11:34:28Z

Guys, should I expect this will be resolved somehow?

Antiarchitect · 2020-08-14T02:09:43Z

@ElvinEfendi @cmluciano Is there any resolution on this?

aledbf · 2020-09-28T17:52:19Z

/lgtm
/approve

aledbf · 2020-09-28T17:52:27Z

@Antiarchitect thanks!

k8s-ci-robot · 2020-09-28T17:53:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, Antiarchitect

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 11, 2020

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 11, 2020

k8s-ci-robot requested review from cmluciano and ElvinEfendi April 11, 2020 18:43

Antiarchitect mentioned this pull request Apr 15, 2020

[stable/nginx-ingress] Ability to suppress access log events for TCP/UDP streams helm/charts#21879

Closed

cmluciano reviewed May 6, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 6, 2020

Antiarchitect added 2 commits May 13, 2020 21:23

Ability to separately disable access log in http and stream contexts

bced1ed

Two new configuration options: `disable-http-access-log` `disable-stream-access-log` Should resolve issue with enormous amount of `TCP 200` useless entries in logs Signed-off-by: Andrey Voronkov <voronkovaa@gmail.com>

Disable Access Log tests. Draft.

96d8699

Signed-off-by: Andrey Voronkov <voronkovaa@gmail.com>

Antiarchitect force-pushed the stream-log-annotations branch from baebda7 to 96d8699 Compare May 13, 2020 19:22

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 13, 2020

Antiarchitect requested a review from cmluciano May 19, 2020 01:23

ElvinEfendi reviewed May 31, 2020

View reviewed changes

test/e2e/annotations/disableaccesslog.go Show resolved Hide resolved

ElvinEfendi mentioned this pull request Jun 3, 2020

access-log-path doesn't work in the Nginx stream block #5647

Closed

k8s-ci-robot assigned aledbf Sep 28, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 28, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2020

k8s-ci-robot merged commit 8d45bb3 into kubernetes:master Sep 28, 2020

Antiarchitect deleted the stream-log-annotations branch September 28, 2020 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to separately disable access log in http and stream contexts #5348

Ability to separately disable access log in http and stream contexts #5348

Antiarchitect commented Apr 11, 2020 •

edited

Loading

k8s-ci-robot commented Apr 11, 2020

k8s-ci-robot commented Apr 11, 2020

aledbf commented Apr 12, 2020

Antiarchitect commented Apr 12, 2020 •

edited

Loading

Antiarchitect commented May 2, 2020

ElvinEfendi commented May 2, 2020

Antiarchitect commented May 3, 2020

cmluciano left a comment

Antiarchitect commented May 13, 2020

codecov-io commented May 13, 2020

Antiarchitect commented May 13, 2020 •

edited

Loading

Antiarchitect commented May 19, 2020

Antiarchitect commented May 29, 2020

aledbf commented Jun 4, 2020

Antiarchitect commented Jun 4, 2020 •

edited

Loading

Antiarchitect commented Jun 15, 2020

Antiarchitect commented Aug 14, 2020

aledbf commented Sep 28, 2020

aledbf commented Sep 28, 2020

k8s-ci-robot commented Sep 28, 2020

Ability to separately disable access log in http and stream contexts #5348

Ability to separately disable access log in http and stream contexts #5348

Conversation

Antiarchitect commented Apr 11, 2020 • edited Loading

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Apr 11, 2020

k8s-ci-robot commented Apr 11, 2020

aledbf commented Apr 12, 2020

Antiarchitect commented Apr 12, 2020 • edited Loading

Antiarchitect commented May 2, 2020

ElvinEfendi commented May 2, 2020

Antiarchitect commented May 3, 2020

cmluciano left a comment

Choose a reason for hiding this comment

Antiarchitect commented May 13, 2020

codecov-io commented May 13, 2020

Codecov Report

Antiarchitect commented May 13, 2020 • edited Loading

Antiarchitect commented May 19, 2020

Antiarchitect commented May 29, 2020

aledbf commented Jun 4, 2020

Antiarchitect commented Jun 4, 2020 • edited Loading

Antiarchitect commented Jun 15, 2020

Antiarchitect commented Aug 14, 2020

aledbf commented Sep 28, 2020

aledbf commented Sep 28, 2020

k8s-ci-robot commented Sep 28, 2020

Antiarchitect commented Apr 11, 2020 •

edited

Loading

Antiarchitect commented Apr 12, 2020 •

edited

Loading

Antiarchitect commented May 13, 2020 •

edited

Loading

Antiarchitect commented Jun 4, 2020 •

edited

Loading