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

Fix full XFF with PROXY #1618

Merged

Conversation

maxlaverse
Copy link
Contributor

@maxlaverse maxlaverse commented Oct 28, 2017

What this PR does / why we need it:
The PR #1489 added a new ConfigMap option (compute-full-forwarded-for) that allows us to compute a full X-Forwarded-For header as described in https://tools.ietf.org/html/rfc7239, section 5.2. With this option enabled, the Nginx Ingress adds the remote IP to the list of IPs (empty or not) of the X-Forwarded-For header (header name configurable). This is fine for requests proxied using the HTTP protocol.

When using the PROXY protocol to forward the request to the Nginx Ingress (with use-proxy-protocol=true in the Ingress Controller ConfigMap), with the current implementation the remote IP is added to the list, which is the L4 proxy IP. As discussed in #1489, this is not the expected behavior as:

  • we loose the address of the client that connected to the L4 proxy
  • L4 proxies act on the TCP level and are supposed to be transparent, meaning they don't appear in the X-Forwarded-For header

This PR will change the behavior of the Ingress when compute-full-forwarded-for and use-proxy-protocol are set to true, and adds the client IP as sent in the PROXY protocol to the X-Forwarded-For header.

Example with compute-full-forwarded-for enabled
With use-proxy-protocol=false, for a client (ip 1.2.3.4) that makes a request to a L7 load-balancer (ip 10.0.0.1) in front of the Ingress, the X-Forwarded-For received by the back-end is 1.2.3.4, 10.0.0.1 (unchanged)

With use-proxy-protocol=true, for a client (ip 1.2.3.4) that makes a request to a L4 load-balancer (ip 10.0.0.1) in front of the Ingress, the X-Forwarded-For received by the back-end would be 1.2.3.4 with this PR. It would have been 10.0.0.1 without.

Special notes for your reviewer:
I used HAProxy in front of the Ingress to test the behavior with the PROXY protocol. I can post the configuration and yaml file if needed.

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

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 28, 2017
@maxlaverse maxlaverse force-pushed the fix_fullforwardedfor_with_useproxy branch from ca00d04 to b85055a Compare October 28, 2017 15:43
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 33.135% when pulling b85055a on maxlaverse:fix_fullforwardedfor_with_useproxy into e4e1e68 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 33.135% when pulling b85055a on maxlaverse:fix_fullforwardedfor_with_useproxy into e4e1e68 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Oct 28, 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 Oct 28, 2017
@aledbf
Copy link
Member

aledbf commented Oct 28, 2017

@maxlaverse thanks!

@aledbf aledbf merged commit 2a9e1eb into kubernetes:master Oct 28, 2017
@maxlaverse maxlaverse deleted the fix_fullforwardedfor_with_useproxy branch October 28, 2017 20:33
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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants