Enable access log for default backend #3780

mymarche · 2019-02-19T21:56:23Z

What this PR does / why we need it:
Enable access log for default backend.
Without this PR, redirected URLs to the default backend are not logged.

https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/template/nginx.tmpl#L1107
on this line, the condition for the default backend is true, since the variable $location.Logs.Access does not exist

Which issue this PR fixes:
fixes #3603

Special notes for your reviewer:

k8s-ci-robot · 2019-02-19T21:56:36Z

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://git.k8s.io/community/CLA.md#the-contributor-license-agreement 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.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

codecov-io · 2019-02-19T22:26:22Z

Codecov Report

Merging #3780 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3780   +/-   ##
=======================================
  Coverage   54.82%   54.82%           
=======================================
  Files          82       82           
  Lines        6116     6116           
=======================================
  Hits         3353     3353           
  Misses       2368     2368           
  Partials      395      395

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 43cfbb9...afc4784. Read the comment docs.

mymarche · 2019-02-22T14:32:29Z

@aledbf Could you approve this PR? Without it very difficult to debug incoming requests to the ingress controller.

aledbf · 2019-02-22T14:35:46Z

@arturxx8 please add at least one e2e test (checking the log contains information about the request) and also squash the commits

k8s-ci-robot · 2019-02-25T16:58:38Z

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://git.k8s.io/community/CLA.md#the-contributor-license-agreement 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.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

aledbf · 2019-02-25T17:02:42Z

@arturxx8 please sign the CLA and squash the commits

ElvinEfendi · 2019-02-25T19:51:09Z

test/e2e/defaultbackend/default_backend.go

+	})
+
+	It("should not logging access to default backend when it disabled", func() {
+		f.UpdateNginxConfigMapData("enable-access-log-for-default-backend", "true")


This should be set to "false", no?

yes, this is mistake. I'm fixed it

ElvinEfendi · 2019-02-25T19:51:29Z

test/e2e/defaultbackend/default_backend.go

+
+		logs, err := f.NginxLogs()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(logs).To(ContainSubstring(`"/somethingTwo"`))


And this should be ToNot instead of To

yes, this is mistake. I'm fixed it too

ElvinEfendi · 2019-02-25T19:52:18Z

test/e2e/defaultbackend/default_backend.go

@@ -98,4 +98,35 @@ var _ = framework.IngressNginxDescribe("Default backend", func() {
 			Expect(resp.StatusCode).Should(Equal(test.Status))
 		}
 	})
+	It("should logging access to default backend when it enabled", func() {


Please change the title to something like

enables access logging for default backend

Thanks, I fixed it

ElvinEfendi · 2019-02-25T19:52:29Z

test/e2e/defaultbackend/default_backend.go

+		Expect(logs).To(ContainSubstring(`"/somethingOne"`))
+	})
+
+	It("should not logging access to default backend when it disabled", func() {


Please change the title to something like

disables access logging for default backend

Thanks, I fixed it too

ElvinEfendi · 2019-02-25T19:53:46Z

test/e2e/defaultbackend/default_backend.go

+		f.UpdateNginxConfigMapData("enable-access-log-for-default-backend", "true")
+		host := "bar"
+		resp, _, errs := gorequest.New().
+			Get(f.IngressController.HTTPURL+"/somethingTwo").


You need to rebase with master and use the new API f.GetURL(framework.HTTP) here instead of f.IngressController.HTTPURL

Thanks, but I don't know because I get it error in previous time:
test/e2e/defaultbackend/default_backend.go:105:9: f.IngressController undefined (type *framework.Framework has no field or method IngressController)

ElvinEfendi · 2019-02-25T19:53:58Z

test/e2e/defaultbackend/default_backend.go

+		f.UpdateNginxConfigMapData("enable-access-log-for-default-backend", "true")
+		host := "foo"
+		resp, _, errs := gorequest.New().
+			Get(f.IngressController.HTTPURL+"/somethingOne").


same as https://github.com/kubernetes/ingress-nginx/pull/3780/files#r259991638

ElvinEfendi · 2019-02-25T20:51:17Z

/lgtm
/approve

thanks @arturxx8 !

mymarche

fix e2e

mymarche · 2019-02-25T21:20:58Z

test/e2e/defaultbackend/default_backend.go

+
+		logs, err := f.NginxLogs()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(logs).To(ContainSubstring("/somethingOne"))


@ElvinEfendi I replace `"/somethingOne"` to "/somethingOne" because compare string looks like this:
10.244.2.1 - [10.244.2.1] - - [25/Feb/2019:21:04:21 +0000] "GET /somethingOne HTTP/1.1" 404 153 "-" "Go-http-client/1.1" 147 0.000 [upstream-default-backend] 127.0.0.1:8181 153 0.000 404 b7b4ad156e96a41d5543528f3bdc2ca6

mymarche · 2019-02-25T21:23:14Z

test/e2e/defaultbackend/default_backend.go

+
+		logs, err := f.NginxLogs()
+		Expect(err).ToNot(HaveOccurred())
+		Expect(logs).ToNot(ContainSubstring("/somethingTwo"))


This i replace this same

Why was this line printed?
Is this a debug? I do not remember such lines in version 0.22.0
127.0.0.1 - [127.0.0.1] - - [25/Feb/2019:21:52:58 +0000] "GET /somethingTwo HTTP/1.1" 404 153 "-" "Go-http-client/1.1" 345 0.000 [internal] - - - - f31a446e7c617a1425739aa8adfa6c67

can you help me?

@arturxx8 I think this is coming from the "upstream server":

ingress-nginx/rootfs/etc/nginx/template/nginx.tmpl

Line 612 in c50fe1b

# backend for when default-backend-service is not configured or it does not have endpoints

. That's the upstream used when there's no custom default backend configured.

I'm okay to solve this by adding access_log off; in that server block.

disable log on default_server

mymarche · 2019-02-26T09:10:50Z

@ElvinEfendi i fixed e2e tests. Could you add lgtm label back

ElvinEfendi · 2019-02-26T13:48:45Z

/lgtm

k8s-ci-robot · 2019-02-26T13:48:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arturxx8, ElvinEfendi

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~~ [ElvinEfendi]

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 requested review from aledbf and ElvinEfendi February 19, 2019 21:56

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 Feb 25, 2019

ElvinEfendi reviewed Feb 25, 2019

View reviewed changes

ElvinEfendi mentioned this pull request Feb 25, 2019

Create 0.23.0-rc1 for testing purposes. Shopify/ingress#201

Merged

k8s-ci-robot assigned ElvinEfendi Feb 25, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 25, 2019

mymarche commented Feb 25, 2019

View reviewed changes

Enable access log for default backend

8b3702c

disable log on default_server

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2019

k8s-ci-robot merged commit ec63281 into kubernetes:master Feb 26, 2019

ilya-spv mentioned this pull request Jul 26, 2019

No logging when traffic is routed to the default backend #4363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable access log for default backend #3780

Enable access log for default backend #3780

mymarche commented Feb 19, 2019

k8s-ci-robot commented Feb 19, 2019

codecov-io commented Feb 19, 2019

mymarche commented Feb 22, 2019

aledbf commented Feb 22, 2019 •

edited

Loading

k8s-ci-robot commented Feb 25, 2019

aledbf commented Feb 25, 2019

ElvinEfendi Feb 25, 2019

mymarche Feb 25, 2019

ElvinEfendi Feb 25, 2019

mymarche Feb 25, 2019

ElvinEfendi Feb 25, 2019

mymarche Feb 25, 2019

ElvinEfendi Feb 25, 2019

mymarche Feb 25, 2019

ElvinEfendi Feb 25, 2019

mymarche Feb 25, 2019

ElvinEfendi Feb 25, 2019

ElvinEfendi commented Feb 25, 2019

mymarche left a comment

mymarche Feb 25, 2019 •

edited

Loading

mymarche Feb 25, 2019

mymarche Feb 25, 2019

ElvinEfendi Feb 25, 2019 •

edited

Loading

mymarche commented Feb 26, 2019

ElvinEfendi commented Feb 26, 2019

k8s-ci-robot commented Feb 26, 2019

Enable access log for default backend #3780

Enable access log for default backend #3780

Conversation

mymarche commented Feb 19, 2019

k8s-ci-robot commented Feb 19, 2019

codecov-io commented Feb 19, 2019

Codecov Report

mymarche commented Feb 22, 2019

aledbf commented Feb 22, 2019 • edited Loading

k8s-ci-robot commented Feb 25, 2019

aledbf commented Feb 25, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Feb 25, 2019

mymarche left a comment

Choose a reason for hiding this comment

mymarche Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

mymarche commented Feb 26, 2019

ElvinEfendi commented Feb 26, 2019

k8s-ci-robot commented Feb 26, 2019

aledbf commented Feb 22, 2019 •

edited

Loading

mymarche Feb 25, 2019 •

edited

Loading

ElvinEfendi Feb 25, 2019 •

edited

Loading