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 <a...@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 <a...@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. _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel