[PATCH 3 of 3] Stream: ngx_stream_pass_module

Sergey Kandaurov pluknet at nginx.com
Thu Feb 29 11:38:55 UTC 2024


On Wed, Feb 28, 2024 at 06:22:34PM +0400, Roman Arutyunyan wrote:
> Hi,
> 
> On Wed, Feb 28, 2024 at 02:15:40PM +0400, Sergey Kandaurov wrote:
> > On Wed, Feb 21, 2024 at 05:37:51PM +0400, Roman Arutyunyan wrote:
> > > Hi,
> > > 
> > > Attached is an improved version with the following changes:
> > > 
> > > - Removed 'listen = 1' flag when parsing "pass" parameter.
> > >   Now it's treated like "proxy_pass" parameter.
> > > - Listen match reworked to be able to match wildcards.
> > > - Local_sockaddr is copied to the connection after match.
> > > - Fixes in log action, log messages, commit log etc.
> > > 
> > > --
> > > Roman Arutyunyan
> > 
> > > # HG changeset patch
> > > # User Roman Arutyunyan <arut at nginx.com>
> > > # Date 1708522562 -14400
> > > #      Wed Feb 21 17:36:02 2024 +0400
> > > # Node ID 44da04c2d4db94ad4eefa84b299e07c5fa4a00b9
> > > # Parent  4eb76c257fd07a69fc9e9386e845edcc9e2b1b08
> > > Stream: ngx_stream_pass_module.
> > > 
> > > The module allows to pass connections from Stream to other modules such as HTTP
> > > or Mail, as well as back to Stream.  Previously, this was only possible with
> > > proxying.  Connections with preread buffer read out from socket cannot be
> > > passed.
> > > 
> > > The module allows selective SSL termination based on SNI.
> > > 
> > >     stream {
> > >         server {
> > >             listen 8000 default_server;
> > >             ssl_preread on;
> > >             ...
> > >         }
> > > 
> > >         server {
> > >             listen 8000;
> > >             server_name foo.example.com;
> > >             pass 127.0.0.1:8001; # to HTTP
> > >         }
> > > 
> > >         server {
> > >             listen 8000;
> > >             server_name bar.example.com;
> > >             ...
> > >         }
> > >     }
> > > 
> > >     http {
> > >         server {
> > >             listen 8001 ssl;
> > >             ...
> > > 
> > >             location / {
> > >                 root html;
> > >             }
> > >         }
> > >     }
> > > 
> > > diff --git a/auto/modules b/auto/modules
> > > --- a/auto/modules
> > > +++ b/auto/modules
> > > @@ -1166,6 +1166,16 @@ if [ $STREAM != NO ]; then
> > >          . auto/module
> > >      fi
> > >  
> > > +    if [ $STREAM_PASS = YES ]; then
> > > +        ngx_module_name=ngx_stream_pass_module
> > > +        ngx_module_deps=
> > > +        ngx_module_srcs=src/stream/ngx_stream_pass_module.c
> > > +        ngx_module_libs=
> > > +        ngx_module_link=$STREAM_PASS
> > > +
> > > +        . auto/module
> > > +    fi
> > > +
> > >      if [ $STREAM_SET = YES ]; then
> > >          ngx_module_name=ngx_stream_set_module
> > >          ngx_module_deps=
> > > diff --git a/auto/options b/auto/options
> > > --- a/auto/options
> > > +++ b/auto/options
> > > @@ -127,6 +127,7 @@ STREAM_GEOIP=NO
> > >  STREAM_MAP=YES
> > >  STREAM_SPLIT_CLIENTS=YES
> > >  STREAM_RETURN=YES
> > > +STREAM_PASS=YES
> > >  STREAM_SET=YES
> > >  STREAM_UPSTREAM_HASH=YES
> > >  STREAM_UPSTREAM_LEAST_CONN=YES
> > > @@ -337,6 +338,7 @@ use the \"--with-mail_ssl_module\" optio
> > >          --without-stream_split_clients_module)
> > >                                           STREAM_SPLIT_CLIENTS=NO    ;;
> > >          --without-stream_return_module)  STREAM_RETURN=NO           ;;
> > > +        --without-stream_pass_module)    STREAM_PASS=NO             ;;
> > >          --without-stream_set_module)     STREAM_SET=NO              ;;
> > >          --without-stream_upstream_hash_module)
> > >                                           STREAM_UPSTREAM_HASH=NO    ;;
> > > @@ -556,6 +558,7 @@ cat << END
> > >    --without-stream_split_clients_module
> > >                                       disable ngx_stream_split_clients_module
> > >    --without-stream_return_module     disable ngx_stream_return_module
> > > +  --without-stream_pass_module       disable ngx_stream_pass_module
> > >    --without-stream_set_module        disable ngx_stream_set_module
> > >    --without-stream_upstream_hash_module
> > >                                       disable ngx_stream_upstream_hash_module
> > > diff --git a/src/stream/ngx_stream_pass_module.c b/src/stream/ngx_stream_pass_module.c
> > > new file mode 100644
> > > --- /dev/null
> > > +++ b/src/stream/ngx_stream_pass_module.c
> > > @@ -0,0 +1,272 @@
> > > +
> > > +/*
> > > + * Copyright (C) Roman Arutyunyan
> > > + * Copyright (C) Nginx, Inc.
> > > + */
> > > +
> > > +
> > > +#include <ngx_config.h>
> > > +#include <ngx_core.h>
> > > +#include <ngx_stream.h>
> > > +
> > > +
> > > +typedef struct {
> > > +    ngx_addr_t                  *addr;
> > > +    ngx_stream_complex_value_t  *addr_value;
> > > +} ngx_stream_pass_srv_conf_t;
> > > +
> > > +
> > > +static void ngx_stream_pass_handler(ngx_stream_session_t *s);
> > > +static ngx_int_t ngx_stream_pass_match(ngx_listening_t *ls, ngx_addr_t *addr);
> > > +static void *ngx_stream_pass_create_srv_conf(ngx_conf_t *cf);
> > > +static char *ngx_stream_pass(ngx_conf_t *cf, ngx_command_t *cmd, void *conf);
> > > +
> > > +
> > > +static ngx_command_t  ngx_stream_pass_commands[] = {
> > > +
> > > +    { ngx_string("pass"),
> > > +      NGX_STREAM_SRV_CONF|NGX_CONF_TAKE1,
> > > +      ngx_stream_pass,
> > > +      NGX_STREAM_SRV_CONF_OFFSET,
> > > +      0,
> > > +      NULL },
> > > +
> > > +      ngx_null_command
> > > +};
> > > +
> > > +
> > > +static ngx_stream_module_t  ngx_stream_pass_module_ctx = {
> > > +    NULL,                                  /* preconfiguration */
> > > +    NULL,                                  /* postconfiguration */
> > > +
> > > +    NULL,                                  /* create main configuration */
> > > +    NULL,                                  /* init main configuration */
> > > +
> > > +    ngx_stream_pass_create_srv_conf,       /* create server configuration */
> > > +    NULL                                   /* merge server configuration */
> > > +};
> > > +
> > > +
> > > +ngx_module_t  ngx_stream_pass_module = {
> > > +    NGX_MODULE_V1,
> > > +    &ngx_stream_pass_module_ctx,           /* module context */
> > > +    ngx_stream_pass_commands,              /* module directives */
> > > +    NGX_STREAM_MODULE,                     /* module type */
> > > +    NULL,                                  /* init master */
> > > +    NULL,                                  /* init module */
> > > +    NULL,                                  /* init process */
> > > +    NULL,                                  /* init thread */
> > > +    NULL,                                  /* exit thread */
> > > +    NULL,                                  /* exit process */
> > > +    NULL,                                  /* exit master */
> > > +    NGX_MODULE_V1_PADDING
> > > +};
> > > +
> > > +
> > > +static void
> > > +ngx_stream_pass_handler(ngx_stream_session_t *s)
> > > +{
> > > +    ngx_url_t                    u;
> > > +    ngx_str_t                    url;
> > > +    ngx_addr_t                  *addr;
> > > +    ngx_uint_t                   i;
> > > +    ngx_listening_t             *ls;
> > > +    struct sockaddr             *sa;
> > > +    ngx_connection_t            *c;
> > > +    ngx_stream_pass_srv_conf_t  *pscf;
> > > +
> > > +    c = s->connection;
> > > +
> > > +    c->log->action = "passing connection to port";
> > > +
> > > +    if (c->buffer && c->buffer->pos != c->buffer->last) {
> > > +        ngx_log_error(NGX_LOG_ERR, c->log, 0,
> > > +                      "cannot pass connection with preread data");
> > > +        goto failed;
> > > +    }
> > > +
> > > +    pscf = ngx_stream_get_module_srv_conf(s, ngx_stream_pass_module);
> > > +
> > > +    addr = pscf->addr;
> > > +
> > > +    if (addr == NULL) {
> > > +        if (ngx_stream_complex_value(s, pscf->addr_value, &url) != NGX_OK) {
> > > +            goto failed;
> > > +        }
> > > +
> > > +        ngx_memzero(&u, sizeof(ngx_url_t));
> > > +
> > > +        u.url = url;
> > > +        u.no_resolve = 1;
> > 
> > This makes configurations with variables of limited use.
> 
> The functionality is indeed different for static and dynamic addresses.
> Currently it's the best we can do without introducing more complexity.
> We can add dynamic resolving here like in the upstream module and eliminate the
> confusion.  However a better way to eliminate it is to disable resolve
> completely for both static and dynamic configurations.  It seems to me,
> resolving names in the pass module makes little sense.  All addresses are
> local anyway.

Agree.

> 
> > > +        if (ngx_parse_url(c->pool, &u) != NGX_OK) {
> > > +            if (u.err) {
> > > +                ngx_log_error(NGX_LOG_ERR, c->log, 0,
> > > +                              "%s in pass \"%V\"", u.err, &u.url);
> > > +            }
> > > +
> > > +            goto failed;
> > > +        }
> > > +
> > > +        if (u.naddrs == 0) {
> > > +            ngx_log_error(NGX_LOG_ERR, c->log, 0,
> > > +                          "no addresses in pass \"%V\"", &u.url);
> > > +            goto failed;
> > > +        }
> > > +
> > > +        addr = &u.addrs[0];
> > > +    }
> > > +
> > > +    ngx_log_debug1(NGX_LOG_DEBUG_STREAM, c->log, 0,
> > > +                   "stream pass addr: \"%V\"", &addr->name);
> > > +
> > > +    ls = ngx_cycle->listening.elts;
> > > +
> > > +    for (i = 0; i < ngx_cycle->listening.nelts; i++) {
> > > +
> > > +        if (ngx_stream_pass_match(&ls[i], addr) != NGX_OK) {
> > > +            continue;
> > > +        }
> > > +
> > > +        c->listening = &ls[i];
> > > +
> > > +        c->data = NULL;
> > > +        c->buffer = NULL;
> > > +
> > > +        *c->log = c->listening->log;
> > > +        c->log->handler = NULL;
> > > +        c->log->data = NULL;
> > > +
> > > +        sa = ngx_palloc(c->pool, addr->socklen);
> > > +        if (sa == NULL) {
> > > +            goto failed;
> > > +        }
> > 
> > Is there a reason to (re-)allocate memory for c->local_sockaddr ?
> > 
> > Either way, "addr" is stored in some pool, allocated in ngx_parse_url()
> > through ngx_inet_add_addr().  It should be safe to reference it there.
> 
> Sure, removed the allocation.
> 
> > > +        ngx_memcpy(sa, addr->sockaddr, addr->socklen);
> > > +        c->local_sockaddr = sa;
> > > +        c->local_socklen = addr->socklen;
> > > +
> > > +        c->listening->handler(c);
> > > +
> > > +        return;
> > > +    }
> > > +
> > > +    ngx_log_error(NGX_LOG_ERR, c->log, 0,
> > > +                  "port not found for \"%V\"", &addr->name);
> > > +
> > > +    ngx_stream_finalize_session(s, NGX_STREAM_OK);
> > > +
> > > +    return;
> > > +
> > > +failed:
> > > +
> > > +    ngx_stream_finalize_session(s, NGX_STREAM_INTERNAL_SERVER_ERROR);
> > > +}
> > > +
> > > +
> > > +static ngx_int_t
> > > +ngx_stream_pass_match(ngx_listening_t *ls, ngx_addr_t *addr)
> > > +{
> > > +    if (!ls->wildcard) {
> > > +        return ngx_cmp_sockaddr(ls->sockaddr, ls->socklen,
> > > +                                addr->sockaddr, addr->socklen, 1);
> > > +    }
> > > +
> > > +    if (ls->sockaddr->sa_family == addr->sockaddr->sa_family
> > > +        && ngx_inet_get_port(ls->sockaddr) == ngx_inet_get_port(addr->sockaddr))
> > > +    {
> > > +        return NGX_OK;
> > > +    }
> > > +
> > > +    return NGX_DECLINED;
> > > +}
> > > +
> > > +
> > > +static void *
> > > +ngx_stream_pass_create_srv_conf(ngx_conf_t *cf)
> > > +{
> > > +    ngx_stream_pass_srv_conf_t  *conf;
> > > +
> > > +    conf = ngx_pcalloc(cf->pool, sizeof(ngx_stream_pass_srv_conf_t));
> > > +    if (conf == NULL) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    /*
> > > +     * set by ngx_pcalloc():
> > > +     *
> > > +     *     conf->addr = NULL;
> > > +     *     conf->addr_value = NULL;
> > > +     */
> > > +
> > > +    return conf;
> > > +}
> > > +
> > > +
> > > +static char *
> > > +ngx_stream_pass(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> > > +{
> > > +    ngx_stream_pass_srv_conf_t *pscf = conf;
> > > +
> > > +    ngx_url_t                            u;
> > > +    ngx_str_t                           *value, *url;
> > > +    ngx_stream_complex_value_t           cv;
> > > +    ngx_stream_core_srv_conf_t          *cscf;
> > > +    ngx_stream_compile_complex_value_t   ccv;
> > > +
> > > +    if (pscf->addr || pscf->addr_value) {
> > > +        return "is duplicate";
> > > +    }
> > > +
> > > +    cscf = ngx_stream_conf_get_module_srv_conf(cf, ngx_stream_core_module);
> > > +
> > > +    cscf->handler = ngx_stream_pass_handler;
> > > +
> > > +    value = cf->args->elts;
> > > +
> > > +    url = &value[1];
> > > +
> > > +    ngx_memzero(&ccv, sizeof(ngx_stream_compile_complex_value_t));
> > > +
> > > +    ccv.cf = cf;
> > > +    ccv.value = url;
> > > +    ccv.complex_value = &cv;
> > > +
> > > +    if (ngx_stream_compile_complex_value(&ccv) != NGX_OK) {
> > > +        return NGX_CONF_ERROR;
> > > +    }
> > > +
> > > +    if (cv.lengths) {
> > > +        pscf->addr_value = ngx_palloc(cf->pool,
> > > +                                      sizeof(ngx_stream_complex_value_t));
> > > +        if (pscf->addr_value == NULL) {
> > > +            return NGX_CONF_ERROR;
> > > +        }
> > > +
> > > +        *pscf->addr_value = cv;
> > > +
> > > +        return NGX_CONF_OK;
> > > +    }
> > > +
> > > +    ngx_memzero(&u, sizeof(ngx_url_t));
> > > +
> > > +    u.url = *url;
> > > +
> > > +    if (ngx_parse_url(cf->pool, &u) != NGX_OK) {
> > > +        if (u.err) {
> > > +            ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > > +                               "%s in \"%V\" of the \"pass\" directive",
> > > +                               u.err, &u.url);
> > > +        }
> > > +
> > > +        return NGX_CONF_ERROR;
> > > +    }
> > 
> > Although you've changed the commit example from "pass 8081" to
> > "pass 127.0.0.1:8001", the former syntax is still allowed.
> > 
> > This may be misleading: with the current code, unlike "u.listen = 1",
> > this means "8081" will be tested as an address (without a port)
> > written in a decimal format, as finally resolved by getaddrinfo(3).
> > So, using "pass 8081" corresponds to 0x1f91, or "0.0.31.145".
> > Further, since it has no port, it will never match listen addresses.
> 
> Similarly we can do this in the http proxy module: "proxy_pass http://8081".
> Of course there's a default port there, but it does not change the fact
> that listen-like syntax is allowed in proxy_pass, and produces misleading
> results as well.
> 
> > I'd check and forbid this explicitly:
> > 
> >     if (u.no_port) {
> >         return "has no port";
> >     }
> 
> Makes sense, thanks.
> 
> > > +
> > > +    if (u.naddrs == 0) {
> > > +        return "has no addresses";
> > > +    }
> > 
> > It seems that this check can never happen if neither "u.no_resolve"
> > nor "u.listen" set, such as in here.
> > In the worst case, when the address couldn't be parsed as a literal,
> > and ngx_parse_url() falls back to ngx_inet_resolve_host() to resolve
> > as a name, which is either NXDOMAIN or has none of A/AAAA records,
> > then ngx_parse_url() will return an error with the "host not found"
> > diagnostics.  It looks like another left-over from "u.listen = 1".
> 
> If we add "u.no_resolve = 1", as discussed above, this condition will make
> sense again.

Ok.

> 
> > > +    pscf->addr = &u.addrs[0];
> > > +
> > > +    return NGX_CONF_OK;
> > > +}
> 
> Diff attached.
> 
> --
> Roman Arutyunyan

