Study tcpproxy replacement for something built-in

rikatz commented 1 year ago

Long story short: back in time, when NGINX didn't supported SNI/TLS Passthrough, a local proxy was implemented on the controller to deal with it:

https://github.com/kubernetes/ingress-nginx/blob/9c73dfb809461a7039bf19cf5648cb46094bc82e/internal/ingress/controller/nginx.go#L749

https://github.com/kubernetes/ingress-nginx/blob/9c73dfb809461a7039bf19cf5648cb46094bc82e/pkg/tcpproxy/tcp.go#L60

This approach removes almost all of the dealing of traffic from NGINX and passes to this routine running on the same process of the controller, having performance impact and not being "NGINX" first.

Now, nginx supports SNI passthrough via stream preread (https://nginx.org/en/docs/stream/ngx_stream_ssl_preread_module.html) and combining it with Lua or NJS, we can simply pass this to the right server.

Even the proxy_protocol may be supported all within NGINX: http://nginx.org/en/docs/stream/ngx_stream_proxy_module.html#proxy_ssl

So this issue is more like a proposal that we deprecate the old tcpproxy implemented inside controller, and implement it on NGINX.

k8s-ci-robot commented 1 year ago

@rikatz: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

rikatz commented 1 year ago

/priority important-longterm /triage accepted

rikatz commented 1 year ago

while we don't migrate this, I have added some e2e tests to make sure it works as desired

https://github.com/kubernetes/ingress-nginx/pull/9457

rikatz commented 1 year ago

This is how nginx.conf should look like with Passthrough:

stream {
upstream something {
    server 10.96.154.212:443;
}
map $ssl_preread_server_name $name {
    test.com something;
}
    server {
            listen 1443;
            ssl_preread on;
            proxy_pass $name;
    }
}

When calling nginx on port 1443, it will get the asked name and map to upstream something, passing to it on port 443. I've tested here and it worked fine.

Problems to be solved:

When sslpassthrough is enabled, today, does the old behavior works on Ingress? IIRC just on port 80, when you call port 443 it will defer to controller. Now, with NGINX listening to it, probably something will change
How to deal with timeouts. I'm not sure we have those today on tcpproxy (not healthy to do this anyway)

EDIT: for wildcards we can use regex on maps:

map $ssl_preread_server_name $name {
    "~^[A-Za-z0-9]*\.test\.com$" something;
}

I'm not a huge fan of this, and probably there is some better library to match this. But well, it works :)

longwuyuan commented 1 year ago

For some unknown reason, I can't get the test working. Maybe mismatched nginx version or misconfigured vhost ;

% docker rm nginxwithproxypass --force ; docker rmi longwuyuan/nginx-withproxypass ; docker build -t longwuyuan/nginx-withproxypass .                                                                                          [5/1143]
nginxwithproxypass                                                                                                                                                                                                                     
Untagged: longwuyuan/nginx-withproxypass:latest                                                                                                                                                                                        
Deleted: sha256:716d65364f1127711a6eaaf7e4cbe0fb9095624444afd779bb9b66a83982dfa8                                                                                                                                                       
Deleted: sha256:125a791fb3f335aa3f8e82b8e77c8f43514d7ee3efd710241dfa8c680bae6b54                                                                                                                                                       
Deleted: sha256:c39a12c793de334769b7437979df347afc4da2cd8cc93e4086dc22478f65f338                                                                                                                                                       
Sending build context to Docker daemon  4.608kB
Step 1/3 : FROM nginx:alpine
 ---> 1e415454686a                                       
Step 2/3 : COPY conf/ /etc/nginx/conf.d/
 ---> 6c6ab623b34f                                       
Step 3/3 : EXPOSE 443                                    
 ---> Running in 08084721bea0
Removing intermediate container 08084721bea0
 ---> 39dd1309da4c                                       
Successfully built 39dd1309da4c
Successfully tagged longwuyuan/nginx-withproxypass:latest 
[~/Documents/ingress-nginx-issues/testssl/frontend] 
% docker ps -a                                           
CONTAINER ID   IMAGE                         COMMAND                  CREATED      STATUS      PORTS                                  NAMES
2a5821aba2eb   longwuyuan/nginx-withmycert   "/docker-entrypoint.…"   2 days ago   Up 2 days   80/tcp, 192.168.192.10:8443->443/tcp   nginxwithmycert
[~/Documents/ingress-nginx-issues/testssl/frontend] 
% docker run -d --name nginxwithproxypass -p 192.168.192.10:443:443 longwuyuan/nginx-withproxypass
338fddfad432818d6bed5ebc7d2587eb9e975d68febf278d7a5bb436c778424c
[~/Documents/ingress-nginx-issues/testssl/frontend] 
% docker ps                                              
CONTAINER ID   IMAGE                         COMMAND                  CREATED      STATUS      PORTS                                  NAMES
2a5821aba2eb   longwuyuan/nginx-withmycert   "/docker-entrypoint.…"   2 days ago   Up 2 days   80/tcp, 192.168.192.10:8443->443/tcp   nginxwithmycert
[~/Documents/ingress-nginx-issues/testssl/frontend] 
% docker ps -a                                           
CONTAINER ID   IMAGE                            COMMAND                  CREATED         STATUS                     PORTS                                  NAMES
338fddfad432   longwuyuan/nginx-withproxypass   "/docker-entrypoint.…"   6 seconds ago   Exited (1) 5 seconds ago                                          nginxwithproxypass
2a5821aba2eb   longwuyuan/nginx-withmycert      "/docker-entrypoint.…"   2 days ago      Up 2 days                  80/tcp, 192.168.192.10:8443->443/tcp   nginxwithmycert
[~/Documents/ingress-nginx-issues/testssl/frontend] 
% docker logs nginxwithproxypass
/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty, will attempt to perform configuration
/docker-entrypoint.sh: Looking for shell scripts in /docker-entrypoint.d/
/docker-entrypoint.sh: Launching /docker-entrypoint.d/10-listen-on-ipv6-by-default.sh
10-listen-on-ipv6-by-default.sh: info: ipv6 not available 
/docker-entrypoint.sh: Launching /docker-entrypoint.d/20-envsubst-on-templates.sh
/docker-entrypoint.sh: Launching /docker-entrypoint.d/30-tune-worker-processes.sh
/docker-entrypoint.sh: Configuration complete; ready for start up
2023/01/01 01:58:41 [emerg] 1#1: "stream" directive is not allowed here in /etc/nginx/conf.d/default.conf:1
nginx: [emerg] "stream" directive is not allowed here in /etc/nginx/conf.d/default.conf:1
[~/Documents/ingress-nginx-issues/testssl/frontend] 
% head -15 conf/default.conf 
stream {                                                 
upstream backend {                                       
    server 192.168.192.10:8443;
}                                                        
map $ssl_preread_server_name $name {
    testsl.test.enjoydevops.com backend;

server {                                                 
    listen              0.0.0.0:443 ;
    ssl_preread on;
    proxy_pass $name;

    #access_log  /var/log/nginx/host.access.log  main;

    location / {

[~/Documents/ingress-nginx-issues/testssl/frontend] 
% cat Dockerfile                                         
FROM nginx:alpine                                        

COPY conf/ /etc/nginx/conf.d/

EXPOSE 443

Will keep trying and keep watching updates here

rikatz commented 1 year ago

Stream directive should be on nginx.conf, the stuff on conf.d are under http{} while stream is its own root directive/section

longwuyuan commented 1 year ago

understood. thanks tons

rikatz commented 1 year ago

cc @yagonobre

truongnht commented 1 year ago

any update on this rework, @rikatz ?

truongnht commented 1 year ago

and can you point me to a working setup of sslpassthrough, the old way? I tried setting up with controller.config.use-proxy-protocol="true" and controller.extraArgs.enable-ssl-passthrough, but nginx-controller is still complaining with 400 BadRequest showing up in logs.

rikatz commented 1 year ago

not yet, I'm still thinking on how to implement it.

rikatz commented 1 year ago

Just an update and getting my head around it (I'm adding here for my own documentation).

The approach will be:

If admin enables sslpassthrough, port 443 now belongs to a new server proxy for passthrough
This port will be composed of stream proxy, that will pass any of the passthrough requests to the right backends
Ingresses not part of passthrough will listen on port 442
Default will be sent to port 442 and follow the default behavior, passing to the right backends

chriskim06 commented 1 year ago

Even the proxy_protocol may be supported all within NGINX: http://nginx.org/en/docs/stream/ngx_stream_proxy_module.html#proxy_ssl

would this be a separate feature? i noticed https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/controller/nginx.go#L468-L474, which i believe is why the proxy protocol header isnt sent to the service in the tcp proxy https://github.com/kubernetes/ingress-nginx/blob/main/pkg/tcpproxy/tcp.go#L92-L105 (might have missed something when going through all this).

is this false by default because there isnt a way to specify proxy protocol per ingress?

rikatz commented 1 year ago

I think if we can mutate the proxy_pass (and we can) we can also mutate the proxy_protocol based on what's configured on the backend.

But honestly, this needs a bit of testing before implementing.

I would first implement just the passthrough as it works today (it will take me 1-2 full days working on it) and then implement the Proxy, which should be almost the same thing. I'm just not sure NGINX can deal with it on different ways, but let's see :)

chriskim06 commented 1 year ago

yep makes sense. thanks for working on this! im dealing with sort of a weird setup with my clusters' ingresses where i need passthrough and proxy protocol. it'll be great whenever this lands so that i dont have to maintain my own fork 🙏

jampy commented 3 months ago

Any news on this?

truongnht commented 3 months ago

I guess you just need to accept tcp passthrough will not work with ingress-nginx

kubernetes / ingress-nginx

Study tcpproxy replacement for something built-in #9448