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

SNI routing using the nginx map preread module #1432

Closed
wants to merge 4 commits into from

Conversation

bobveznat
Copy link
Contributor

I want to preface this by saying that this is a disgusting pile of a diff here. The purpose of me sharing this commit is to share how I'm using the project and possibly spawn conversation.

I was a little surprised to learn that the current SNI routing system implemented by this project is not actually leveraging nginx at all, but is entirely done in golang. I simply was unwilling to connect the current ingress sni routing feature to the Internet. nginx, however, is tried and true and supports SNI routing.

I modeled this change after the TCP l4 stuff that already exists, there's a fair bit of copy-pasta in here.

I also really wanted to leverage the serviceUpstream change that is reasonably recent to the project. My deployment of Kubernetes relies on having persistent connections last as long as possible because TLS handshakes are expensive. During deployments every time the "pod topology" changed (i.e. anytime a pod changed status) nginx would reload it's configuration and recycle, ultimately nuking all open connections. Using serviceUpstream we are ~never reloading our nginx config and therefore aren't artificially terminating connections.

Again, I don't expect this code to ever get merged, at least not as-is. I just wanted the maintainers to be able to see how we're using the project.

Since you might be wondering why I would deploy code that I don't want merged upstream: we're expecting to migrate away from SNI routing ~soon. This is a stopgap (prior to this we were SNI routing with haproxy and code that looks like an ingress controller but that predates ingress controllers).

I've developed a poor and embarrassing form of SNI support into the
ingress controller based on the existing TCP "support". This probably
only works with nginx.

I don't think anyone should attempt to merge this code anywhere,
however, functionally this is the behavior that I wanted.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 27, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 27, 2017
@aledbf
Copy link
Member

aledbf commented Sep 27, 2017

@bobveznat please check #614
Using the preread module implies we lose the source IP address which is deal breaker from the start.
Just in case the golang code is used only if you use the flag --enable-ssl-passthrough (disabled by default) in which case the preread module is not required.

@bobveznat
Copy link
Contributor Author

I don't think I understand your source IP comment? With the config in this PR the nginx variable $remote_addr contains the proper remote address. And when the upstream is configured for proxy protocol (like in this diff) the destination pod sees the "real" source IP. My pods are, of course, using proxy protocol capable sockets.

My hesitation to use the current SNI routing (--ssl-passthrough) is that I believe building a reverse proxy is hard (I once worked on a forward proxy). In my experience there's a "long tail" of weird ways connections fail and in some cases resources leak and servers become overloaded. I'm much more confident that nginx has dealt with most of these. This is just my opinion. I get paged when stuff blows up and I don't like to get paged :-).

@aledbf
Copy link
Member

aledbf commented Sep 27, 2017

My pods are, of course, using proxy protocol capable sockets.

Not all the user can do that

@aledbf aledbf added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 30, 2017
@aledbf aledbf closed this Nov 4, 2017
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. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants