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

Remove support for TCP and UDP services #3197

Merged
merged 2 commits into from
Oct 8, 2018

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Oct 7, 2018

No description provided.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 7, 2018
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 7, 2018
@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 Oct 7, 2018
@antoineco
Copy link
Contributor

Can you explain why this is being removed?

@aledbf
Copy link
Member Author

aledbf commented Oct 8, 2018

Can you explain why this is being removed?

These features were added almost three years ago when there were no other options available. Now you can use metallb for instance to get the same features. Also, this created confusion about what Ingress (k8s concept) is about.

If enough users are interested in this feature, I can start a small project with the code being removed, running a different deployment, sharing the service with the ingress controller to avoid the costs of additional LBs. One of the reasons to remove this is the need to reload nginx on changes (there's no support for dynamic configuration)

@aledbf aledbf removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2018
@@ -697,63 +697,6 @@ stream {
{{ end }}

error_log {{ $cfg.ErrorLogPath }};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because I want to refactor the SSL passthrough feature to use the stream section, removing the go proxy code.

@ElvinEfendi
Copy link
Member

/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 8, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 3cf00b2 into kubernetes:master Oct 8, 2018
@aledbf aledbf deleted the remove-tcp-udp branch October 8, 2018 14:57
@karlskewes
Copy link

Shall we also remove from the docs at: https://kubernetes.github.io/ingress-nginx/user-guide/exposing-tcp-udp-services/

? Or do we want to add a notice first? "The TCP and UDP feature is deprecated and will be removed in 0.21.0."

I can do a PR for either.

@ElvinEfendi
Copy link
Member

@kskewes it's being cleanup at https://github.com/kubernetes/ingress-nginx/pull/3222/files#diff-b7d944663319ec93108dd9eb66df1d9dL19

@MMeent
Copy link
Contributor

MMeent commented Oct 13, 2018

These features were added almost three years ago when there were no other options available. Now you can use metallb for instance to get the same features. Also, this created confusion about what Ingress (k8s concept) is about.

Thanks for the explanation. One thing I'm worried about though, is 'how do I migrate'?

Our current setup uses the ingress-nginx as a ingress-controller, as well as a tcp-proxy for several underlying services. All those are then behind a singular LoadBalancer resource (they have no, and will not have, any overlapping externally available ports).

Setting up the underlying services to be LoadBalancers is not an option, as we'll reach limits with our cloud provider very quickly. Using one service as the LoadBalancer and pointing that to specific endpoints is impossible as well due to namespace and label-selector constraints. What do you suggest we should use as a replacement TCP/UDP forwarder, as ingress-nginx is halting support? It doesn't look like metallb will work for us, as we're working in a cloud.

@lambertjosh
Copy link

lambertjosh commented Oct 15, 2018

To share another affected use case, we at GitLab use this functionality as part of our helm chart, so we can support both HTTP and SSH based Git traffic.

Our challenge with this change is that we need both Git and HTTP to be available on the same DNS, and we cannot use MetalLB as we need to support cloud providers. (Plus a key goal of ours is that this chart is easy to use.)

We appreciate all the great work on NGINX Ingress, but unfortunately we will probably need to migrate users off Ingress and find another solution because of this change.

@aledbf
Copy link
Member Author

aledbf commented Oct 15, 2018

@MMeent @lambertjosh as I commented here #3197 (comment) I can start a small project with just this feature, sharing the ingress-nginx service to avoid multiple LBs and also share a FQDN

@lambertjosh
Copy link

@aledbf sorry for missing that comment earlier. I think that would address our use case, and still allow us to take advantage of all the goodness you and the team have built here with NGINX Ingress.

If you'd be willing to do that, it would be great. 🙇

@dghubble
Copy link

Similar feedback here, using this Nginx TCP/UDP feature for Gitea SSH, VOIP, game servers, across multiple platforms, not just bare-metal.. Its convenient to represent TCP/UDP alongside Ingress resources and for ingress-nginx to be the component sitting directly behind the LB. Its also handy for writing Network Policy that all traffic flows through it.

So the idea is the same code moves to a different image/deployment (i.e. spin it off as a separate project) to make this repo just an http/https ingress-nginx?

I'd always rather hoped TCP/UDP would get promoted into the v1 Ingress object, but I suppose that ship has sailed.

@fcgravalos
Copy link

We also distinguish between external and infernal ingress, exposing TCP ports. We even need to expose SFTP for some customers.

I am afraid that removing support for TCP/UDP will make us shift to a different Ingress Controller implementation.

@antoineco
Copy link
Contributor

Also a bit puzzled why this dropped here out of nowhere. But to address your concern @fcgravalos, don't forget that a Pod can have multiple containers. You can run NGINX + (whatever is the stream replacement) side-by-side inside the same Pod. If @aledbf ensures the contract doesn't change you will be able to continue using the solution as you always did.

@aledbf
Copy link
Member Author

aledbf commented Nov 7, 2018

The removal of the TCP and UDP features is being reverted here #3374

Trying to port this features to a separate project failed because:

  • I end up copying too much code from this repository
  • the separate project would not be hosted in the Kubernetes repository
  • this separation introduces a duplication of requests against the apiserver

That said, I am introducing a change in #3374 to handle the upstream servers in Lua.
Is this required? No. Then why this change? Any change in the pods (endpoints) introduce changes in the generated nginx.conf file, which means a reload is required. In most of the cases, this is not an issue but users using keepalive or websocket connections will see a termination in the connection. The goal with this new feature is to avoid nginx reloads.

@aledbf
Copy link
Member Author

aledbf commented Nov 7, 2018

Also and more important I want to apology for this removal.

Until this PR the received feedback from the features was negative (being polite) and we don't have a way to measure what is being used or not. Also, the misconception that these features are part of the Ingress spec confuses lots of users.

If anyone in this thread has any idea how we can find out what is being used or not and the way to interact in the future to avoid something like this happens again, any idea/feedback is welcome

@fcgravalos
Copy link

@antoineco It seems It won't be needed ;). On the other hand, yes I know I can use an nginx container in front if our TCP/UDP services, but that brings other issues. Hopefully this has been reverted :)

@dghubble
Copy link

To followup on my comment above, I reacted to planned removal by switching my infrastructure to use kube-proxy (or IPVS) for non-HTTP(s) TCP/UDP traffic to eliminate a hop (since I had traffic routed to nginx pods via service IP (via ECMP to nodes running a proxy anyway). So this did at least spur revisiting the architecture and I appreciate the original reasons for wanting to descope the feature. But I'm also glad to see the TCP/UDP feature can stay and will follow along with it! :D

@aledbf aledbf deleted the remove-tcp-udp branch November 16, 2018 16:54
@jxadro
Copy link

jxadro commented Nov 20, 2018

Hi, if the removal has been reverted please publish again the documentation. It is not available anymore.
https://kubernetes.github.io/ingress-nginx/user-guide/exposing-tcp-udp-services/

@yhjhoo
Copy link

yhjhoo commented Sep 30, 2022

https://kubernetes.github.io/ingress-nginx/user-guide/exposing-tcp-udp-services/

seems this document is out of dated, is this being removed again?

@dghubble
Copy link

The doc you link to tells you how to proxy TCP/UDP services. Also don't bump years old closed issues.

This issue could be locked

@Rohmilchkaese
Copy link

Can confirm, functionality is there and working, for anyone wondering

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/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.