Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

[nginx-ingress-controller] Initialize proxy_upstream_name variable #1802

Merged
merged 1 commit into from
Sep 29, 2016

Conversation

aledbf
Copy link
Contributor

@aledbf aledbf commented Sep 29, 2016

fixes #1801


This change is Reviewable

@kayrus
Copy link
Contributor

kayrus commented Sep 29, 2016

two notes:

  • set empty value to -, otherwise log parsing will be broken
  • please remove whitespaces

@aledbf
Copy link
Contributor Author

aledbf commented Sep 29, 2016

set empty value to -, otherwise log parsing will be broken

It's inside []

@kayrus
Copy link
Contributor

kayrus commented Sep 29, 2016

@aledbf true. LGTM then.

@bprashanth
Copy link

LGTM, please send the documentation pr that describs log format output so people can leverage it for greater good (and it keeps un honest about changing the format)

@bprashanth bprashanth added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2016
@kayrus
Copy link
Contributor

kayrus commented Sep 29, 2016

Issue still exists:

2016/09/29 18:25:16 [warn] 25#25: *67 using uninitialized "proxy_upstream_name" variable while logging request, client: 1.2.3.4, server: example.com, request: "HEAD / HTTP/1.0", host: "example.com"

@aledbf
Copy link
Contributor Author

aledbf commented Sep 29, 2016

@kayrus example.com is defined in an Ingress rule?

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 0e7f260 into kubernetes-retired:master Sep 29, 2016
@kayrus
Copy link
Contributor

kayrus commented Sep 29, 2016

@aledbf Yep

@kayrus
Copy link
Contributor

kayrus commented Sep 29, 2016

It is reproducible within non-https request.

@aledbf
Copy link
Contributor Author

aledbf commented Sep 29, 2016

@kayrus please change

            {{- buildProxyPass $location }}
            set $proxy_upstream_name "{{ $location.Upstream.Name }}";

to

            set $proxy_upstream_name "{{ $location.Upstream.Name }}";
            {{- buildProxyPass $location }}

@kayrus
Copy link
Contributor

kayrus commented Sep 29, 2016

It is not easy without this fix :) #1498
just wonder whether it is reasonable to change these strings too:

         location / {
             set $proxy_upstream_name "upstream-default-backend";
             proxy_pass             http://upstream-default-backend;
         }

to

         set $proxy_upstream_name "upstream-default-backend";
         location / {
             proxy_pass             http://upstream-default-backend;
         }

@kayrus
Copy link
Contributor

kayrus commented Sep 29, 2016

@aledbf no, your last suggestion doesn't resolve this issue. you can easily reproduce it using:

curl -v -X HEAD example.com

@aledbf
Copy link
Contributor Author

aledbf commented Sep 29, 2016

@kayrus I cannot reproduce the error. Can you test with the mentioned image #1805 (comment) ?

@aledbf aledbf deleted the fix-vars branch October 2, 2016 20:50
aledbf pushed a commit to aledbf/contrib that referenced this pull request Nov 10, 2016
Automatic merge from submit-queue

[nginx-ingress-controller] Initialize proxy_upstream_name variable

fixes kubernetes-retired#1801
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ingress: uninitialized "proxy_upstream_name" variable while logging request
5 participants