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.

Sign up for GitHub

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

Jump to bottom

proxy_protocol on ssl_passthrough listener #227

Merged
merged 2 commits into from
Feb 5, 2017
Merged

proxy_protocol on ssl_passthrough listener #227

merged 2 commits into from
Feb 5, 2017

Conversation

justinsb
Copy link
Member

@justinsb justinsb commented Feb 4, 2017

Move proxy_protocol to listener.

Fix #207

@justinsb
proxy_protocol on ssl_passthrough listener
6fa461c 
Move proxy_protocol to listener.

Fix kubernetes#207
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@justinsb
Copy link
Member Author

justinsb commented Feb 4, 2017

Without this PR, running behind an ELB with proxy-protocol enabled, we see the inbound request, and nginx immediately responds with:

HTTP/1.1 400 Bad Request
Server: nginx/1.11.9
Date: Sat, 04 Feb 2017 07:35:51 GMT
Content-Type: text/html
Content-Length: 173
Connection: close
Strict-Transport-Security: max-age=15724800; includeSubDomains; preload

<html>
<head><title>400 Bad Request</title></head>
<body bgcolor="white">
<center><h1>400 Bad Request</h1></center>
<hr><center>nginx/1.11.9</center>
</body>
</html>

With this PR, ssl passthrough when the ELB has proxy protocol enabled works.

I believe that with the proxy_protocol in its current position, nginx adds the proxy protocol header before forwarding. This may actually be useful in some circumstances, but is not what we are expecting here (I believe).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 44.249% when pulling 6fa461c on justinsb:use_proxy_protocol into 95d6900 on kubernetes:master.

@justinsb
Copy link
Member Author

justinsb commented Feb 4, 2017

I have found a snafu, so marking WIP.

The problem is that the SSL 442 listener also has proxy_protocol, but it should not, because the 443 listener with this PR already removes the header. I think I need to remove proxy_protocol from the 442 listener.

@justinsb justinsb changed the title proxy_protocol on ssl_passthrough listener WIP: proxy_protocol on ssl_passthrough listener Feb 4, 2017
@justinsb
Remove proxy_protocol from 442 listener
8d71557 
The proxy_protocol processing should only happen once, on the
"external-facing" listeners.
@justinsb
Copy link
Member Author

justinsb commented Feb 5, 2017

OK, that was easy! Removed proxy_protocol from 442, marking as no-longer-WIP.

@justinsb justinsb changed the title WIP: proxy_protocol on ssl_passthrough listener proxy_protocol on ssl_passthrough listener Feb 5, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 44.409% when pulling 8d71557 on justinsb:use_proxy_protocol into 95d6900 on kubernetes:master.

@aledbf aledbf self-assigned this Feb 5, 2017
@aledbf
Copy link
Member

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

aledbf commented Feb 5, 2017

@justinsb thanks for fixing this.

@aledbf aledbf merged commit e35e5bf into kubernetes:master Feb 5, 2017
@gianrubio gianrubio mentioned this pull request Mar 1, 2017
Merged
gianrubio added a commit to gianrubio/ingress that referenced this pull request Mar 1, 2017
@gianrubio
Disable listen only on ipv6 and fix proxy_protocol
90fdea7 
- Always listen on ipv4 address for port 443
- Rollback previous PR kubernetes#227 that broke the proxy_protocol when passthroughBackends is disabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aledbf aledbf

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

Successfully merging this pull request may close these issues.
5 participants
@justinsb @k8s-reviewable @coveralls @aledbf @k8s-ci-robot