On Fri, 2020-05-01 at 16:52 +0200, Harald Jensas wrote:
> On Fri, 1 May 2020, 16:28 Petr Menšík, <pemen...@redhat.com> wrote:
> > Hi Harald and Geert,
> > 
> > I looked at it and it is quite confusing what it does. Not quite
> > simple
> > to understand. I think it is not uncommon for dnsmasq code, but
> > this
> > brings more knobs to make it more complicated.
> > 
> > I tried to rewrite it hopefully with less repetitions and cleaner
> > code.
> > However when I did this, I have realized current algorihm is quite
> > bad.
> > 
> > When dnsmasq would have 40 client records for dhcp-host, it would
> > have
> > to walk throught that list about 4*40 times for every DHCP request.
> > It
> > seems to me smarter sorting of dhcp_configs would allow single or
> > two
> > passes. Just push the most specific records with client ids and
> > hwaddr
> > first, then just hostnames.
> > 
> > Anyway, I think some unit test would be needed. Any opinions on my
> > patches. Have not yet tested them in real network.
> > 
> > Cheers,
> > Petr
> 
> Hi Petr,
> 
> I looked at the patches, I think the refactoring looks good.
> 
> I am slightly worried about the sorting, I know openstack neutron
> does sorting to ensure IPv4 records come before records without
> address (IPv6 stateless). Look at the docstring[1] for details. In
> the future I would like to use tag:dhcpv6 in neutron instead of
> sorting, but that is'nt really possible before most distros have a
> version of dnsmasq with support for tags in host definitions.
> 

On second review of this, afaict the matching based on address family
makes the sorting currently done in Openstack neutron redundant. With
these patches dnsmasq will prefer the entry with the IPv4 address, thus
sorting the hosts file to ensure the IPv4 entry is "first" would no
longer be required. A quick test verifies this is the case, i.e both
configs below allow the DHCPv4 client to succeed:

 # IPv4 entry last:
  dhcp-host=52:54:00:bc:c3:fd,set:foo-tag,host2-foo
  dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
 # IPv4 entry first:
  dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
  dhcp-host=52:54:00:bc:c3:fd,set:foo-tag,host2-foo

On master the configuration with the IPv4 entry first fails.

> Today is a public holiday for me, and I am away from computers. I
> will do some testing as soon as I am back home.
> 

I did some tests with Petr's two patches applied:

Both clients succeed regardless of order in config:

  dhcp-host=52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2
  dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
and
  dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
  dhcp-host=52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2



Using a prefix for IPv6 still works:

  dhcp-host=52:54:00:3f:5c:c0,[fd12:3456:789a:1::aa08/126],host1

Using a list for IPv6 still works:

dhcp-host=52:54:00:3f:5c:c0,[fd12:3456:789a:1::aa02],[fd12:3456:789a:1::aa04],[fd12:3456:789a:1::aa06],host1



My conclusion is that Petr's proposed patches are good.

> [1] 
> https://opendev.org/openstack/neutron/src/branch/master/neutron/agent/linux/dhcp.py#L572
> 
> 
> //
> Harald
> 
> > On 4/30/20 11:07 AM, Harald Jensås wrote:
> > > On Wed, 2020-04-29 at 16:18 +0200, Harald Jensås wrote:
> > >> Hi,
> > >>
> > >> We discovered an issue introduced in this commit:
> > >> 137286e9baecf6a3ba97722ef1b49c851b531810
> > >>
> > >> Prior to this commit one could have two dhcp-host entries, one
> > for
> > >> IPv4
> > >> and another for IPv6, for example:
> > >>
> > >>  dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
> > >>  dhcp-host=52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2
> > >>
> > >> This no longer works. In the above example dhcpv6 client
> > succedes,
> > >> but
> > >> the dhcpv4 client get 'no address available'. Swapping the order
> > of
> > >> the
> > >> two entries in the config file allow the dhcpv4 client to
> > succeed,
> > >> but
> > >> then the dhcpv5 client fails.
> > >>
> > >> Alternative configurations that do work in 2.81:
> > >>
> > >>  dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
> > >>  dhcp-
> > >> host=tag:dhcpv6,52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2
> > >>
> > >> or: 
> > >>
> > >>  dhcp-
> > >>
> > host=52:54:00:bc:c3:fd,172.20.0.11,[fd12:3456:789a:1::aadd],host2
> > >>
> > >>
> > >> I'm not sure using two dhcp-host entries was ever intended to
> > work,
> > >> as
> > >> the manual page states that both ipv4 and ipv6 address can be
> > defined
> > >> in a single entry.
> > >>
> > >> A first tought for possible fix would be to internally set the
> > >> 'tag:dhcpv6' for any dhcp-host entry with only ipv6 addresse(s).
> > >>
> > > 
> > > An attempt for a fix, preferring dhcp-host entries with
> > CONFIG_ADDR or
> > > CONFIG_ADDR6 flag set. With this change I can use these two
> > configs
> > > successfully:
> > > 
> > >   dhcp-
> > host=52:54:00:bc:c3:fd,172.20.0.11,[fd12:3456:789a:1::aadd],host2
> > > or
> > >   dhcp-host=52:54:00:bc:c3:fd,[fd12:3456:789a:1::aadd],host2
> > >   dhcp-host=52:54:00:bc:c3:fd,172.20.0.11,host2
> > > 
> > > (The latter works in 2.80, but fails in 2.81)
> > > 
> > > 
> > > 
> > > From 13de4c3346462146ce3826a7fc0546fdb5a569b0 Mon Sep 17 00:00:00
> > 2001
> > > From: Harald Jensås <hjen...@redhat.com>
> > > Date: Thu, 30 Apr 2020 10:42:07 +0200
> > > Subject: [PATCH 1/1] Tag filtering in find_config, filter on IP
> > version
> > > 
> > > Adds the possibility to filter on a config flag when
> > > looking up configuration.
> > > 
> > > Update the DHCP and DHCPv6 code to prefer dhcp-host
> > > entries in config with CONFIG_ADDR/CONFIG_ADDR6 flag
> > > set.
> > > 
> > > This fixes a regression in 2.81 where using one dhcp-host
> > > entry for IPv4 and another for IPv6 no longer work.
> > > 
> > > Related: rhbz#1829448
> > > ---
> > >  src/dhcp-common.c | 48 +++++++++++++++++++++++++++++++++++++--
> > --------
> > >  src/dnsmasq.h     |  3 ++-
> > >  src/lease.c       |  2 +-
> > >  src/rfc2131.c     |  6 +++---
> > >  src/rfc3315.c     |  6 +++---
> > >  5 files changed, 47 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/src/dhcp-common.c b/src/dhcp-common.c
> > > index eae9886..1277fa6 100644
> > > --- a/src/dhcp-common.c
> > > +++ b/src/dhcp-common.c
> > > @@ -192,6 +192,18 @@ int match_netid(struct dhcp_netid *check,
> > struct dhcp_netid *pool, int tagnotnee
> > >    return 1;
> > >  }
> > >  
> > > +/* Is the config flag set. */
> > > +int match_flag(unsigned int flags, unsigned int flag, int
> > flagnotneeded)
> > > +{
> > > +  if (flagnotneeded)
> > > +    return 1;
> > > +  
> > > +  if (flags & flag)
> > > +    return 1;
> > > +  
> > > +  return 0;
> > > +}
> > > +
> > >  /* return domain or NULL if none. */
> > >  char *strip_hostname(char *hostname)
> > >  {
> > > @@ -318,7 +330,8 @@ static struct dhcp_config
> > *find_config_match(struct dhcp_config *configs,
> > >                                            unsigned char *clid,
> > int clid_len,
> > >                                            unsigned char *hwaddr,
> > int hw_len, 
> > >                                            int hw_type, char
> > *hostname,
> > > -                                          struct dhcp_netid
> > *tags, int tag_not_needed)
> > > +                                          struct dhcp_netid
> > *tags, int tag_not_needed,
> > > +               unsigned int flag, int flag_not_needed)
> > >  {
> > >    int count, new;
> > >    struct dhcp_config *config, *candidate; 
> > > @@ -331,7 +344,8 @@ static struct dhcp_config
> > *find_config_match(struct dhcp_config *configs,
> > >         if (config->clid_len == clid_len && 
> > >             memcmp(config->clid, clid, clid_len) == 0 &&
> > >             is_config_in_context(context, config) &&
> > > -           match_netid(config->filter, tags, tag_not_needed))
> > > +           match_netid(config->filter, tags, tag_not_needed) &&
> > > +        match_flag(config->flags, flag, flag_not_needed))
> > >           
> > >           return config;
> > >         
> > > @@ -341,7 +355,8 @@ static struct dhcp_config
> > *find_config_match(struct dhcp_config *configs,
> > >         if ((!context || !(context->flags & CONTEXT_V6)) && *clid
> > == 0 && config->clid_len == clid_len-1  &&
> > >             memcmp(config->clid, clid+1, clid_len-1) == 0 &&
> > >             is_config_in_context(context, config) &&
> > > -           match_netid(config->filter, tags, tag_not_needed))
> > > +           match_netid(config->filter, tags, tag_not_needed) &&
> > > +        match_flag(config->flags, flag, flag_not_needed))
> > >           return config;
> > >       }
> > >    
> > > @@ -350,7 +365,8 @@ static struct dhcp_config
> > *find_config_match(struct dhcp_config *configs,
> > >      for (config = configs; config; config = config->next)
> > >        if (config_has_mac(config, hwaddr, hw_len, hw_type) &&
> > >         is_config_in_context(context, config) &&
> > > -       match_netid(config->filter, tags, tag_not_needed))
> > > +       match_netid(config->filter, tags, tag_not_needed) &&
> > > +    match_flag(config->flags, flag, flag_not_needed))
> > >       return config;
> > >    
> > >    if (hostname && context)
> > > @@ -358,7 +374,8 @@ static struct dhcp_config
> > *find_config_match(struct dhcp_config *configs,
> > >        if ((config->flags & CONFIG_NAME) && 
> > >         hostname_isequal(config->hostname, hostname) &&
> > >         is_config_in_context(context, config) &&
> > > -       match_netid(config->filter, tags, tag_not_needed))
> > > +       match_netid(config->filter, tags, tag_not_needed) &&
> > > +    match_flag(config->flags, flag, flag_not_needed))
> > >       return config;
> > >  
> > >    
> > > @@ -368,7 +385,8 @@ static struct dhcp_config
> > *find_config_match(struct dhcp_config *configs,
> > >    /* use match with fewest wildcard octets */
> > >    for (candidate = NULL, count = 0, config = configs; config;
> > config = config->next)
> > >      if (is_config_in_context(context, config) &&
> > > -     match_netid(config->filter, tags, tag_not_needed))
> > > +     match_netid(config->filter, tags, tag_not_needed) &&
> > > +  match_flag(config->flags, flag, flag_not_needed))
> > >        for (conf_addr = config->hwaddr; conf_addr; conf_addr =
> > conf_addr->next)
> > >       if (conf_addr->wildcard_mask != 0 &&
> > >           conf_addr->hwaddr_len == hw_len &&  
> > > @@ -382,17 +400,27 @@ static struct dhcp_config
> > *find_config_match(struct dhcp_config *configs,
> > >    return candidate;
> > >  }
> > >  
> > > -/* Find tagged configs first. */
> > > +/* Find tagged configs with flags first. */
> > >  struct dhcp_config *find_config(struct dhcp_config *configs,
> > >                               struct dhcp_context *context,
> > >                               unsigned char *clid, int clid_len,
> > >                               unsigned char *hwaddr, int hw_len, 
> > > -                             int hw_type, char *hostname, struct
> > dhcp_netid *tags)
> > > +                             int hw_type, char *hostname, struct
> > dhcp_netid *tags, unsigned int flag)
> > >  {
> > > -  struct dhcp_config *ret = find_config_match(configs, context,
> > clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 0);
> > > +  /* Find tagged config with flags */
> > > +  struct dhcp_config *ret = find_config_match(configs, context,
> > clid, clid_len, hwaddr, hw_len, hw_type, hostname, tags, 0, flag,
> > 0);
> > > +
> > > +  /* Find tagged config without flags */
> > > +  if (!ret)
> > > +    ret = find_config_match(configs, context, clid, clid_len,
> > hwaddr, hw_len, hw_type, hostname, tags, 0, flag, 1);
> > > +
> > > +  /* Find untagged config with flags */
> > > +  if (!ret)
> > > +    ret = find_config_match(configs, context, clid, clid_len,
> > hwaddr, hw_len, hw_type, hostname, tags, 1, flag, 0);
> > >  
> > > +  /* Find untagged config without flags */
> > >    if (!ret)
> > > -    ret = find_config_match(configs, context, clid, clid_len,
> > hwaddr, hw_len, hw_type, hostname, tags, 1);
> > > +    ret = find_config_match(configs, context, clid, clid_len,
> > hwaddr, hw_len, hw_type, hostname, tags, 1, flag, 1);
> > >  
> > >    return ret;
> > >  }
> > > diff --git a/src/dnsmasq.h b/src/dnsmasq.h
> > > index 18c381e..bdb085b 100644
> > > --- a/src/dnsmasq.h
> > > +++ b/src/dnsmasq.h
> > > @@ -1571,7 +1571,8 @@ struct dhcp_config *find_config(struct
> > dhcp_config *configs,
> > >                               unsigned char *clid, int clid_len,
> > >                               unsigned char *hwaddr, int hw_len, 
> > >                               int hw_type, char *hostname,
> > > -                             struct dhcp_netid *filter);
> > > +                             struct dhcp_netid *filter,
> > > +        unsigned int flag);
> > >  int config_has_mac(struct dhcp_config *config, unsigned char
> > *hwaddr, int len, int type);
> > >  #ifdef HAVE_LINUX_NETWORK
> > >  char *whichdevice(void);
> > > diff --git a/src/lease.c b/src/lease.c
> > > index 23e6fe0..917f7f3 100644
> > > --- a/src/lease.c
> > > +++ b/src/lease.c
> > > @@ -230,7 +230,7 @@ void lease_update_from_configs(void)
> > >      if (lease->flags & (LEASE_TA | LEASE_NA))
> > >        continue;
> > >      else if ((config = find_config(daemon->dhcp_conf, NULL,
> > lease->clid, lease->clid_len, 
> > > -                                lease->hwaddr, lease-
> > >hwaddr_len, lease->hwaddr_type, NULL, NULL)) && 
> > > +                                lease->hwaddr, lease-
> > >hwaddr_len, lease->hwaddr_type, NULL, NULL, 0)) && 
> > >            (config->flags & CONFIG_NAME) &&
> > >            (!(config->flags & CONFIG_ADDR) || config->addr.s_addr 
> > == lease->addr.s_addr))
> > >        lease_set_hostname(lease, config->hostname, 1,
> > get_domain(lease->addr), NULL);
> > > diff --git a/src/rfc2131.c b/src/rfc2131.c
> > > index fc54aab..22c2d63 100644
> > > --- a/src/rfc2131.c
> > > +++ b/src/rfc2131.c
> > > @@ -504,7 +504,7 @@ size_t dhcp_reply(struct dhcp_context
> > *context, char *iface_name, int int_index,
> > >    mess->op = BOOTREPLY;
> > >    
> > >    config = find_config(daemon->dhcp_conf, context, clid,
> > clid_len, 
> > > -                    mess->chaddr, mess->hlen, mess->htype, NULL,
> > run_tag_if(netid));
> > > +                    mess->chaddr, mess->hlen, mess->htype, NULL,
> > run_tag_if(netid), CONFIG_ADDR);
> > >  
> > >    /* set "known" tag for known hosts */
> > >    if (config)
> > > @@ -514,7 +514,7 @@ size_t dhcp_reply(struct dhcp_context
> > *context, char *iface_name, int int_index,
> > >        netid = &known_id;
> > >      }
> > >    else if (find_config(daemon->dhcp_conf, NULL, clid, clid_len, 
> > > -                    mess->chaddr, mess->hlen, mess->htype, NULL,
> > run_tag_if(netid)))
> > > +                    mess->chaddr, mess->hlen, mess->htype, NULL,
> > run_tag_if(netid), CONFIG_ADDR))
> > >      {
> > >        known_id.net = "known-othernet";
> > >        known_id.next = netid;
> > > @@ -781,7 +781,7 @@ size_t dhcp_reply(struct dhcp_context
> > *context, char *iface_name, int int_index,
> > >                to avoid impersonation by name. */
> > >             struct dhcp_config *new = find_config(daemon-
> > >dhcp_conf, context, NULL, 0,
> > >                                                   mess->chaddr,
> > mess->hlen, 
> > > -                                                 mess->htype,
> > hostname, run_tag_if(netid));
> > > +                                                 mess->htype,
> > hostname, run_tag_if(netid), CONFIG_ADDR);
> > >             if (new && !have_config(new, CONFIG_CLID) && !new-
> > >hwaddr)
> > >               {
> > >                 config = new;
> > > diff --git a/src/rfc3315.c b/src/rfc3315.c
> > > index b3f0a0a..27b0611 100644
> > > --- a/src/rfc3315.c
> > > +++ b/src/rfc3315.c
> > > @@ -527,7 +527,7 @@ static int dhcp6_no_relay(struct state
> > *state, int msg_type, void *inbuff, size_
> > >    
> > >    if (state->clid &&
> > >        (config = find_config(daemon->dhcp_conf, state->context,
> > state->clid, state->clid_len,
> > > -                         state->mac, state->mac_len, state-
> > >mac_type, NULL, run_tag_if(state->tags))) &&
> > > +                         state->mac, state->mac_len, state-
> > >mac_type, NULL, run_tag_if(state->tags), CONFIG_ADDR6)) &&
> > >        have_config(config, CONFIG_NAME))
> > >      {
> > >        state->hostname = config->hostname;
> > > @@ -547,7 +547,7 @@ static int dhcp6_no_relay(struct state
> > *state, int msg_type, void *inbuff, size_
> > >             /* Search again now we have a hostname. 
> > >                Only accept configs without CLID here, (it won't
> > match)
> > >                to avoid impersonation by name. */
> > > -           struct dhcp_config *new = find_config(daemon-
> > >dhcp_conf, state->context, NULL, 0, NULL, 0, 0, state->hostname,
> > run_tag_if(state->tags));
> > > +           struct dhcp_config *new = find_config(daemon-
> > >dhcp_conf, state->context, NULL, 0, NULL, 0, 0, state->hostname,
> > run_tag_if(state->tags), CONFIG_ADDR6);
> > >             if (new && !have_config(new, CONFIG_CLID) && !new-
> > >hwaddr)
> > >               config = new;
> > >           }
> > > @@ -574,7 +574,7 @@ static int dhcp6_no_relay(struct state
> > *state, int msg_type, void *inbuff, size_
> > >      }
> > >    else if (state->clid &&
> > >          find_config(daemon->dhcp_conf, NULL, state->clid, state-
> > >clid_len,
> > > -                    state->mac, state->mac_len, state->mac_type, 
> > NULL, run_tag_if(state->tags)))
> > > +                    state->mac, state->mac_len, state->mac_type, 
> > NULL, run_tag_if(state->tags), CONFIG_ADDR6))
> > >      {
> > >        known_id.net = "known-othernet";
> > >        known_id.next = state->tags;
> > > 
> > 


_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Reply via email to