> # HG changeset patch
> # User Roman Arutyunyan <arut at nginx.com>
> # Date 1709125245 -14400
> #      Wed Feb 28 17:00:45 2024 +0400
> # Node ID f533e218c56a1d14be02c0b81409bcc12bed3562
> # Parent  44da04c2d4db94ad4eefa84b299e07c5fa4a00b9
> imported patch stream-pass-fix1
> 
> diff --git a/src/stream/ngx_stream_pass_module.c b/src/stream/ngx_stream_pass_module.c
> --- a/src/stream/ngx_stream_pass_module.c
> +++ b/src/stream/ngx_stream_pass_module.c
> @@ -71,7 +71,6 @@ ngx_stream_pass_handler(ngx_stream_sessi
>      ngx_addr_t                  *addr;
>      ngx_uint_t                   i;
>      ngx_listening_t             *ls;
> -    struct sockaddr             *sa;
>      ngx_connection_t            *c;
>      ngx_stream_pass_srv_conf_t  *pscf;
>  
> @@ -114,6 +113,12 @@ ngx_stream_pass_handler(ngx_stream_sessi
>              goto failed;
>          }
>  
> +        if (u.no_port) {
> +            ngx_log_error(NGX_LOG_ERR, c->log, 0,
> +                          "no port in pass \"%V\"", &u.url);
> +            goto failed;
> +        }
> +
>          addr = &u.addrs[0];
>      }
>  
> @@ -137,13 +142,7 @@ ngx_stream_pass_handler(ngx_stream_sessi
>          c->log->handler = NULL;
>          c->log->data = NULL;
>  
> -        sa = ngx_palloc(c->pool, addr->socklen);
> -        if (sa == NULL) {
> -            goto failed;
> -        }
> -
> -        ngx_memcpy(sa, addr->sockaddr, addr->socklen);
> -        c->local_sockaddr = sa;
> +        c->local_sockaddr = addr->sockaddr;
>          c->local_socklen = addr->socklen;
>  
>          c->listening->handler(c);
> @@ -251,6 +250,7 @@ ngx_stream_pass(ngx_conf_t *cf, ngx_comm
>      ngx_memzero(&u, sizeof(ngx_url_t));
>  
>      u.url = *url;
> +    u.no_resolve = 1;
>  
>      if (ngx_parse_url(cf->pool, &u) != NGX_OK) {
>          if (u.err) {
> @@ -266,6 +266,10 @@ ngx_stream_pass(ngx_conf_t *cf, ngx_comm
>          return "has no addresses";
>      }
>  
> +    if (u.no_port) {
> +        return "has no port";
> +    }
> +
>      pscf->addr = &u.addrs[0];
>  
>      return NGX_CONF_OK;

Looks good to me.


More information about the nginx-devel mailing list