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

Don't use $proxy_protocol var which may be undefined. #885

Closed
wants to merge 1 commit into from
Closed

Don't use $proxy_protocol var which may be undefined. #885

wants to merge 1 commit into from

Conversation

pmahoney-raise
Copy link

@pmahoney-raise pmahoney-raise commented Jun 20, 2017

I'm on AWS with an nginx-ingress-controller behind an ELB (created by a kubernetes Service of type LoadBalancer). I set the following annotations:

      annotations:
        service.beta.kubernetes.io/aws-load-balancer-backend-protocol: "http"
        service.beta.kubernetes.io/aws-load-balancer-proxy-protocol: ""

which disable proxy protocol and enable http backend protocol, which cause the ELB to set X-Forwarded-For and X-Forwarded-Proto headers when communicating with the backend (nginx in this case).

And I have this annotation on the nginx-ingress-controller:

      use-proxy-protocol: "false"

However, the X-Forwarded-For header is not being passed along by nginx to its upstream services. This is because when the X-Forwarded-Proto is set to "https", the nginx config on line 128 sets $pass_access_scheme to "https", which in turn results in $the_x_forwarded_for being set to $proxy_protocol_addr on line 140.

Because I'm not using proxy protocol, $proxy_protocol_addr is unset. However, when using either proxy protocol or X-Forwarded-For, $remote_addr is set correctly by the real_ip_header directives on lines 24 and 27.

This pull request solves my problem, though I can't say it doesn't break other configurations. My goal with this PR is to document the issue.

See #800 (comment)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 20, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 44.539% when pulling b916b5f on raisemarketplace:pm/non-proxy-protocol into 9ec862b on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Jun 21, 2017

@pmahoney-raise please update to quay.io/aledbf/nginx-ingress-controller:0.148 to check this issue is fixed

@pmahoney-raise
Copy link
Author

Yes, it does work. Inspecting the nginx template looks good too. Thanks.

What's the relationship between that version and this codebase?

@aledbf
Copy link
Member

aledbf commented Jun 21, 2017

The change is in #890. Basically I replaced the map checking if proxy is enabled or not.
(Using maps in nginx is not very flexible)

@aledbf
Copy link
Member

aledbf commented Jun 21, 2017

Can we close this PR?

@pmahoney-raise
Copy link
Author

Fixed in #890

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants