[PATCH] The "sort=" parameter of the "resolver" directive

Sergey Kandaurov pluknet at nginx.com
Tue Jun 28 16:00:05 UTC 2022


On Tue, Jun 28, 2022 at 04:24:58PM +0400, Roman Arutyunyan wrote:
> On Thu, Jun 16, 2022 at 07:17:13PM +0400, Sergey Kandaurov wrote:
> > 
> > > On 23 Feb 2022, at 08:11, Ruslan Ermilov <ru at nginx.com> wrote:
> > > 
> > > src/core/ngx_resolver.c |  38 +++++++++++++++++++++++++++++++++++++-
> > > src/core/ngx_resolver.h |   5 +++++
> > > 2 files changed, 42 insertions(+), 1 deletions(-)
> > > 
> > > 
> > > # HG changeset patch
> > > # User Ruslan Ermilov <ru at nginx.com>
> > > # Date 1645589387 -10800
> > > #      Wed Feb 23 07:09:47 2022 +0300
> > > # Node ID 8db4bbd67840e8bebb23f9c6d10c0f633552e616
> > > # Parent  1c19779448db2309d607c74e2628ff98f84569ff
> > > The "sort=" parameter of the "resolver" directive.
> > > 
> > 
> > IMHO, the name isn't enough self-documenting in this context.
> > It can be misinterpreted as sorting addresses, not families.
> > I'd prefer the parameter name "prefer" here.
> 
> Jftr, we had a discussion about this.  I like the new name.  There's no
> sorting here.
> 
> > > diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
> > > --- a/src/core/ngx_resolver.c
> > > +++ b/src/core/ngx_resolver.c
> > > @@ -266,6 +266,27 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
> > >         }
> > > #endif
> > > 
> > > +        if (ngx_strncmp(names[i].data, "sort=", 5) == 0) {
> > > +
> > > +            if (ngx_strcasecmp(&names[i].data[5], (u_char *) "ipv4") == 0) {
> > 
> > The same question raises about case-sensitivity.
> > I prefer to make it case-sensitive, like other similar directive parameters.
> > 
> > > +                r->sort = NGX_RESOLVE_A_FIRST;
> > > +
> > > +#if (NGX_HAVE_INET6)
> > > +            } else if (ngx_strcasecmp(&names[i].data[5], (u_char *) "ipv6")
> > > +                       == 0)
> > > +            {
> > > +                r->sort = NGX_RESOLVE_AAAA_FIRST;
> > > +#endif
> > > +
> > > +            } else {
> > > +                ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > > +                                   "invalid parameter: %V", &names[i]);
> > > +                return NULL;
> > > +            }
> > > +
> > > +            continue;
> > > +        }
> > > +
> > 
> > What about to put the whole block under NGX_HAVE_INET6?
> > If built without ipv6 support it makes same little sense
> > as for "ipv6" and "ipv4" parameters that already there.
> > 
> > >         ngx_memzero(&u, sizeof(ngx_url_t));
> > > 
> > >         u.url = names[i];
> > > @@ -4253,7 +4274,22 @@ ngx_resolver_export(ngx_resolver_t *r, n
> > >     }
> > > 
> > >     i = 0;
> > > -    d = rotate ? ngx_random() % n : 0;
> > > +
> > > +    if (r->sort == NGX_RESOLVE_A_FIRST) {
> > > +        d = 0;
> > > +
> > > +#if (NGX_HAVE_INET6)
> > > +    } else if (r->sort == NGX_RESOLVE_AAAA_FIRST) {
> > > +        d = rn->naddrs6;
> > > +
> > > +        if (d == n) {
> > > +            d = 0;
> > > +        }
> > > +#endif
> > > +
> > > +    } else {
> > > +        d = rotate ? ngx_random() % n : 0;
> > > +    }
> > > 
> > >     if (rn->naddrs) {
> > >         j = rotate ? ngx_random() % rn->naddrs : 0;
> > > diff --git a/src/core/ngx_resolver.h b/src/core/ngx_resolver.h
> > > --- a/src/core/ngx_resolver.h
> > > +++ b/src/core/ngx_resolver.h
> > > @@ -36,6 +36,9 @@
> > > 
> > > #define NGX_RESOLVER_MAX_RECURSION    50
> > > 
> > > +#define NGX_RESOLVE_A_FIRST           1
> > > +#define NGX_RESOLVE_AAAA_FIRST        2
> > > +
> > 
> > I'd adjust name for better sorting.
> > 
> > > 
> > > typedef struct ngx_resolver_s  ngx_resolver_t;
> > > 
> > > @@ -185,6 +188,8 @@ struct ngx_resolver_s {
> > >     ngx_queue_t               addr6_expire_queue;
> > > #endif
> > > 
> > > +    ngx_uint_t                sort;
> > > +
> > 
> > Since the previous patch introduces ipv4/ipv6 bit fields,
> > it makes sense to make this one a bit field as well.
> > 
> > >     time_t                    resend_timeout;
> > >     time_t                    tcp_timeout;
> > >     time_t                    expire;
> > > 
> > 
> > diff --git a/src/core/ngx_resolver.c b/src/core/ngx_resolver.c
> > --- a/src/core/ngx_resolver.c
> > +++ b/src/core/ngx_resolver.c
> > @@ -227,6 +227,7 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
> >          }
> >  
> >  #if (NGX_HAVE_INET6)
> > +
> >          if (ngx_strncmp(names[i].data, "ipv4=", 5) == 0) {
> >  
> >              if (ngx_strcmp(&names[i].data[5], "on") == 0) {
> > @@ -260,6 +261,24 @@ ngx_resolver_create(ngx_conf_t *cf, ngx_
> >  
> >              continue;
> >          }
> > +
> > +        if (ngx_strncmp(names[i].data, "prefer=", 7) == 0) {
> > +
> > +            if (ngx_strcmp(&names[i].data[7], "ipv4") == 0) {
> > +                r->prefer = NGX_RESOLVE_PREFER_A;
> > +
> > +            } else if (ngx_strcmp(&names[i].data[7], "ipv6") == 0) {
> > +                r->prefer = NGX_RESOLVE_PREFER_AAAA;
> > +
> > +            } else {
> > +                ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > +                                   "invalid parameter: %V", &names[i]);
> > +                return NULL;
> > +            }
> > +
> > +            continue;
> > +        }
> > +
> >  #endif
> >  
> >          ngx_memzero(&u, sizeof(ngx_url_t));
> > @@ -4250,7 +4269,22 @@ ngx_resolver_export(ngx_resolver_t *r, n
> >      }
> >  
> >      i = 0;
> > -    d = rotate ? ngx_random() % n : 0;
> > +
> > +    if (r->prefer == NGX_RESOLVE_PREFER_A) {
> > +        d = 0;
> > +
> 
> If we put "prefer=" handler under NGX_HAVE_INET6, the block above should
> also go under NGX_HAVE_INET6.  All 3 blocks could even be substituted with
> a switch for simplicity.

I agree it will never match in the no-inet6 case.
Please apply on top off:

diff -r 5210f97325ed src/core/ngx_resolver.c
--- a/src/core/ngx_resolver.c	Tue Jun 28 19:36:18 2022 +0400
+++ b/src/core/ngx_resolver.c	Tue Jun 28 19:58:26 2022 +0400
@@ -4270,19 +4270,24 @@ ngx_resolver_export(ngx_resolver_t *r, n
 
     i = 0;
 
-    if (r->prefer == NGX_RESOLVE_PREFER_A) {
-        d = 0;
+    switch (r->prefer) {
 
 #if (NGX_HAVE_INET6)
-    } else if (r->prefer == NGX_RESOLVE_PREFER_AAAA) {
+    case NGX_RESOLVE_PREFER_A:
+        d = 0;
+        break;
+
+    case NGX_RESOLVE_PREFER_AAAA:
         d = rn->naddrs6;
 
         if (d == n) {
             d = 0;
         }
+
+        break;
 #endif
 
-    } else {
+    default:
         d = rotate ? ngx_random() % n : 0;
     }
 

> 
> > +#if (NGX_HAVE_INET6)
> > +    } else if (r->prefer == NGX_RESOLVE_PREFER_AAAA) {
> > +        d = rn->naddrs6;
> > +
> > +        if (d == n) {
> > +            d = 0;
> > +        }
> > +#endif
> > +
> > +    } else {
> > +        d = rotate ? ngx_random() % n : 0;
> > +    }
> >  
> >      if (rn->naddrs) {
> >          j = rotate ? ngx_random() % rn->naddrs : 0;
> > diff --git a/src/core/ngx_resolver.h b/src/core/ngx_resolver.h
> > --- a/src/core/ngx_resolver.h
> > +++ b/src/core/ngx_resolver.h
> > @@ -36,6 +36,9 @@
> >  
> >  #define NGX_RESOLVER_MAX_RECURSION    50
> >  
> > +#define NGX_RESOLVE_PREFER_A          1
> > +#define NGX_RESOLVE_PREFER_AAAA       2
> > +
> >  
> >  typedef struct ngx_resolver_s  ngx_resolver_t;
> >  
> > @@ -175,6 +178,8 @@ struct ngx_resolver_s {
> >      ngx_queue_t               srv_expire_queue;
> >      ngx_queue_t               addr_expire_queue;
> >  
> > +    unsigned                  prefer:2;
> > +
> >      unsigned                  ipv4:1;
> >  
> >  #if (NGX_HAVE_INET6)
> > 



More information about the nginx-devel mailing list