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

Would you consider building unprivileged nginx-ingress controller or have you got this already? #529

Closed
sandywang1982 opened this issue Apr 2, 2019 · 13 comments · Fixed by #710
Assignees
Labels
proposal An issue that proposes a feature request

Comments

@sandywang1982
Copy link

sandywang1982 commented Apr 2, 2019

Is your feature request related to a problem? Please describe.
We gain tls termination over tcp with this nginx-ingress controller, we appreciate a lot.
In order to follow the security compliance, we need to start pod using nonroot user.

I noticed there is unprivileged nginx image: https://github.com/nginxinc/docker-nginx-unprivileged/blob/master/README.md, and I followed the instructions https://github.com/nginxinc/kubernetes-ingress/tree/v1.4.5/build to build nginx-ingress after tweaking the dockerfiles and nginx.tmpl.
There are some privileged locations hard-coded in go source code, for example: listen unix:/var/run/nginx-config-version.sock, so the nginx-ingress binary built by go doesn't really want to run in unprivileged way.
We have no expert on Go language, so we are pretty much stuck here.

Describe the solution you'd like
Has this project ever been considered to build unprivileged nginx-ingress?
If not, what is the reason or concerns?
If yes, that's great.

@Rulox
Copy link
Contributor

Rulox commented Apr 3, 2019

Hi @sandywang1982!, let me try to help you.

We gain tls termination over tcp with this nginx-ingress controller, we appreciate a lot.

This feature is not supported right now (natively) in this Ingress Controller. There are, of course, ways of achieving this. Can you provide more information on how you made possible TLS termination for TCP connections? That would help a lot.

Regarding the unprivileged use of IC, a couple of things:

  • We agree it seems it's a good practice and we can consider the implementation.

  • What is your use case right now for the IC? Would it be possible that you run NGINX without the Ingress Controller and specify the configuration via a ConfigMap volume? In this case, you could use the unprivileged docker image (https://github.com/nginxinc/docker-nginx-unprivileged/blob/master/README.md)

  • At the moment, the Ingress Controller needs to be run as root for some of its internal features as you mentioned. We are discussing internally the consequences of adapting the IC to run without root, I will update you as soon as we make a decision.

In the meantime, if you could answer the questions that would help a lot to make sure we are on the same page.
Thanks!

@Rulox Rulox self-assigned this Apr 3, 2019
@pleshakov pleshakov added the proposal An issue that proposes a feature request label Apr 3, 2019
@sandywang1982
Copy link
Author

sandywang1982 commented Apr 7, 2019

Thanks Rulox.
You are right, the helm charts we pulled doesn't support tls termination over tcp.
We have achieved this by:

  1. Mount the certs from k8s secrets onto nginx-ingress controller pod using volumemount.
  2. use stream-snippets in configmap and point to the mounted certs for server configuration
  3. add tcp ports to controller service.
    There is a shortcoming for defining upstream in stream-snippets. If the upstream server doesn't exist, nginx-ingress complains, so we alter the stream-snippets for different clusters.

@sandywang1982
Copy link
Author

Also we use nginx-ingress as part of our saas solution, there are many http endpoints and tcp servers behind it, to use configmap to serve all those upstream servers seems overkill for us. We may have to stick with root user for now and harden the environment around nginx-ingress controller.

@Rulox
Copy link
Contributor

Rulox commented Apr 8, 2019

Hi @sandywang1982

Regarding this:

There is a shortcoming for defining upstream in stream-snippets. If the upstream server doesn't exist, nginx-ingress complains, so we alter the stream-snippets for different clusters.

One solution would be to use proxy_pass as a variable. Check this blog post (https://www.nginx.com/blog/dns-service-discovery-nginx-plus) look for the section Setting the Domain Name in a Variable

Alternatively, you can use NGINX Plus with its re-resolving DNS feature (https://github.com/nginxinc/kubernetes-ingress/tree/master/examples/tcp-udp#3-configure-load-balancing)

In the short term we are not going to add additional support for TCP as we are focused in other features for the next release. However, we do think this could be considered and included in the future. Therefore it would be nice to have more insights on how you are using TCP Load Balancing by knowing how many ports are you exposing through the IC, or what exactly are the features you are looking for/missing. That would be really helpful.

Regarding the unprivileged nginx-ingress-controller, this is right now being considered by the team and I will update this issue with more information once I have it.

Thanks!

@sandywang1982
Copy link
Author

sandywang1982 commented Apr 8, 2019

Thanks for the useful links and feedback.
Yes, we made the dynamic dns resolve work by using proxy_pass as a variable.
We currently have 3 tcp upstream servers behind nginx, they are all defined in stream-snippets.

@RichardoC
Copy link

@Rulox is "unprivileged nginx-ingress-controller" going to be coming any time soon?

Thanks!

@Rulox
Copy link
Contributor

Rulox commented Aug 8, 2019

Hi @RichardoC

We consider this is really important and we will start working on this feature probably from next week, so yes, it'll be part of the edge version of the Ingress Controller soon.

@Rulox
Copy link
Contributor

Rulox commented Sep 27, 2019

@RichardoC @sandywang1982 once #710 is merged, non-root will be available in the edge version of the IC. Sorry for the delay, we needed to test this so carefully before committing to it!

This will also be in the next release (1.6) of the IC. Thanks!

@Rulox Rulox closed this as completed in #710 Oct 1, 2019
@Rulox
Copy link
Contributor

Rulox commented Oct 1, 2019

@sandywang1982 @RichardoC The non-root images are in master now, this means they are available in the edge version of the Ingress Controller.

Please do not hesitate to open an issue if you spot any problem. 🎉

@RichardoC
Copy link

@Rulox where can I find the instructions for running the helm chart without root? Thanks!

@Rulox
Copy link
Contributor

Rulox commented Oct 29, 2019

Hi @RichardoC

The edge version of the Ingress Controller (master branch) is ready for this feature. You only need to use manifests on that branch. Depending whether you are using NGINX open source or NGINX Plus.

  1. For NGINX OSS, simply redeploy the ingress controller using the master branch manifests and the last edge from dockerhub.

  2. For NGINX Plus, rebuild the Ingress Controller using the DockerfileForPlus in master and redeploy the ingress controller using the master branch manifests.

Let me know if this makes sense. Non-root feature will be available in 1.6 release of the NGINX Ingress Controller that we will release soon.

@RichardoC
Copy link

Thanks for the update! That makes sense

@medicharlachiranjeevi
Copy link

It still uses the root user group. Is there a way to avoid that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal An issue that proposes a feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants